netdev - Re: [PATCH net-next v3 1/6] net: ethtool: allow symmetric-xor RSS hash for any flow type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cf6c824a-be09-4b6c-b2a2-fb870e9f0c37@intel.com>
Date: Sat, 14 Oct 2023 06:19:54 -0600
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<corbet@....net>, <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<vladimir.oltean@....com>, <andrew@...n.ch>, <horms@...nel.org>,
	<mkubecek@...e.cz>, <linux-doc@...r.kernel.org>, Wojciech Drewek
	<wojciech.drewek@...el.com>, Willem de Bruijn
	<willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net-next v3 1/6] net: ethtool: allow symmetric-xor RSS
 hash for any flow type



On 2023-10-11 16:45, Willem de Bruijn wrote:
> On Tue, Oct 10, 2023 at 5:34 PM Ahmed Zaki <ahmed.zaki@...el.com> wrote:
>>
>>
>> On 2023-10-10 14:40, Willem de Bruijn wrote:
>>
>> On Tue, Oct 10, 2023 at 4:05 PM Ahmed Zaki <ahmed.zaki@...el.com> wrote:
>>
>> Symmetric RSS hash functions are beneficial in applications that monitor
>> both Tx and Rx packets of the same flow (IDS, software firewalls, ..etc).
>> Getting all traffic of the same flow on the same RX queue results in
>> higher CPU cache efficiency.
>>
>> A NIC that supports "symmetric-xor" can achieve this RSS hash symmetry
>> by XORing the source and destination fields and pass the values to the
>> RSS hash algorithm.
>>
>> Only fields that has counterparts in the other direction can be
>> accepted; IP src/dst and L4 src/dst ports.
>>
>> The user may request RSS hash symmetry for a specific flow type, via:
>>
>>      # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n symmetric-xor
>>
>> or turn symmetry off (asymmetric) by:
>>
>>      # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>
>> ---
>>   Documentation/networking/scaling.rst |  6 ++++++
>>   include/uapi/linux/ethtool.h         | 17 +++++++++--------
>>   net/ethtool/ioctl.c                  | 11 +++++++++++
>>   3 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/networking/scaling.rst b/Documentation/networking/scaling.rst
>> index 92c9fb46d6a2..64f3d7566407 100644
>> --- a/Documentation/networking/scaling.rst
>> +++ b/Documentation/networking/scaling.rst
>> @@ -44,6 +44,12 @@ by masking out the low order seven bits of the computed hash for the
>>   packet (usually a Toeplitz hash), taking this number as a key into the
>>   indirection table and reading the corresponding value.
>>
>> +Some NICs support symmetric RSS hashing where, if the IP (source address,
>> +destination address) and TCP/UDP (source port, destination port) tuples
>> +are swapped, the computed hash is the same. This is beneficial in some
>> +applications that monitor TCP/IP flows (IDS, firewalls, ...etc) and need
>> +both directions of the flow to land on the same Rx queue (and CPU).
>> +
>>
>> Maybe add a short ethtool example?
>>
>> Same example as in commit message is OK?
>>
>> AFAIK, the "ethtool" patch has to be sent after this series is accepted. So I am not 100% sure of how the ethtool side will look like, but I can add the line above to Doc.
> 
> Good point. Then let's not if the API is not final yet.
>>
>>
>>   Some advanced NICs allow steering packets to queues based on
>>   programmable filters. For example, webserver bound TCP port 80 packets
>>   can be directed to their own receive queue. Such “n-tuple” filters can
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index f7fba0dc87e5..b9ee667ad7e5 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -2018,14 +2018,15 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>>   #define        FLOW_RSS        0x20000000
>>
>>   /* L3-L4 network traffic flow hash options */
>> -#define        RXH_L2DA        (1 << 1)
>> -#define        RXH_VLAN        (1 << 2)
>> -#define        RXH_L3_PROTO    (1 << 3)
>> -#define        RXH_IP_SRC      (1 << 4)
>> -#define        RXH_IP_DST      (1 << 5)
>> -#define        RXH_L4_B_0_1    (1 << 6) /* src port in case of TCP/UDP/SCTP */
>> -#define        RXH_L4_B_2_3    (1 << 7) /* dst port in case of TCP/UDP/SCTP */
>> -#define        RXH_DISCARD     (1 << 31)
>> +#define        RXH_L2DA                (1 << 1)
>> +#define        RXH_VLAN                (1 << 2)
>> +#define        RXH_L3_PROTO            (1 << 3)
>> +#define        RXH_IP_SRC              (1 << 4)
>> +#define        RXH_IP_DST              (1 << 5)
>> +#define        RXH_L4_B_0_1            (1 << 6) /* src port in case of TCP/UDP/SCTP */
>> +#define        RXH_L4_B_2_3            (1 << 7) /* dst port in case of TCP/UDP/SCTP */
>> +#define        RXH_SYMMETRIC_XOR       (1 << 30)
>> +#define        RXH_DISCARD             (1 << 31)
>>
>> Are these indentation changes intentional?
>>
>>
>> Yes, for alignment ("RXH_SYMMETRIC_XOR" is too long).
> 
> I think it's preferable to not touch other lines. Among others, that
> messes up git blame. But it's subjective. Follow your preference if no
> one else chimes in.

Jakub,

Sorry for late reply, I was off for few days.

I'd like to keep this version, I don't see any other comments that needs 
to be addressed. Can you accept this or need a v4/rebase ?

Ahmed