On 10 Sep 2024, at 17:11, Kevin Traynor 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. Thanks for fixing this, and sending the patch. One comment below on the log messages. > Signed-off-by: Kevin Traynor <[email protected]> > --- > 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..378bee161 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 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)) { > + VLOG_DBG("%s: rte_flow creation failed: %d (%s).", > + netdev_get_name(netdev), error->type, error->message); > 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: Failed flow: %s flow create %d %s", > + netdev_get_name(netdev), extra_str, > + netdev_dpdk_get_port_id(netdev), ds_cstr(&s)); Would it make sense to combine the two debug log messages? Something in the lines of: %s: rte_flow creation failed [%d (%s)]: %s flow create %d %s” > } > } > -- > 2.46.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
