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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Apr 20, 2020 at 01:52:10PM +0200, Jiri Pirko wrote:
> 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.

I see, I think requesting HW_STATS_DISABLED in tc fails with the
existing code though.

> However so far (before HW_STATS patchset), the offload just did the
> stats and you ignored them in netfilter code, correct?

Yes, netfilter is not collecting stats yet.

> Perhaps we need another value of this, like "HW_STATS_MAY_DISABLED" for
> such case.

Or just redefine FLOW_ACTION_HW_STATS_DISABLED to define a bit in
enum flow_action_hw_stats_bit.

enum flow_action_hw_stats_bit {
        FLOW_ACTION_HW_STATES_DISABLED_BIT,
        FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
        FLOW_ACTION_HW_STATS_DELAYED_BIT,
};

Then update:

        FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_DISABLED |
                                   FLOW_ACTION_HW_STATS_IMMEDIATE |
                                   FLOW_ACTION_HW_STATS_DELAYED,

?

> 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.

My interpretation is that _DISABLED means that front-end does not
request counters to the driver.

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

If this is solved as I'm proposing above, then
__flow_action_hw_stats_check() need to take a bitmask instead of enum
flow_action_hw_stats_bit as parameter, because a driver need to
specify what they support, eg.

        if (!__flow_action_hw_stats_check(action, &extack,
                                         FLOW_ACTION_HW_STATS_DISABLED |
                                         FLOW_ACTION_HW_STATS_DELAYED))
                return -EOPNOSUPP;

or alternatively, if the driver supports any case:

        if (!__flow_action_hw_stats_check(action, &extack,
                                         FLOW_ACTION_HW_STATS_ANY))
                return -EOPNOSUPP;