Re: [PATCH net-next 1/5] net: ethtool: only allow set_rxnfc with rss + ring_cookie if driver opts in

[Gal Pressman <gal@nvidia.com>]

On 25/11/2024 16:20, Ahmed Zaki wrote:
> 
> 
> On 2024-11-25 7:10 a.m., Gal Pressman wrote:
>> On 25/11/2024 15:21, Edward Cree wrote:
>>> On 25/11/2024 07:11, Gal Pressman wrote:
>>>> On 13/11/2024 14:13, edward.cree@amd.com wrote:
>>>>> Ethtool ntuple filters with FLOW_RSS were originally defined as adding
>>>>>   the base queue ID (ring_cookie) to the value from the indirection
>>>>> table,
>>>>>   so that the same table could distribute over more than one set of
>>>>> queues
>>>>>   when used by different filters.
>>>>
>>>> TBH, I'm not sure I understand the difference? Perhaps you can share an
>>>> example?
>>>
>>> Something like this:
>>>
>>> ethtool -X $intf context new equal 2
>>> # creates context ID 1, table filled with 0s and 1s
>>> ethtool -N $intf <match fields...> context 1
>>> # filter distributes traffic to queues 0 and 1
>>> ethtool -N $intf <match fields...> context 1 action 2
>>> # filter distributes traffic to queues 2 and 3
>>>
>>> See the selftest in patch 4 for a concrete example of this.
>>> Some NICs were apparently sending the traffic from both filters to
>>>   queues 0 and 1, and ignoring the 'action 2' on the second filter.
>>
>> Thanks, I did not know it works that way, is it actually documented
>> anywhere?
>>
>>>
>>>>> @@ -992,6 +992,11 @@ static noinline_for_stack int
>>>>> ethtool_set_rxnfc(struct net_device *dev,
>>>>>       if (rc)
>>>>>           return rc;
>>>>>   +    /* Nonzero ring with RSS only makes sense if NIC adds them
>>>>> together */
>>>>> +    if (info.flow_type & FLOW_RSS && !ops->cap_rss_rxnfc_adds &&
>>>>> +        ethtool_get_flow_spec_ring(info.fs.ring_cookie))
>>>>> +        return -EINVAL;
>>>>
>>>> I believe this check shouldn't happen when we do ETHTOOL_SRXCLSRLDEL as
>>>> flow_type is garbage, WDYT?
>>>
>>> Agreed; this check should only apply to ETHTOOL_SRXCLSRLINS.  Do you
>>> want
>>>   to send the fix or shall I?
>>
>> I will do it.
>>
>>>
>>> Also, the check below it, dealing with sym-xor, looks like it's only
>>>   relevant to ETHTOOL_SRXFH, since info.data is garbage for other
>>> commands.
>>>   Ahmed, is my understanding correct there?
>>>
>>
>> Speaking of the below check, the sanity check depends on the order of
>> operations, for example:
>> 1. Enable symmetric xor
>> 2. Request hash on src only
>> = Error as expected, however:
> 
> Correct. The check below is to make sure that no ntuple that does not
> cover symmetric fields is added if symm-xor is enabled.
> 
>>
>> 1. Request hash on src only
>> 2. Enable symmetric xor
>> = Success :(.
>>
>> I've been thinking of improving the situation, but that requires
>> iterating over all flow types on symmetric xor enablement and that feels
>> quite bad..
> 
> and delete/disable filters? may be just a warning to the user that some
> filters are not symmetric.

I think the right thing to do in that case is fail the symmetric xor
enablement, but do we really want to query the driver for all flow types
and check if an asymmetric filter exists?