On 11/09/2024 09:41, Eelco Chaudron wrote: > > > 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” >
Ack, yes, had same comment from David. Will look to merge them. > >> } >> } >> -- >> 2.46.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
