On 16 Sep 2024, at 10:01, David Marchand wrote:
> On Fri, Sep 13, 2024 at 2:25 PM Kevin Traynor <[email protected]> wrote: >> >> Previously when a flow was attempted to be offloaded, if it >> could not be offloaded and did not return an actions error, >> a warning was logged. >> >> The reason there was an exception for an actions error was to allow >> for failure for full offload of a flow because it will fallback to >> partial offload. There are some issues with this approach to logging. >> >> Some NICs do not specify an actions error, because other config in >> the NIC may be checked first. e.g. In the case of Mellanox CX-5, >> there can be different types of offload configured, so an unspecified >> error may be returned. >> >> More generally, enabling hw-offload is best effort per datapath/NIC/flow >> as full and partial offload support in NICs is variable and there is >> always fallback to software. >> >> So there is likely to be repeated logging about offloading of flows >> failing. With this in mind, change the log level to debug. >> >> The status of the offload can still be seen with below command: >> $ ovs-appctl dpctl/dump-flows -m >> ... offloaded:partial ... >> >> Also, remove some duplicated rate limiting and tidy-up the succeed >> and failure logs. >> >> Signed-off-by: Kevin Traynor <[email protected]> >> >> --- >> v2: combined logs on failure and added confirmation of result on >> succeed. >> --- >> lib/netdev-offload-dpdk.c | 23 +++++++++-------------- >> 1 file changed, 9 insertions(+), 14 deletions(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index 623005b1c..342292d23 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -920,22 +920,17 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev, >> dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions); >> extra_str = ds_cstr(&s_extra); >> - VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR" %s flow create %d >> %s", >> - netdev_get_name(netdev), (intptr_t) flow, extra_str, >> - netdev_dpdk_get_port_id(netdev), ds_cstr(&s)); >> + VLOG_DBG("%s: rte_flow creation succeeded: rte_flow >> 0x%"PRIxPTR" " >> + "%s flow create %d %s", netdev_get_name(netdev), >> + (intptr_t) flow, extra_str, >> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s)); >> } >> } else { >> - enum vlog_level level = VLL_WARN; >> - >> - if (error->type == RTE_FLOW_ERROR_TYPE_ACTION) { >> - level = VLL_DBG; >> - } >> - VLOG_RL(&rl, level, "%s: rte_flow creation failed: %d (%s).", >> - netdev_get_name(netdev), error->type, error->message); >> - if (!vlog_should_drop(&this_module, level, &rl)) { >> + if (!VLOG_DROP_DBG(&rl)) { >> dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions); >> extra_str = ds_cstr(&s_extra); >> - VLOG_RL(&rl, level, "%s: Failed flow: %s flow create %d %s", >> - netdev_get_name(netdev), extra_str, >> - netdev_dpdk_get_port_id(netdev), ds_cstr(&s)); >> + VLOG_DBG("%s: rte_flow creation failed [%d (%s)]: " >> + "%s flow create %d %s", netdev_get_name(netdev), >> + error->type, error->message,extra_str, > > Nit: I think an extra space is missing before extra_str. Thanks for the review David! I’ll fix it at commit time. > > >> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s)); >> } >> } >> -- >> 2.46.0 >> > > Reviewed-by: David Marchand <[email protected]> > > > -- > David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
