On Mon, Apr 20, 2020 at 01:52:10PM +0200, Jiri Pirko wrote:
> Mon, Apr 20, 2020 at 12:03:41PM CEST, pablo@xxxxxxxxxxxxx wrote:
> >On Mon, Apr 20, 2020 at 11:13:02AM +0200, Jiri Pirko wrote:
> >> Mon, Apr 20, 2020 at 11:05:05AM CEST, pablo@xxxxxxxxxxxxx wrote:
> >> >On Mon, Apr 20, 2020 at 10:02:00AM +0200, Jiri Pirko wrote:
> >> >> Sun, Apr 19, 2020 at 01:53:38PM CEST, pablo@xxxxxxxxxxxxx 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;
