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

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

[Pablo Neira Ayuso]

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.

Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/flow_offload.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 3619c6acf60f..c2519a25d0bd 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -326,6 +326,9 @@ __flow_action_hw_stats_check(const struct flow_action *action,
 	if (!flow_action_mixed_hw_stats_check(action, extack))
 		return false;
 	action_entry = flow_action_first_entry_get(action);
+	if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DISABLED)
+		return true;
+
 	if (!check_allow_bit &&
 	    action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
-- 
2.11.0

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

[Jiri Pirko]

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.


>
>Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
> include/net/flow_offload.h | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index 3619c6acf60f..c2519a25d0bd 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -326,6 +326,9 @@ __flow_action_hw_stats_check(const struct flow_action *action,
> 	if (!flow_action_mixed_hw_stats_check(action, extack))
> 		return false;
> 	action_entry = flow_action_first_entry_get(action);
>+	if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DISABLED)
>+		return true;
>+
> 	if (!check_allow_bit &&
> 	    action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
> 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
>-- 
>2.11.0
>

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

[Pablo Neira Ayuso]

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.

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

[Jiri Pirko]

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.

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

[Pablo Neira Ayuso]

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

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.

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.

Thank you!

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

[Jiri Pirko]

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!

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

[Pablo Neira Ayuso]

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;

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

[Edward Cree]

On 20/04/2020 12:52, Jiri Pirko wrote:
> However for TC, when user specifies "HW_STATS_DISABLED", the driver
> should not do stats.
What should a driver do if the user specifies DISABLED, but the stats
 are still needed for internal bookkeeping (e.g. to prod an ARP entry
 that's in use for encapsulation offload, so that it doesn't get
 expired out of the cache)?  Enable the stats on the HW anyway but
 not report them to FLOW_CLS_STATS?  Or return an error?

-ed

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 02:28:22PM CEST, ecree@solarflare.com wrote:
>On 20/04/2020 12:52, Jiri Pirko wrote:
>> However for TC, when user specifies "HW_STATS_DISABLED", the driver
>> should not do stats.
>What should a driver do if the user specifies DISABLED, but the stats
> are still needed for internal bookkeeping (e.g. to prod an ARP entry
> that's in use for encapsulation offload, so that it doesn't get
> expired out of the cache)?  Enable the stats on the HW anyway but
> not report them to FLOW_CLS_STATS?  Or return an error?

If internally needed, it means they cannot be disabled. So returning
error would make sense, as what the user requested is not supported.


>
>-ed

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

[Pablo Neira Ayuso]

On Mon, Apr 20, 2020 at 01:28:22PM +0100, Edward Cree wrote:
> On 20/04/2020 12:52, Jiri Pirko wrote:
> > However for TC, when user specifies "HW_STATS_DISABLED", the driver
> > should not do stats.
>
> What should a driver do if the user specifies DISABLED, but the stats
>  are still needed for internal bookkeeping (e.g. to prod an ARP entry
>  that's in use for encapsulation offload, so that it doesn't get
>  expired out of the cache)?  Enable the stats on the HW anyway but
>  not report them to FLOW_CLS_STATS?  Or return an error?

My interpretation is that HW_STATS_DISABLED means that the front-end
does not care / does not need counters. The driver can still allocate
them if needed. So the enum flow_action_hw_stats flags represent what
the front-end requires.

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

[Pablo Neira Ayuso]

On Mon, Apr 20, 2020 at 02:36:11PM +0200, Jiri Pirko wrote:
> Mon, Apr 20, 2020 at 02:28:22PM CEST, ecree@solarflare.com wrote:
> >On 20/04/2020 12:52, Jiri Pirko wrote:
> >> However for TC, when user specifies "HW_STATS_DISABLED", the driver
> >> should not do stats.
> >What should a driver do if the user specifies DISABLED, but the stats
> > are still needed for internal bookkeeping (e.g. to prod an ARP entry
> > that's in use for encapsulation offload, so that it doesn't get
> > expired out of the cache)?  Enable the stats on the HW anyway but
> > not report them to FLOW_CLS_STATS?  Or return an error?
> 
> If internally needed, it means they cannot be disabled. So returning
> error would make sense, as what the user requested is not supported.

Hm.

Then, if the user disables counters but there is internal dependency
on them, the tc rule fails to be loaded for this reason.

After this the user is forced to re-load the rule, specifying enable
counters.

Why does the user need to force in this case to reload? It seems more
natural to me to give the user what it is requesting (disabled
counters / front-end doesn't care) and the driver internally allocates
the resources that it needs (actually turn them on if there is a
dependency like tunneling).

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 02:49:20PM CEST, pablo@netfilter.org wrote:
>On Mon, Apr 20, 2020 at 02:36:11PM +0200, Jiri Pirko wrote:
>> Mon, Apr 20, 2020 at 02:28:22PM CEST, ecree@solarflare.com wrote:
>> >On 20/04/2020 12:52, Jiri Pirko wrote:
>> >> However for TC, when user specifies "HW_STATS_DISABLED", the driver
>> >> should not do stats.
>> >What should a driver do if the user specifies DISABLED, but the stats
>> > are still needed for internal bookkeeping (e.g. to prod an ARP entry
>> > that's in use for encapsulation offload, so that it doesn't get
>> > expired out of the cache)?  Enable the stats on the HW anyway but
>> > not report them to FLOW_CLS_STATS?  Or return an error?
>> 
>> If internally needed, it means they cannot be disabled. So returning
>> error would make sense, as what the user requested is not supported.
>
>Hm.
>
>Then, if the user disables counters but there is internal dependency
>on them, the tc rule fails to be loaded for this reason.

User asked for disable. They should be disabled. If driver does not
support it, should not pretend they are disabled. Does not make sense.


>
>After this the user is forced to re-load the rule, specifying enable
>counters.
>
>Why does the user need to force in this case to reload? It seems more
>natural to me to give the user what it is requesting (disabled
>counters / front-end doesn't care) and the driver internally allocates
>the resources that it needs (actually turn them on if there is a
>dependency like tunneling).

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 02:39:15PM CEST, pablo@netfilter.org wrote:
>On Mon, Apr 20, 2020 at 01:28:22PM +0100, Edward Cree wrote:
>> On 20/04/2020 12:52, Jiri Pirko wrote:
>> > However for TC, when user specifies "HW_STATS_DISABLED", the driver
>> > should not do stats.
>>
>> What should a driver do if the user specifies DISABLED, but the stats
>>  are still needed for internal bookkeeping (e.g. to prod an ARP entry
>>  that's in use for encapsulation offload, so that it doesn't get
>>  expired out of the cache)?  Enable the stats on the HW anyway but
>>  not report them to FLOW_CLS_STATS?  Or return an error?
>
>My interpretation is that HW_STATS_DISABLED means that the front-end
>does not care / does not need counters. The driver can still allocate

That is wrong interpretation. If user does not care, he specifies "ANY".
That is the default.

When he says "DISABLED" he means disabled. Not "I don't care".


>them if needed. So the enum flow_action_hw_stats flags represent what
>the front-end requires.

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 02:13:51PM CEST, pablo@netfilter.org wrote:
>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,

No! That would break the default. "ANY" can never mean "disabled".


>
>?
>
>> 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;

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

[Florian Westphal]

Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Apr 20, 2020 at 02:39:15PM CEST, pablo@netfilter.org wrote:
> >On Mon, Apr 20, 2020 at 01:28:22PM +0100, Edward Cree wrote:
> >> On 20/04/2020 12:52, Jiri Pirko wrote:
> >> > However for TC, when user specifies "HW_STATS_DISABLED", the driver
> >> > should not do stats.
> >>
> >> What should a driver do if the user specifies DISABLED, but the stats
> >>  are still needed for internal bookkeeping (e.g. to prod an ARP entry
> >>  that's in use for encapsulation offload, so that it doesn't get
> >>  expired out of the cache)?  Enable the stats on the HW anyway but
> >>  not report them to FLOW_CLS_STATS?  Or return an error?
> >
> >My interpretation is that HW_STATS_DISABLED means that the front-end
> >does not care / does not need counters. The driver can still allocate
> 
> That is wrong interpretation. If user does not care, he specifies "ANY".
> That is the default.
> 
> When he says "DISABLED" he means disabled. Not "I don't care".

Under what circumstances would the user care about this?
Rejecting such config seems to be just to annoy user?

I mean, the user is forced to use SW datapath just because HW can't turn
off stats?!  Same for a config change, why do i need to change my rules
to say 'enable stats' even though I don't need them in first place?

Unlike the inverse (want feature X but HW can't support it), it makes
no sense to me to reject with an error here:
stats-off is just a hint that can be safely ignored.

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 03:57:54PM CEST, fw@strlen.de wrote:
>Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Apr 20, 2020 at 02:39:15PM CEST, pablo@netfilter.org wrote:
>> >On Mon, Apr 20, 2020 at 01:28:22PM +0100, Edward Cree wrote:
>> >> On 20/04/2020 12:52, Jiri Pirko wrote:
>> >> > However for TC, when user specifies "HW_STATS_DISABLED", the driver
>> >> > should not do stats.
>> >>
>> >> What should a driver do if the user specifies DISABLED, but the stats
>> >>  are still needed for internal bookkeeping (e.g. to prod an ARP entry
>> >>  that's in use for encapsulation offload, so that it doesn't get
>> >>  expired out of the cache)?  Enable the stats on the HW anyway but
>> >>  not report them to FLOW_CLS_STATS?  Or return an error?
>> >
>> >My interpretation is that HW_STATS_DISABLED means that the front-end
>> >does not care / does not need counters. The driver can still allocate
>> 
>> That is wrong interpretation. If user does not care, he specifies "ANY".
>> That is the default.
>> 
>> When he says "DISABLED" he means disabled. Not "I don't care".
>
>Under what circumstances would the user care about this?

On some HW, the stats are separate resource. The user instructs the
stats to be disabled so safe resources. It is explicit. He like to safe
the resources for other usage (he can list them in devlink resource).


>Rejecting such config seems to be just to annoy user?

Well, the user passed the arg, he should know what is he doing. There's
no annoyment.


>
>I mean, the user is forced to use SW datapath just because HW can't turn
>off stats?!  Same for a config change, why do i need to change my rules

By default, they are on. That is what user should do in most of the
cases.


>to say 'enable stats' even though I don't need them in first place?

It may require to program HW differently (as in case of mlxsw).


>
>Unlike the inverse (want feature X but HW can't support it), it makes
>no sense to me to reject with an error here:
>stats-off is just a hint that can be safely ignored.

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

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Mon, Apr 20, 2020 at 04:14:22PM +0200, Jiri Pirko wrote:
> Mon, Apr 20, 2020 at 03:57:54PM CEST, fw@strlen.de wrote:
[...]
> >I mean, the user is forced to use SW datapath just because HW can't turn
> >off stats?!  Same for a config change, why do i need to change my rules
> 
> By default, they are on. That is what user should do in most of the
> cases.

Fair enough, I can workaround this problem by using
FLOW_ACTION_HW_STATS_ANY. However, I still don't need counters and
there is no way to say "I don't care" to the drivers.

Note that the flow_offload infrastructure is used by ethtool,
netfilter, flowtable and tc these days.

* ethtool's default behaviour is no counters.
* netfilter's default behaviour is no counters.
* flowtable's default behaviour is no counters.


I understand FLOW_ACTION_HW_STATS_DISABLED means disabled, strictly.
But would you allow me to introduce FLOW_ACTION_HW_STATS_DONT_CARE to
fix ethtool, netfilter and flowtable? :-)

FLOW_ACTION_HW_STATS_DONT_CARE means "this front-end doesn't need
counters, let driver decide what it is best".

Thank you.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 909 bytes --]

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 3619c6acf60f..ae09d1911912 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -164,17 +164,21 @@ enum flow_action_mangle_base {
 };
 
 enum flow_action_hw_stats_bit {
+	FLOW_ACTION_HW_STATS_DONT_CARE_BIT,
 	FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
 	FLOW_ACTION_HW_STATS_DELAYED_BIT,
 };
 
 enum flow_action_hw_stats {
 	FLOW_ACTION_HW_STATS_DISABLED = 0,
+	FLOW_ACTION_HW_STATS_DONT_CARE =
+		BIT(FLOW_ACTION_HW_STATS_DONT_CARE_BIT),
 	FLOW_ACTION_HW_STATS_IMMEDIATE =
 		BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT),
 	FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT),
 	FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE |
-				   FLOW_ACTION_HW_STATS_DELAYED,
+				   FLOW_ACTION_HW_STATS_DELAYED |
+				   FLOW_ACTION_HW_STATS_DONT_CARE,
 };
 
 typedef void (*action_destr)(void *priv);

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

[Jiri Pirko]

Mon, Apr 20, 2020 at 09:18:32PM CEST, pablo@netfilter.org wrote:
>On Mon, Apr 20, 2020 at 04:14:22PM +0200, Jiri Pirko wrote:
>> Mon, Apr 20, 2020 at 03:57:54PM CEST, fw@strlen.de wrote:
>[...]
>> >I mean, the user is forced to use SW datapath just because HW can't turn
>> >off stats?!  Same for a config change, why do i need to change my rules
>> 
>> By default, they are on. That is what user should do in most of the
>> cases.
>
>Fair enough, I can workaround this problem by using
>FLOW_ACTION_HW_STATS_ANY. However, I still don't need counters and
>there is no way to say "I don't care" to the drivers.
>
>Note that the flow_offload infrastructure is used by ethtool,
>netfilter, flowtable and tc these days.
>
>* ethtool's default behaviour is no counters.
>* netfilter's default behaviour is no counters.
>* flowtable's default behaviour is no counters.
>
>
>I understand FLOW_ACTION_HW_STATS_DISABLED means disabled, strictly.
>But would you allow me to introduce FLOW_ACTION_HW_STATS_DONT_CARE to
>fix ethtool, netfilter and flowtable? :-)
>
>FLOW_ACTION_HW_STATS_DONT_CARE means "this front-end doesn't need
>counters, let driver decide what it is best".
>
>Thank you.

>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index 3619c6acf60f..ae09d1911912 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -164,17 +164,21 @@ enum flow_action_mangle_base {
> };
> 
> enum flow_action_hw_stats_bit {
>+	FLOW_ACTION_HW_STATS_DONT_CARE_BIT,
> 	FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
> 	FLOW_ACTION_HW_STATS_DELAYED_BIT,
> };
> 
> enum flow_action_hw_stats {
> 	FLOW_ACTION_HW_STATS_DISABLED = 0,
>+	FLOW_ACTION_HW_STATS_DONT_CARE =
>+		BIT(FLOW_ACTION_HW_STATS_DONT_CARE_BIT),
> 	FLOW_ACTION_HW_STATS_IMMEDIATE =
> 		BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT),
> 	FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT),
> 	FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE |
>-				   FLOW_ACTION_HW_STATS_DELAYED,
>+				   FLOW_ACTION_HW_STATS_DELAYED |
>+				   FLOW_ACTION_HW_STATS_DONT_CARE,

"Any" can't be "don't care". TC User expects stats. That's default.


Let's have "don't care" bit only and set it for
ethtool/netfilter/flowtable. Don't change any. Teach the drivers to deal
with "don't care", most probably using the default checker.


> };
> 
> typedef void (*action_destr)(void *priv);

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

[Edward Cree]

On 22/04/2020 19:37, Jiri Pirko wrote:
> "Any" can't be "don't care". TC User expects stats. That's default.
>
> Let's have "don't care" bit only and set it for
> ethtool/netfilter/flowtable. Don't change any. Teach the drivers to deal
> with "don't care", most probably using the default checker.
I think the right solution is either this, or the semantically-similar
 approach of "0 means don't care, we have a bit for disabled, and ANY
 (the TC default) is "all the bits except disabled", i.e.
 DELAYED | IMMEDIATE.  That seems slightly cleaner to me, as then non-
 zero settings are always "here is a bitmask of options, driver may
 choose any of them".  (And 0 differs from DELAYED | IMMEDIATE | DISABLED
 only in that if new bits are added to kernel, 0 includes them.)
And of course either way the TC uAPI needs to be able to specify the
 new "don't care" option.

-ed

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

[Jakub Kicinski]

On Mon, 27 Apr 2020 15:31:48 +0100 Edward Cree wrote:
> On 22/04/2020 19:37, Jiri Pirko wrote:
> > "Any" can't be "don't care". TC User expects stats. That's default.
> >
> > Let's have "don't care" bit only and set it for
> > ethtool/netfilter/flowtable. Don't change any. Teach the drivers to deal
> > with "don't care", most probably using the default checker.  
> I think the right solution is either this, or the semantically-similar
>  approach of "0 means don't care, we have a bit for disabled, and ANY
>  (the TC default) is "all the bits except disabled", i.e.
>  DELAYED | IMMEDIATE.  That seems slightly cleaner to me, as then non-
>  zero settings are always "here is a bitmask of options, driver may
>  choose any of them".  (And 0 differs from DELAYED | IMMEDIATE | DISABLED
>  only in that if new bits are added to kernel, 0 includes them.)

+1

> And of course either way the TC uAPI needs to be able to specify the
>  new "don't care" option.