Re: [PATCH net] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DISABLED

[Jiri Pirko <jiri@resnulli.us>]

Mon, Apr 20, 2020 at 12:03:41PM CEST, pablo@netfilter.org wrote:
>On Mon, Apr 20, 2020 at 11:13:02AM +0200, Jiri Pirko wrote:
>> Mon, Apr 20, 2020 at 11:05:05AM CEST, pablo@netfilter.org wrote:
>> >On Mon, Apr 20, 2020 at 10:02:00AM +0200, Jiri Pirko wrote:
>> >> Sun, Apr 19, 2020 at 01:53:38PM CEST, pablo@netfilter.org wrote:
>> >> >If the frontend requests no stats through FLOW_ACTION_HW_STATS_DISABLED,
>> >> >drivers that are checking for the hw stats configuration bail out with
>> >> >EOPNOTSUPP.
>> >>
>> >> Wait, that was a point. Driver has to support stats disabling.
>> >
>> >Hm, some drivers used to accept FLOW_ACTION_HW_STATS_DISABLED, now
>> >rulesets that used to work don't work anymore.
>>
>> How? This check is here since the introduction of hw stats types.
>
>Netfilter is setting the counter support to
>FLOW_ACTION_HW_STATS_DISABLED in this example below:
>
>  table netdev filter {
>        chain ingress {
>                type filter hook ingress device eth0 priority 0; flags offload;
>
>                tcp dport 22 drop
>        }
>  }

Hmm. In TC the HW_STATS_DISABLED has to be explicitly asked by the user,
as the sw stats are always on. Your case is different.
However so far (before HW_STATS patchset), the offload just did the
stats and you ignored them in netfilter code, correct?

Perhaps we need another value of this, like "HW_STATS_MAY_DISABLED" for
such case. Because you don't care if the HW actually does the stats or
not. It is an optimization for you.

However for TC, when user specifies "HW_STATS_DISABLED", the driver
should not do stats.


>
>The user did not specify a counter in this case.
>
>I think __flow_action_hw_stats_check() cannot work with
>FLOW_ACTION_HW_STATS_DISABLED.
>
>If check_allow_bit is false and FLOW_ACTION_HW_STATS_DISABLED is
>specified, then this always evaluates true:
>
>        if (!check_allow_bit &&
>            action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
>
>Similarly:
>
>        } else if (check_allow_bit &&
>                   !(action_entry->hw_stats & BIT(allow_bit))) {
>
>evaluates true for FLOW_ACTION_HW_STATS_DISABLED, assuming allow_bit is
>set, which I think it is the intention.

That is correct. __flow_action_hw_stats_check() helper is here for
simple drivers that support just one type of hw stats
(immediate/delayed).


>
>Another suggestion: This is control plane code and this
>__flow_action_hw_stats_check() function is relatively large, I'd suggest
>to move it to net/core/flow_offload.c at some point.

Sure, why not.


>
>Thank you!