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