[ovs-dev,v2] netdev-offload-dpdk: Change flow offload failure log level.

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 <ktraynor@redhat.com>

---
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(-)

On 13 Sep 2024, at 14:25, 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 and tidy-up the succeed
> and failure logs.
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>

Thanks, Kevin, for following up and for the consistent debug messages for both failures and successes.

Acked-by: Eelco Chaudron <echaudro@redhat.com>

On Fri, Sep 13, 2024 at 2:25 PM Kevin Traynor <ktraynor@redhat.com> 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 <ktraynor@redhat.com>
>
> ---
> 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.


> +                     netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>          }
>      }
> --
> 2.46.0
>

Reviewed-by: David Marchand <david.marchand@redhat.com>

On 16 Sep 2024, at 10:01, David Marchand wrote:

> On Fri, Sep 13, 2024 at 2:25 PM Kevin Traynor <ktraynor@redhat.com> 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 <ktraynor@redhat.com>
>>
>> ---
>> 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 <david.marchand@redhat.com>
>
>
> -- 
> David Marchand

On 13 Sep 2024, at 14:25, 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 and tidy-up the succeed
> and failure logs.
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

Kevin, thanks for the patch, and David thanks for the Review.

This patch was applied to main.

Cheers,

Eelco