diff mbox series

[net-next,v4,1/6] net: ethtool: allow symmetric-xor RSS hash for any flow type

Message ID 20231016154937.41224-2-ahmed.zaki@intel.com
State Handled Elsewhere
Headers show
Series Support symmetric RSS (Toeplitz) hash | expand

Commit Message

Ahmed Zaki Oct. 16, 2023, 3:49 p.m. UTC 
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@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 Documentation/networking/scaling.rst |  6 ++++++
 include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
 net/ethtool/ioctl.c                  | 11 +++++++++++
 3 files changed, 30 insertions(+), 8 deletions(-)

Comments

Alexander Duyck Oct. 16, 2023, 8:17 p.m. UTC | #1 
On Mon, 2023-10-16 at 09:49 -0600, Ahmed Zaki 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@intel.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> ---
>  Documentation/networking/scaling.rst |  6 ++++++
>  include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
>  net/ethtool/ioctl.c                  | 11 +++++++++++
>  3 files changed, 30 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).
> +
>  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..4e8d38fb55ce 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -2018,14 +2018,19 @@ 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 */
> +/* XOR the corresponding source and destination fields of each specified
> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
> + * calculation.
> + */
> +#define	RXH_SYMMETRIC_XOR	(1 << 30)
> +#define	RXH_DISCARD		(1 << 31)

I guess this has already been discussed but I am not a fan of long
names for defines. I would prefer to see this just be something like
RXH_SYMMETRIC or something like that. The XOR is just an implementation
detail. I have seen the same thing accomplished by just reordering the
fields by min/max approaches.

>  
>  #define	RX_CLS_FLOW_DISC	0xffffffffffffffffULL
>  #define RX_CLS_FLOW_WAKE	0xfffffffffffffffeULL
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 0b0ce4f81c01..b1bd0d4b48e8 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -980,6 +980,17 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>  	if (rc)
>  		return rc;
>  
> +	/* If a symmetric hash is requested, then:
> +	 * 1 - no other fields besides IP src/dst and/or L4 src/dst
> +	 * 2 - If src is set, dst must also be set
> +	 */
> +	if ((info.data & RXH_SYMMETRIC_XOR) &&
> +	    ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
> +	      RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
> +	     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
> +	     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
> +		return -EINVAL;
> +
>  	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
>  	if (rc)
>  		return rc;

You are pushing implementation from your device into the interface
design here. You should probably push these requirements down into the
driver rather than making it a part of the generic implementation.

It would be nice to see input from other NIC vendors on this as I
suspect they probably have similar functionality available to them.
Ahmed Zaki Oct. 16, 2023, 9:08 p.m. UTC | #2 
On 2023-10-16 14:17, Alexander H Duyck wrote:
> On Mon, 2023-10-16 at 09:49 -0600, Ahmed Zaki 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@intel.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> ---
>>   Documentation/networking/scaling.rst |  6 ++++++
>>   include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
>>   net/ethtool/ioctl.c                  | 11 +++++++++++
>>   3 files changed, 30 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).
>> +
>>   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..4e8d38fb55ce 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -2018,14 +2018,19 @@ 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 */
>> +/* XOR the corresponding source and destination fields of each specified
>> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
>> + * calculation.
>> + */
>> +#define	RXH_SYMMETRIC_XOR	(1 << 30)
>> +#define	RXH_DISCARD		(1 << 31)
> 
> I guess this has already been discussed but I am not a fan of long
> names for defines. I would prefer to see this just be something like
> RXH_SYMMETRIC or something like that. The XOR is just an implementation
> detail. I have seen the same thing accomplished by just reordering the
> fields by min/max approaches.

Correct. We discussed this and the consensus was that the user needs to 
have complete control on which implementation/algorithm is used to 
provide this symmetry, because each will yield different hash and may be 
different performance.


> 
>>   
>>   #define	RX_CLS_FLOW_DISC	0xffffffffffffffffULL
>>   #define RX_CLS_FLOW_WAKE	0xfffffffffffffffeULL
>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index 0b0ce4f81c01..b1bd0d4b48e8 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
>> @@ -980,6 +980,17 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>>   	if (rc)
>>   		return rc;
>>   
>> +	/* If a symmetric hash is requested, then:
>> +	 * 1 - no other fields besides IP src/dst and/or L4 src/dst
>> +	 * 2 - If src is set, dst must also be set
>> +	 */
>> +	if ((info.data & RXH_SYMMETRIC_XOR) &&
>> +	    ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
>> +	      RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
>> +	     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
>> +	     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
>> +		return -EINVAL;
>> +
>>   	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
>>   	if (rc)
>>   		return rc;
> 
> You are pushing implementation from your device into the interface
> design here. You should probably push these requirements down into the
> driver rather than making it a part of the generic implementation.

This is the most basic check and should be applied in any symmetric RSS 
implementation. Nothing specific to the XOR method. It can also be 
extended to include other "RXH_SYMMETRIC_XXX" in the future.
Alexander Duyck Oct. 16, 2023, 10:15 p.m. UTC | #3 
On Mon, Oct 16, 2023 at 2:09 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>
>
>
> On 2023-10-16 14:17, Alexander H Duyck wrote:
> > On Mon, 2023-10-16 at 09:49 -0600, Ahmed Zaki 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@intel.com>
> >> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> >> ---
> >>   Documentation/networking/scaling.rst |  6 ++++++
> >>   include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
> >>   net/ethtool/ioctl.c                  | 11 +++++++++++
> >>   3 files changed, 30 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).
> >> +
> >>   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..4e8d38fb55ce 100644
> >> --- a/include/uapi/linux/ethtool.h
> >> +++ b/include/uapi/linux/ethtool.h
> >> @@ -2018,14 +2018,19 @@ 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 */
> >> +/* XOR the corresponding source and destination fields of each specified
> >> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
> >> + * calculation.
> >> + */
> >> +#define     RXH_SYMMETRIC_XOR       (1 << 30)
> >> +#define     RXH_DISCARD             (1 << 31)
> >
> > I guess this has already been discussed but I am not a fan of long
> > names for defines. I would prefer to see this just be something like
> > RXH_SYMMETRIC or something like that. The XOR is just an implementation
> > detail. I have seen the same thing accomplished by just reordering the
> > fields by min/max approaches.
>
> Correct. We discussed this and the consensus was that the user needs to
> have complete control on which implementation/algorithm is used to
> provide this symmetry, because each will yield different hash and may be
> different performance.

I agree about the user having control over the algorithm, but this
interface isn't about selecting the algorithm. It is just about
setting up the inputs. Selecting the algorithm is handled via the
set/get_rxfh interface hfunc variable. If this is just a different
hash function it really belongs there rather than being made a part of
the input string.

> >
> >>
> >>   #define    RX_CLS_FLOW_DISC        0xffffffffffffffffULL
> >>   #define RX_CLS_FLOW_WAKE   0xfffffffffffffffeULL
> >> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> >> index 0b0ce4f81c01..b1bd0d4b48e8 100644
> >> --- a/net/ethtool/ioctl.c
> >> +++ b/net/ethtool/ioctl.c
> >> @@ -980,6 +980,17 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
> >>      if (rc)
> >>              return rc;
> >>
> >> +    /* If a symmetric hash is requested, then:
> >> +     * 1 - no other fields besides IP src/dst and/or L4 src/dst
> >> +     * 2 - If src is set, dst must also be set
> >> +     */
> >> +    if ((info.data & RXH_SYMMETRIC_XOR) &&
> >> +        ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
> >> +          RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
> >> +         (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
> >> +         (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
> >> +            return -EINVAL;
> >> +
> >>      rc = dev->ethtool_ops->set_rxnfc(dev, &info);
> >>      if (rc)
> >>              return rc;
> >
> > You are pushing implementation from your device into the interface
> > design here. You should probably push these requirements down into the
> > driver rather than making it a part of the generic implementation.
>
> This is the most basic check and should be applied in any symmetric RSS
> implementation. Nothing specific to the XOR method. It can also be
> extended to include other "RXH_SYMMETRIC_XXX" in the future.

You are partially correct. Your item 2 is accurate, however you are
excluding other fields in your item 1. Fields such as L2DA wouldn't be
symmetric, but VLAN and L3_PROTO would be. That is the implementation
specific detail I was referring to.
Ahmed Zaki Oct. 16, 2023, 10:44 p.m. UTC | #4 
On 2023-10-16 16:15, Alexander Duyck wrote:
> On Mon, Oct 16, 2023 at 2:09 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>>
>>
>>
>> On 2023-10-16 14:17, Alexander H Duyck wrote:
>>> On Mon, 2023-10-16 at 09:49 -0600, Ahmed Zaki 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@intel.com>
>>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>>>> ---
>>>>    Documentation/networking/scaling.rst |  6 ++++++
>>>>    include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
>>>>    net/ethtool/ioctl.c                  | 11 +++++++++++
>>>>    3 files changed, 30 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).
>>>> +
>>>>    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..4e8d38fb55ce 100644
>>>> --- a/include/uapi/linux/ethtool.h
>>>> +++ b/include/uapi/linux/ethtool.h
>>>> @@ -2018,14 +2018,19 @@ 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 */
>>>> +/* XOR the corresponding source and destination fields of each specified
>>>> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
>>>> + * calculation.
>>>> + */
>>>> +#define     RXH_SYMMETRIC_XOR       (1 << 30)
>>>> +#define     RXH_DISCARD             (1 << 31)
>>>
>>> I guess this has already been discussed but I am not a fan of long
>>> names for defines. I would prefer to see this just be something like
>>> RXH_SYMMETRIC or something like that. The XOR is just an implementation
>>> detail. I have seen the same thing accomplished by just reordering the
>>> fields by min/max approaches.
>>
>> Correct. We discussed this and the consensus was that the user needs to
>> have complete control on which implementation/algorithm is used to
>> provide this symmetry, because each will yield different hash and may be
>> different performance.
> 
> I agree about the user having control over the algorithm, but this
> interface isn't about selecting the algorithm. It is just about
> setting up the inputs. Selecting the algorithm is handled via the
> set/get_rxfh interface hfunc variable. If this is just a different
> hash function it really belongs there rather than being made a part of
> the input string.

My bad. It is the same RSS algorithm (Toeplitz in our case). Still the 
user needs to be able to manipulate the inputs. The point is, a generic 
define like "RXH_SYMETRIC" was rejected (that was actually v1).


> 
>>>
>>>>
>>>>    #define    RX_CLS_FLOW_DISC        0xffffffffffffffffULL
>>>>    #define RX_CLS_FLOW_WAKE   0xfffffffffffffffeULL
>>>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>>>> index 0b0ce4f81c01..b1bd0d4b48e8 100644
>>>> --- a/net/ethtool/ioctl.c
>>>> +++ b/net/ethtool/ioctl.c
>>>> @@ -980,6 +980,17 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>>>>       if (rc)
>>>>               return rc;
>>>>
>>>> +    /* If a symmetric hash is requested, then:
>>>> +     * 1 - no other fields besides IP src/dst and/or L4 src/dst
>>>> +     * 2 - If src is set, dst must also be set
>>>> +     */
>>>> +    if ((info.data & RXH_SYMMETRIC_XOR) &&
>>>> +        ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
>>>> +          RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
>>>> +         (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
>>>> +         (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
>>>> +            return -EINVAL;
>>>> +
>>>>       rc = dev->ethtool_ops->set_rxnfc(dev, &info);
>>>>       if (rc)
>>>>               return rc;
>>>
>>> You are pushing implementation from your device into the interface
>>> design here. You should probably push these requirements down into the
>>> driver rather than making it a part of the generic implementation.
>>
>> This is the most basic check and should be applied in any symmetric RSS
>> implementation. Nothing specific to the XOR method. It can also be
>> extended to include other "RXH_SYMMETRIC_XXX" in the future.
> 
> You are partially correct. Your item 2 is accurate, however you are
> excluding other fields in your item 1. Fields such as L2DA wouldn't be
> symmetric, but VLAN and L3_PROTO would be. That is the implementation
> specific detail I was referring to.

hmm.. not sure how VLAN tag would be used in this case. But moving this 
into ice_ethtool is trivial. We can start there and unify when/if other 
vendors push similar functionalities.

How does that sound?
Alexander Duyck Oct. 16, 2023, 10:55 p.m. UTC | #5 
On Mon, Oct 16, 2023 at 3:44 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>
>
>
> On 2023-10-16 16:15, Alexander Duyck wrote:
> > On Mon, Oct 16, 2023 at 2:09 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
> >>
> >>
> >>
> >> On 2023-10-16 14:17, Alexander H Duyck wrote:
> >>> On Mon, 2023-10-16 at 09:49 -0600, Ahmed Zaki 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@intel.com>
> >>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> >>>> ---
> >>>>    Documentation/networking/scaling.rst |  6 ++++++
> >>>>    include/uapi/linux/ethtool.h         | 21 +++++++++++++--------
> >>>>    net/ethtool/ioctl.c                  | 11 +++++++++++
> >>>>    3 files changed, 30 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).
> >>>> +
> >>>>    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..4e8d38fb55ce 100644
> >>>> --- a/include/uapi/linux/ethtool.h
> >>>> +++ b/include/uapi/linux/ethtool.h
> >>>> @@ -2018,14 +2018,19 @@ 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 */
> >>>> +/* XOR the corresponding source and destination fields of each specified
> >>>> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
> >>>> + * calculation.
> >>>> + */
> >>>> +#define     RXH_SYMMETRIC_XOR       (1 << 30)
> >>>> +#define     RXH_DISCARD             (1 << 31)
> >>>
> >>> I guess this has already been discussed but I am not a fan of long
> >>> names for defines. I would prefer to see this just be something like
> >>> RXH_SYMMETRIC or something like that. The XOR is just an implementation
> >>> detail. I have seen the same thing accomplished by just reordering the
> >>> fields by min/max approaches.
> >>
> >> Correct. We discussed this and the consensus was that the user needs to
> >> have complete control on which implementation/algorithm is used to
> >> provide this symmetry, because each will yield different hash and may be
> >> different performance.
> >
> > I agree about the user having control over the algorithm, but this
> > interface isn't about selecting the algorithm. It is just about
> > setting up the inputs. Selecting the algorithm is handled via the
> > set/get_rxfh interface hfunc variable. If this is just a different
> > hash function it really belongs there rather than being made a part of
> > the input string.
>
> My bad. It is the same RSS algorithm (Toeplitz in our case). Still the
> user needs to be able to manipulate the inputs. The point is, a generic
> define like "RXH_SYMETRIC" was rejected (that was actually v1).

No it is not. That is the point and you made it quite clear. The hash
you are using is a variant of Toeplitz, but it is also hybridized with
XOR. Why would it make sense to add it as an input mask bit when what
you are doing is using a different algorithm.

It would make more sense to just add it as a variant hash function of
toeplitz. If you did it right you could probably make the formatting
pretty, something like:
RSS hash function:
    toeplitz: on
        symmetric xor: on
    xor: off
    crc32: off

It doesn't make sense to place it in the input flags and will just
cause quick congestion as things get added there. This is an algorithm
change so it makes more sense to place it there.

> >
> >>>
> >>>>
> >>>>    #define    RX_CLS_FLOW_DISC        0xffffffffffffffffULL
> >>>>    #define RX_CLS_FLOW_WAKE   0xfffffffffffffffeULL
> >>>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> >>>> index 0b0ce4f81c01..b1bd0d4b48e8 100644
> >>>> --- a/net/ethtool/ioctl.c
> >>>> +++ b/net/ethtool/ioctl.c
> >>>> @@ -980,6 +980,17 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
> >>>>       if (rc)
> >>>>               return rc;
> >>>>
> >>>> +    /* If a symmetric hash is requested, then:
> >>>> +     * 1 - no other fields besides IP src/dst and/or L4 src/dst
> >>>> +     * 2 - If src is set, dst must also be set
> >>>> +     */
> >>>> +    if ((info.data & RXH_SYMMETRIC_XOR) &&
> >>>> +        ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
> >>>> +          RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
> >>>> +         (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
> >>>> +         (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
> >>>> +            return -EINVAL;
> >>>> +
> >>>>       rc = dev->ethtool_ops->set_rxnfc(dev, &info);
> >>>>       if (rc)
> >>>>               return rc;
> >>>
> >>> You are pushing implementation from your device into the interface
> >>> design here. You should probably push these requirements down into the
> >>> driver rather than making it a part of the generic implementation.
> >>
> >> This is the most basic check and should be applied in any symmetric RSS
> >> implementation. Nothing specific to the XOR method. It can also be
> >> extended to include other "RXH_SYMMETRIC_XXX" in the future.
> >
> > You are partially correct. Your item 2 is accurate, however you are
> > excluding other fields in your item 1. Fields such as L2DA wouldn't be
> > symmetric, but VLAN and L3_PROTO would be. That is the implementation
> > specific detail I was referring to.
>
> hmm.. not sure how VLAN tag would be used in this case. But moving this
> into ice_ethtool is trivial. We can start there and unify when/if other
> vendors push similar functionalities.
>
> How does that sound?

I still think this might be something best handled in your set_rxnfc
function rather than generically here. I would be okay with adding a
helper though since this seems like something that could probably be
handled via an inline. As it stands with the suggestion to move this
out as a separate hash type it would make more sense to not have this
as a part of the ethtool_set_rxnfc itself.
Jakub Kicinski Oct. 16, 2023, 11:30 p.m. UTC | #6 
On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
> It would make more sense to just add it as a variant hash function of
> toeplitz. If you did it right you could probably make the formatting
> pretty, something like:
> RSS hash function:
>     toeplitz: on
>         symmetric xor: on
>     xor: off
>     crc32: off
> 
> It doesn't make sense to place it in the input flags and will just
> cause quick congestion as things get added there. This is an algorithm
> change so it makes more sense to place it there.

Algo is also a bit confusing, it's more like key pre-processing?
There's nothing toeplitz about xoring input fields. Works as well
for CRC32.. or XOR.

We can use one of the reserved fields of struct ethtool_rxfh to carry
this extension. I think I asked for this at some point, but there's
only so much repeated feedback one can send in a day :(
Ahmed Zaki Oct. 17, 2023, 12:08 a.m. UTC | #7 
On 2023-10-16 17:30, Jakub Kicinski wrote:
> On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
>> It would make more sense to just add it as a variant hash function of
>> toeplitz. If you did it right you could probably make the formatting
>> pretty, something like:
>> RSS hash function:
>>      toeplitz: on
>>          symmetric xor: on
>>      xor: off
>>      crc32: off
>>
>> It doesn't make sense to place it in the input flags and will just
>> cause quick congestion as things get added there. This is an algorithm
>> change so it makes more sense to place it there.
> 
> Algo is also a bit confusing, it's more like key pre-processing?
> There's nothing toeplitz about xoring input fields. Works as well
> for CRC32.. or XOR.
> 
> We can use one of the reserved fields of struct ethtool_rxfh to carry
> this extension. I think I asked for this at some point, but there's
> only so much repeated feedback one can send in a day :(

Sorry you felt that. I took you comment [1]:

"Using hashing algo for configuring fields feels like a dirty hack".

To mean that the we should not use the hfunc API ("ethtool_rxfh"). This 
is why in the new series I chose to configure the RSS fields. This also 
provides the user with more control and better granularity on which 
flow-types to be symmetric, and which protocols (L3 and/or L4) to use. I 
have no idea how to do any of these via hfunc/ethtool_rxfh API so it 
seemed a better approach.

I see you marked the series as "Changes Requested". I will send a new 
version tomorrow and move the sanity checks inside ice_ethtool.


[1]: https://lore.kernel.org/netdev/20230824174336.6fb801d5@kernel.org/
Alexander Duyck Oct. 17, 2023, 6:37 p.m. UTC | #8 
On Mon, Oct 16, 2023 at 4:31 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
> > It would make more sense to just add it as a variant hash function of
> > toeplitz. If you did it right you could probably make the formatting
> > pretty, something like:
> > RSS hash function:
> >     toeplitz: on
> >         symmetric xor: on
> >     xor: off
> >     crc32: off
> >
> > It doesn't make sense to place it in the input flags and will just
> > cause quick congestion as things get added there. This is an algorithm
> > change so it makes more sense to place it there.
>
> Algo is also a bit confusing, it's more like key pre-processing?
> There's nothing toeplitz about xoring input fields. Works as well
> for CRC32.. or XOR.

I agree that the change to the algorithm doesn't necessarily have
anything to do with toeplitz, however it is still a change to the
algorithm by performing the extra XOR on the inputs prior to
processing. That is why I figured it might make sense to just add a
new hfunc value that would mean toeplitz w/ symmetric XOR.

> We can use one of the reserved fields of struct ethtool_rxfh to carry
> this extension. I think I asked for this at some point, but there's
> only so much repeated feedback one can send in a day :(

Why add an extra reserved field when this is just a variant on a hash
function? I view it as not being dissimilar to how we handle TSO or
tx-checksumming. It would make sense to me to just set something like
toeplitz-symmetric-xor to on in order to turn this on.
Alexander Duyck Oct. 17, 2023, 6:42 p.m. UTC | #9 
On Mon, Oct 16, 2023 at 5:08 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>
>
>
> On 2023-10-16 17:30, Jakub Kicinski wrote:
> > On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
> >> It would make more sense to just add it as a variant hash function of
> >> toeplitz. If you did it right you could probably make the formatting
> >> pretty, something like:
> >> RSS hash function:
> >>      toeplitz: on
> >>          symmetric xor: on
> >>      xor: off
> >>      crc32: off
> >>
> >> It doesn't make sense to place it in the input flags and will just
> >> cause quick congestion as things get added there. This is an algorithm
> >> change so it makes more sense to place it there.
> >
> > Algo is also a bit confusing, it's more like key pre-processing?
> > There's nothing toeplitz about xoring input fields. Works as well
> > for CRC32.. or XOR.
> >
> > We can use one of the reserved fields of struct ethtool_rxfh to carry
> > this extension. I think I asked for this at some point, but there's
> > only so much repeated feedback one can send in a day :(
>
> Sorry you felt that. I took you comment [1]:
>
> "Using hashing algo for configuring fields feels like a dirty hack".
>
> To mean that the we should not use the hfunc API ("ethtool_rxfh"). This
> is why in the new series I chose to configure the RSS fields. This also
> provides the user with more control and better granularity on which
> flow-types to be symmetric, and which protocols (L3 and/or L4) to use. I
> have no idea how to do any of these via hfunc/ethtool_rxfh API so it
> seemed a better approach.
>
> I see you marked the series as "Changes Requested". I will send a new
> version tomorrow and move the sanity checks inside ice_ethtool.
>
>
> [1]: https://lore.kernel.org/netdev/20230824174336.6fb801d5@kernel.org/

So one question I would have is what happens if you were to ignore the
extra configuration that prevents people from disabling either source
or destination from the input? Does it actually have to be hard
restricted or do you end up with the hardware generating non-symmetric
hashes because it isn't doing the XOR with both source and destination
fields?

My thought would be to possibly just look at reducing your messaging
to a warning from the driver if the inputs are not symmetric, but you
have your symmetric xor hash function enabled.
Ahmed Zaki Oct. 17, 2023, 7:14 p.m. UTC | #10 
On 2023-10-17 12:42, Alexander Duyck wrote:
> On Mon, Oct 16, 2023 at 5:08 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>>
>>
>>
>> On 2023-10-16 17:30, Jakub Kicinski wrote:
>>> On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
>>>> It would make more sense to just add it as a variant hash function of
>>>> toeplitz. If you did it right you could probably make the formatting
>>>> pretty, something like:
>>>> RSS hash function:
>>>>       toeplitz: on
>>>>           symmetric xor: on
>>>>       xor: off
>>>>       crc32: off
>>>>
>>>> It doesn't make sense to place it in the input flags and will just
>>>> cause quick congestion as things get added there. This is an algorithm
>>>> change so it makes more sense to place it there.
>>>
>>> Algo is also a bit confusing, it's more like key pre-processing?
>>> There's nothing toeplitz about xoring input fields. Works as well
>>> for CRC32.. or XOR.
>>>
>>> We can use one of the reserved fields of struct ethtool_rxfh to carry
>>> this extension. I think I asked for this at some point, but there's
>>> only so much repeated feedback one can send in a day :(
>>
>> Sorry you felt that. I took you comment [1]:
>>
>> "Using hashing algo for configuring fields feels like a dirty hack".
>>
>> To mean that the we should not use the hfunc API ("ethtool_rxfh"). This
>> is why in the new series I chose to configure the RSS fields. This also
>> provides the user with more control and better granularity on which
>> flow-types to be symmetric, and which protocols (L3 and/or L4) to use. I
>> have no idea how to do any of these via hfunc/ethtool_rxfh API so it
>> seemed a better approach.
>>
>> I see you marked the series as "Changes Requested". I will send a new
>> version tomorrow and move the sanity checks inside ice_ethtool.
>>
>>
>> [1]: https://lore.kernel.org/netdev/20230824174336.6fb801d5@kernel.org/
> 
> So one question I would have is what happens if you were to ignore the
> extra configuration that prevents people from disabling either source
> or destination from the input? Does it actually have to be hard
> restricted or do you end up with the hardware generating non-symmetric
> hashes because it isn't doing the XOR with both source and destination
> fields?

Do you mean allow the user to use any RSS fields as input? What do we 
gain by that?

The hardware's TOEPLITZ and SYM_TOEPLITZ functions are the same except 
for the XORing step. What gets XOR'd needs to be programmed (Patch 5: 
ice_rss_config_xor()) and we are programming the hardware to XOR the src 
and dst fields to get this hash symmetry. If any fields that are not 
swapped in the other flow direction or if (for example) only src is 
used, the hardware will generate non-symmetric hash.


> 
> My thought would be to possibly just look at reducing your messaging
> to a warning from the driver if the inputs are not symmetric, but you
> have your symmetric xor hash function enabled.

With the restrictions (to be moved into ice_ethtool), the user is unable 
to use non-symmetric inputs.
Alexander Duyck Oct. 17, 2023, 8:03 p.m. UTC | #11 
On Tue, Oct 17, 2023 at 12:15 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
>
>
>
> On 2023-10-17 12:42, Alexander Duyck wrote:
> > On Mon, Oct 16, 2023 at 5:08 PM Ahmed Zaki <ahmed.zaki@intel.com> wrote:
> >>
> >>
> >>
> >> On 2023-10-16 17:30, Jakub Kicinski wrote:
> >>> On Mon, 16 Oct 2023 15:55:21 -0700 Alexander Duyck wrote:
> >>>> It would make more sense to just add it as a variant hash function of
> >>>> toeplitz. If you did it right you could probably make the formatting
> >>>> pretty, something like:
> >>>> RSS hash function:
> >>>>       toeplitz: on
> >>>>           symmetric xor: on
> >>>>       xor: off
> >>>>       crc32: off
> >>>>
> >>>> It doesn't make sense to place it in the input flags and will just
> >>>> cause quick congestion as things get added there. This is an algorithm
> >>>> change so it makes more sense to place it there.
> >>>
> >>> Algo is also a bit confusing, it's more like key pre-processing?
> >>> There's nothing toeplitz about xoring input fields. Works as well
> >>> for CRC32.. or XOR.
> >>>
> >>> We can use one of the reserved fields of struct ethtool_rxfh to carry
> >>> this extension. I think I asked for this at some point, but there's
> >>> only so much repeated feedback one can send in a day :(
> >>
> >> Sorry you felt that. I took you comment [1]:
> >>
> >> "Using hashing algo for configuring fields feels like a dirty hack".
> >>
> >> To mean that the we should not use the hfunc API ("ethtool_rxfh"). This
> >> is why in the new series I chose to configure the RSS fields. This also
> >> provides the user with more control and better granularity on which
> >> flow-types to be symmetric, and which protocols (L3 and/or L4) to use. I
> >> have no idea how to do any of these via hfunc/ethtool_rxfh API so it
> >> seemed a better approach.
> >>
> >> I see you marked the series as "Changes Requested". I will send a new
> >> version tomorrow and move the sanity checks inside ice_ethtool.
> >>
> >>
> >> [1]: https://lore.kernel.org/netdev/20230824174336.6fb801d5@kernel.org/
> >
> > So one question I would have is what happens if you were to ignore the
> > extra configuration that prevents people from disabling either source
> > or destination from the input? Does it actually have to be hard
> > restricted or do you end up with the hardware generating non-symmetric
> > hashes because it isn't doing the XOR with both source and destination
> > fields?
>
> Do you mean allow the user to use any RSS fields as input? What do we
> gain by that?
>
> The hardware's TOEPLITZ and SYM_TOEPLITZ functions are the same except
> for the XORing step. What gets XOR'd needs to be programmed (Patch 5:
> ice_rss_config_xor()) and we are programming the hardware to XOR the src
> and dst fields to get this hash symmetry. If any fields that are not
> swapped in the other flow direction or if (for example) only src is
> used, the hardware will generate non-symmetric hash.

The point I am getting at is to determine if the
toeplitz-symmetric-xor is actually changes to the inputs or a change
to the algorithm. Based on your description here it is essentially a
subset of toeplitz, and all of the same inputs would apply. All you
have essentially done is collapsed the key. Rather than symmetric
toeplitz this could almost be considered simplified toeplitz.

One side effect of XORing the source and destination data is that you
can essentially collapse the key. You could XOR together the 5 DWORDs
(159 bits) associated with the source and destination IP portion of
the key, and then do the same with the 3 WORDs (47 bits) associated
with the source and destination port. Then you would only have to
process the XORed inputs. As a result you are going to lose a fair bit
of entropy since it effectively cuts the input length and key length
in half. The same could essentially be done by doing a bit of key
manipulation, the simplest approach being using a 16b repeating key
value, and the more nuanced requiring paying attention to IP and port
boundaries in terms of repetition. I would say because of the extra
processing steps it is a different hfunc versus just being a different
set of inputs.

> >
> > My thought would be to possibly just look at reducing your messaging
> > to a warning from the driver if the inputs are not symmetric, but you
> > have your symmetric xor hash function enabled.
>
> With the restrictions (to be moved into ice_ethtool), the user is unable
> to use non-symmetric inputs.

I think a warning would make more sense than an outright restriction.
You could warn on both the enabling if the mask is already unbalanced,
or you could warn if the mask is set to be unbalanced after enabling
your hashing.
Jakub Kicinski Oct. 17, 2023, 8:17 p.m. UTC | #12 
On Tue, 17 Oct 2023 11:37:52 -0700 Alexander Duyck wrote:
> > Algo is also a bit confusing, it's more like key pre-processing?
> > There's nothing toeplitz about xoring input fields. Works as well
> > for CRC32.. or XOR.  
> 
> I agree that the change to the algorithm doesn't necessarily have
> anything to do with toeplitz, however it is still a change to the
> algorithm by performing the extra XOR on the inputs prior to
> processing. That is why I figured it might make sense to just add a
> new hfunc value that would mean toeplitz w/ symmetric XOR.

XOR is just one form of achieving symmetric hashing, sorting is another.

> > We can use one of the reserved fields of struct ethtool_rxfh to carry
> > this extension. I think I asked for this at some point, but there's
> > only so much repeated feedback one can send in a day :(  
> 
> Why add an extra reserved field when this is just a variant on a hash
> function? I view it as not being dissimilar to how we handle TSO or
> tx-checksumming. It would make sense to me to just set something like
> toeplitz-symmetric-xor to on in order to turn this on.

It's entirely orthogonal. {sym-XOR, sym-sort} x {toep, crc, xor} -
all combinations can work.

Forget the "is it algo or not algo" question, just purely from data
normalization perspective, in terms of the API, if combinations make
sense they should be controllable independently.

https://en.wikipedia.org/wiki/First_normal_form
Jakub Kicinski Oct. 17, 2023, 8:19 p.m. UTC | #13 
On Tue, 17 Oct 2023 13:03:39 -0700 Alexander Duyck wrote:
> > > My thought would be to possibly just look at reducing your messaging
> > > to a warning from the driver if the inputs are not symmetric, but you
> > > have your symmetric xor hash function enabled.  
> >
> > With the restrictions (to be moved into ice_ethtool), the user is unable
> > to use non-symmetric inputs.  
> 
> I think a warning would make more sense than an outright restriction.
> You could warn on both the enabling if the mask is already unbalanced,
> or you could warn if the mask is set to be unbalanced after enabling
> your hashing.

Either it's a valid configuration or we should error out in the core.
Keep in mind that we can always _loosen_ the restriction, like you
asked for VLAN ID, but we can never _tighten_ it without breaking uAPI.
So error.
Alexander Duyck Oct. 17, 2023, 8:28 p.m. UTC | #14 
On Tue, Oct 17, 2023 at 1:20 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 Oct 2023 13:03:39 -0700 Alexander Duyck wrote:
> > > > My thought would be to possibly just look at reducing your messaging
> > > > to a warning from the driver if the inputs are not symmetric, but you
> > > > have your symmetric xor hash function enabled.
> > >
> > > With the restrictions (to be moved into ice_ethtool), the user is unable
> > > to use non-symmetric inputs.
> >
> > I think a warning would make more sense than an outright restriction.
> > You could warn on both the enabling if the mask is already unbalanced,
> > or you could warn if the mask is set to be unbalanced after enabling
> > your hashing.
>
> Either it's a valid configuration or we should error out in the core.
> Keep in mind that we can always _loosen_ the restriction, like you
> asked for VLAN ID, but we can never _tighten_ it without breaking uAPI.
> So error.

I would say it is a valid configuration then. If the user opts to
shoot themselves in the foot then so be it. It doesn't actually break
anything and is just there to make sure the hashing conforms to the
marketing use case.
Alexander Duyck Oct. 17, 2023, 8:41 p.m. UTC | #15 
On Tue, Oct 17, 2023 at 1:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 Oct 2023 11:37:52 -0700 Alexander Duyck wrote:
> > > Algo is also a bit confusing, it's more like key pre-processing?
> > > There's nothing toeplitz about xoring input fields. Works as well
> > > for CRC32.. or XOR.
> >
> > I agree that the change to the algorithm doesn't necessarily have
> > anything to do with toeplitz, however it is still a change to the
> > algorithm by performing the extra XOR on the inputs prior to
> > processing. That is why I figured it might make sense to just add a
> > new hfunc value that would mean toeplitz w/ symmetric XOR.
>
> XOR is just one form of achieving symmetric hashing, sorting is another.

Right, but there are huge algorithmic differences between the two.
With sorting you don't lose any entropy, whereas with XOR you do. For
example one side effect of XOR is that for every two hosts on the same
IP subnet the IP subnets will cancel out. As such with the same key
192.168.0.1->192.168.0.2 will hash out essentially the same as
fc::1->fc::2.

> > > We can use one of the reserved fields of struct ethtool_rxfh to carry
> > > this extension. I think I asked for this at some point, but there's
> > > only so much repeated feedback one can send in a day :(
> >
> > Why add an extra reserved field when this is just a variant on a hash
> > function? I view it as not being dissimilar to how we handle TSO or
> > tx-checksumming. It would make sense to me to just set something like
> > toeplitz-symmetric-xor to on in order to turn this on.
>
> It's entirely orthogonal. {sym-XOR, sym-sort} x {toep, crc, xor} -
> all combinations can work.
>
> Forget the "is it algo or not algo" question, just purely from data
> normalization perspective, in terms of the API, if combinations make
> sense they should be controllable independently.
>
> https://en.wikipedia.org/wiki/First_normal_form

I am thinking of this from a software engineering perspective. This
symmetric-xor aka simplified-toeplitz is actually much cheaper to
implement in software than the original. As such I would want it to be
considered a separate algorithm as I could make use of something like
that when having to implement RSS in QEMU for instance. Based on
earlier comments it doesn't change the inputs, it just changes how I
have to handle the data and the key. It starts reducing things down to
something like the Intel implementation of Flow Director in terms of
how the key gets generated and hashed.

As far as sorting that is a different can of worms, but I would be
more open to that being an input specific thing as all it would affect
is the ordering of the fields, it doesn't impact how I would have to
handle the key or hash the inputs.
Ahmed Zaki Oct. 17, 2023, 10:12 p.m. UTC | #16 
On 2023-10-17 14:41, Alexander Duyck wrote:
> On Tue, Oct 17, 2023 at 1:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Tue, 17 Oct 2023 11:37:52 -0700 Alexander Duyck wrote:
>>>> Algo is also a bit confusing, it's more like key pre-processing?
>>>> There's nothing toeplitz about xoring input fields. Works as well
>>>> for CRC32.. or XOR.
>>>
>>> I agree that the change to the algorithm doesn't necessarily have
>>> anything to do with toeplitz, however it is still a change to the
>>> algorithm by performing the extra XOR on the inputs prior to
>>> processing. That is why I figured it might make sense to just add a
>>> new hfunc value that would mean toeplitz w/ symmetric XOR.
>>
>> XOR is just one form of achieving symmetric hashing, sorting is another.
> 
> Right, but there are huge algorithmic differences between the two.
> With sorting you don't lose any entropy, whereas with XOR you do. For
> example one side effect of XOR is that for every two hosts on the same
> IP subnet the IP subnets will cancel out. As such with the same key
> 192.168.0.1->192.168.0.2 will hash out essentially the same as
> fc::1->fc::2.

I agree of course that we lose entropy by XORing, but don't we also lose 
entropy, for example, if we hash only the L4 dst_port vs (ip_src, 
ip_dst, l4_src, l4_dst,..etc)? we still say we are using the same alg.


>>>> We can use one of the reserved fields of struct ethtool_rxfh to carry
>>>> this extension. I think I asked for this at some point, but there's
>>>> only so much repeated feedback one can send in a day :(
>>>
>>> Why add an extra reserved field when this is just a variant on a hash
>>> function? I view it as not being dissimilar to how we handle TSO or
>>> tx-checksumming. It would make sense to me to just set something like
>>> toeplitz-symmetric-xor to on in order to turn this on.
>>
>> It's entirely orthogonal. {sym-XOR, sym-sort} x {toep, crc, xor} -
>> all combinations can work.
>>
>> Forget the "is it algo or not algo" question, just purely from data
>> normalization perspective, in terms of the API, if combinations make
>> sense they should be controllable independently.
>>
>> https://en.wikipedia.org/wiki/First_normal_form
> 
> I am thinking of this from a software engineering perspective. This
> symmetric-xor aka simplified-toeplitz is actually much cheaper to
> implement in software than the original. As such I would want it to be
> considered a separate algorithm as I could make use of something like
> that when having to implement RSS in QEMU for instance. Based on
> earlier comments it doesn't change the inputs, it just changes how I
> have to handle the data and the key. It starts reducing things down to
> something like the Intel implementation of Flow Director in terms of
> how the key gets generated and hashed.

The key is independent of all of this discussion. It is set by the user 
and whatever that key is, the hardware (after properly configuring what 
fields are XOR'd) will generate the symmetric hash from the input data. 
The "alg" does not handle or manipulate the key.
Jakub Kicinski Oct. 18, 2023, 12:34 a.m. UTC | #17 
On Tue, 17 Oct 2023 13:41:18 -0700 Alexander Duyck wrote:
> I am thinking of this from a software engineering perspective. This
> symmetric-xor aka simplified-toeplitz is actually much cheaper to
> implement in software than the original. As such I would want it to be
> considered a separate algorithm as I could make use of something like
> that when having to implement RSS in QEMU for instance.

That's exactly why XOR and CRC32 _algorithms_ already exist.
CPUs have instructions to do them word at a time. 

	ETH_RSS_HASH_TOP_BIT, /* Configurable RSS hash function -
	Toeplitz */
	ETH_RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
	ETH_RSS_HASH_CRC32_BIT, /* Configurable RSS hash function - Crc32 */

If efficient SW implementation is important why do some weird
bastardized para-toeplitz and not crc32? Hashes fairly well
from what I recall with the older NFPs. x86 has an instruction
for it, IIRC it was part of SSE but on normal registers.

> Based on earlier comments it doesn't change the inputs, it just
> changes how I have to handle the data and the key. It starts reducing
> things down to something like the Intel implementation of Flow
> Director in terms of how the key gets generated and hashed.

About Flow Director I know only that it is bad :)
Alexander Duyck Oct. 18, 2023, 6:12 p.m. UTC | #18 
On Tue, Oct 17, 2023 at 5:34 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 Oct 2023 13:41:18 -0700 Alexander Duyck wrote:
> > I am thinking of this from a software engineering perspective. This
> > symmetric-xor aka simplified-toeplitz is actually much cheaper to
> > implement in software than the original. As such I would want it to be
> > considered a separate algorithm as I could make use of something like
> > that when having to implement RSS in QEMU for instance.
>
> That's exactly why XOR and CRC32 _algorithms_ already exist.
> CPUs have instructions to do them word at a time.
>
>         ETH_RSS_HASH_TOP_BIT, /* Configurable RSS hash function -
>         Toeplitz */
>         ETH_RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
>         ETH_RSS_HASH_CRC32_BIT, /* Configurable RSS hash function - Crc32 */
>
> If efficient SW implementation is important why do some weird
> bastardized para-toeplitz and not crc32? Hashes fairly well
> from what I recall with the older NFPs. x86 has an instruction
> for it, IIRC it was part of SSE but on normal registers.

If we want to not support that I would be fine with that too. In my
view this is about as secure as using the 16b repeating key.

> > Based on earlier comments it doesn't change the inputs, it just
> > changes how I have to handle the data and the key. It starts reducing
> > things down to something like the Intel implementation of Flow
> > Director in terms of how the key gets generated and hashed.
>
> About Flow Director I know only that it is bad :)

Yeah, and that is my concern w/ the symmetric XOR is that it isn't
good. It opens up the toeplitz hash to exploitation. You can target
the same bucket by just making sure that source IP and port XOR with
destination IP and port to the same value. That can be done by adding
the same amount to each side. So there are 2^144 easily predictable
possible combinations that will end up in the same hash bucket. Seems
like it might be something that could be exploitable. That is why I
want it marked out as a separate algo since it is essentially
destroying entropy before we even get to the Toeplitz portion of the
hash. As such it isn't a hash I would want to use for anything that is
meant to spread workload since it is so easily exploitable.
Jakub Kicinski Oct. 18, 2023, 11:50 p.m. UTC | #19 
On Wed, 18 Oct 2023 11:12:13 -0700 Alexander Duyck wrote:
> > > Based on earlier comments it doesn't change the inputs, it just
> > > changes how I have to handle the data and the key. It starts reducing
> > > things down to something like the Intel implementation of Flow
> > > Director in terms of how the key gets generated and hashed.  
> >
> > About Flow Director I know only that it is bad :)  
> 
> Yeah, and that is my concern w/ the symmetric XOR is that it isn't
> good. It opens up the toeplitz hash to exploitation. You can target
> the same bucket by just making sure that source IP and port XOR with
> destination IP and port to the same value. That can be done by adding
> the same amount to each side. So there are 2^144 easily predictable
> possible combinations that will end up in the same hash bucket. Seems
> like it might be something that could be exploitable. That is why I
> want it marked out as a separate algo since it is essentially
> destroying entropy before we even get to the Toeplitz portion of the
> hash. As such it isn't a hash I would want to use for anything that is
> meant to spread workload since it is so easily exploitable.

I see your point.

Which is not to say that I know what to do about it. crc or any
future secure algo will get destroyed all the same. It's the input
entropy that gets destroyed, independently of the algo.

We already support xor, and it doesn't come with a warning saying
it's insecure so we kind of assume user knows what they are doing.

I think the API we pick for configuring sym-xor should be the same as
sym-sort. And the "makes algo insecure" argument won't apply to sort.

IMO fat warning in the documentation and ethtool man saying that this
makes the algo (any / all) vulnerable to attack would be enough.
Willem?
Ahmed Zaki Oct. 20, 2023, 9:24 p.m. UTC | #20 
On 2023-10-18 17:50, Jakub Kicinski wrote:
> On Wed, 18 Oct 2023 11:12:13 -0700 Alexander Duyck wrote:
>>>> Based on earlier comments it doesn't change the inputs, it just
>>>> changes how I have to handle the data and the key. It starts reducing
>>>> things down to something like the Intel implementation of Flow
>>>> Director in terms of how the key gets generated and hashed.
>>>
>>> About Flow Director I know only that it is bad :)
>>
>> Yeah, and that is my concern w/ the symmetric XOR is that it isn't
>> good. It opens up the toeplitz hash to exploitation. You can target
>> the same bucket by just making sure that source IP and port XOR with
>> destination IP and port to the same value. That can be done by adding
>> the same amount to each side. So there are 2^144 easily predictable
>> possible combinations that will end up in the same hash bucket. Seems
>> like it might be something that could be exploitable. That is why I
>> want it marked out as a separate algo since it is essentially
>> destroying entropy before we even get to the Toeplitz portion of the
>> hash. As such it isn't a hash I would want to use for anything that is
>> meant to spread workload since it is so easily exploitable.
> 
> I see your point.
> 
> Which is not to say that I know what to do about it. crc or any
> future secure algo will get destroyed all the same. It's the input
> entropy that gets destroyed, independently of the algo.
> 
> We already support xor, and it doesn't come with a warning saying
> it's insecure so we kind of assume user knows what they are doing.
> 
> I think the API we pick for configuring sym-xor should be the same as
> sym-sort. And the "makes algo insecure" argument won't apply to sort.
> 
> IMO fat warning in the documentation and ethtool man saying that this
> makes the algo (any / all) vulnerable to attack would be enough.
> Willem?

Please advise on the next step. Should I send a new version with the Doc 
warning, or will you use v5?

Thanks.
Jakub Kicinski Oct. 20, 2023, 10:33 p.m. UTC | #21 
On Fri, 20 Oct 2023 15:24:41 -0600 Ahmed Zaki wrote:
> > IMO fat warning in the documentation and ethtool man saying that this
> > makes the algo (any / all) vulnerable to attack would be enough.
> > Willem?  
> 
> Please advise on the next step. Should I send a new version with the Doc 
> warning, or will you use v5?

Not just the doc changes:

| We can use one of the reserved fields of struct ethtool_rxfh to carry
| this extension. I think I asked for this at some point, but there's
| only so much repeated feedback one can send in a day :(

https://lore.kernel.org/all/20231016163059.23799429@kernel.org/

You can take care of that, post v6 and see what Alex and Willem say.
Ahmed Zaki Oct. 20, 2023, 11:14 p.m. UTC | #22 
On 2023-10-20 16:33, Jakub Kicinski wrote:
> On Fri, 20 Oct 2023 15:24:41 -0600 Ahmed Zaki wrote:
>>> IMO fat warning in the documentation and ethtool man saying that this
>>> makes the algo (any / all) vulnerable to attack would be enough.
>>> Willem?
>>
>> Please advise on the next step. Should I send a new version with the Doc
>> warning, or will you use v5?
> 
> Not just the doc changes:
> 
> | We can use one of the reserved fields of struct ethtool_rxfh to carry
> | this extension. I think I asked for this at some point, but there's
> | only so much repeated feedback one can send in a day :(
> 
> https://lore.kernel.org/all/20231016163059.23799429@kernel.org/

I replied to that here:

https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/

I am kind of confused now so please bear with me. ethtool either sends 
"ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the interface 
for "ethtool -X" which is used to set the RSS algorithm. But we kind of 
agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses 
"ethtool_rxnfc" (as implemented in this series).

Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would 
that work on the ethtool user interface?

Finally, a note on Alex's comment:
 >It doesn't make sense to place it in the input flags and will just
 > cause quick congestion as things get added there. This is an algorithm
 > change so it makes more sense to place it there.

the "ethtool_rxnfc->data" is 64 bits and we are only using 8 bits so far.

Thank you.
Jakub Kicinski Oct. 20, 2023, 11:49 p.m. UTC | #23 
On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
> I replied to that here:
> 
> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
> 
> I am kind of confused now so please bear with me. ethtool either sends 
> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the interface 
> for "ethtool -X" which is used to set the RSS algorithm. But we kind of 
> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses 
> "ethtool_rxnfc" (as implemented in this series).

I have no strong preference. Sounds like Alex prefers to keep it closer
to algo, which is "ethtool_rxfh".

> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would 
> that work on the ethtool user interface?

I don't know what you're asking of us. If you find the code to confusing
maybe someone at Intel can help you :|
Ahmed Zaki Oct. 21, 2023, midnight UTC | #24 
On 2023-10-20 17:49, Jakub Kicinski wrote:
> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>> I replied to that here:
>>
>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>
>> I am kind of confused now so please bear with me. ethtool either sends
>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the interface
>> for "ethtool -X" which is used to set the RSS algorithm. But we kind of
>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>> "ethtool_rxnfc" (as implemented in this series).
> 
> I have no strong preference. Sounds like Alex prefers to keep it closer
> to algo, which is "ethtool_rxfh".
> 
>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>> that work on the ethtool user interface?
> 
> I don't know what you're asking of us. If you find the code to confusing
> maybe someone at Intel can help you :|

The code is straightforward. I am confused by the requirements: don't 
add a new algorithm but use "ethtool_rxfh".

I'll see if I can get more help, may be I am missing something.
Gal Pressman Oct. 29, 2023, 12:25 p.m. UTC | #25 
On 21/10/2023 3:00, Ahmed Zaki wrote:
> 
> 
> On 2023-10-20 17:49, Jakub Kicinski wrote:
>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>> I replied to that here:
>>>
>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>
>>> I am kind of confused now so please bear with me. ethtool either sends
>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the interface
>>> for "ethtool -X" which is used to set the RSS algorithm. But we kind of
>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>> "ethtool_rxnfc" (as implemented in this series).
>>
>> I have no strong preference. Sounds like Alex prefers to keep it closer
>> to algo, which is "ethtool_rxfh".
>>
>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>> that work on the ethtool user interface?
>>
>> I don't know what you're asking of us. If you find the code to confusing
>> maybe someone at Intel can help you :|
> 
> The code is straightforward. I am confused by the requirements: don't
> add a new algorithm but use "ethtool_rxfh".
> 
> I'll see if I can get more help, may be I am missing something.
> 

What was the decision here?
Is this going to be exposed through ethtool -N or -X?
Ahmed Zaki Oct. 29, 2023, 12:42 p.m. UTC | #26 
On 2023-10-29 06:25, Gal Pressman wrote:
> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>
>>
>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>> I replied to that here:
>>>>
>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>
>>>> I am kind of confused now so please bear with me. ethtool either sends
>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the interface
>>>> for "ethtool -X" which is used to set the RSS algorithm. But we kind of
>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>> "ethtool_rxnfc" (as implemented in this series).
>>>
>>> I have no strong preference. Sounds like Alex prefers to keep it closer
>>> to algo, which is "ethtool_rxfh".
>>>
>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>> that work on the ethtool user interface?
>>>
>>> I don't know what you're asking of us. If you find the code to confusing
>>> maybe someone at Intel can help you :|
>>
>> The code is straightforward. I am confused by the requirements: don't
>> add a new algorithm but use "ethtool_rxfh".
>>
>> I'll see if I can get more help, may be I am missing something.
>>
> 
> What was the decision here?
> Is this going to be exposed through ethtool -N or -X?

I am working on a new version that uses "ethtool_rxfh" to set the 
symmetric-xor. The user will set per-device via:

ethtool -X eth0 hfunc toeplitz symmetric-xor

then specify the per-flow type RSS fields as usual:

ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n

The downside is that all flow-types will have to be either symmetric or 
asymmetric.

I should be able to send this early in the next cycle.
Gal Pressman Oct. 29, 2023, 12:48 p.m. UTC | #27 
On 29/10/2023 14:42, Ahmed Zaki wrote:
> 
> 
> On 2023-10-29 06:25, Gal Pressman wrote:
>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>> I replied to that here:
>>>>>
>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>
>>>>> I am kind of confused now so please bear with me. ethtool either sends
>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>> interface
>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>> kind of
>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>
>>>> I have no strong preference. Sounds like Alex prefers to keep it closer
>>>> to algo, which is "ethtool_rxfh".
>>>>
>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>> that work on the ethtool user interface?
>>>>
>>>> I don't know what you're asking of us. If you find the code to
>>>> confusing
>>>> maybe someone at Intel can help you :|
>>>
>>> The code is straightforward. I am confused by the requirements: don't
>>> add a new algorithm but use "ethtool_rxfh".
>>>
>>> I'll see if I can get more help, may be I am missing something.
>>>
>>
>> What was the decision here?
>> Is this going to be exposed through ethtool -N or -X?
> 
> I am working on a new version that uses "ethtool_rxfh" to set the
> symmetric-xor. The user will set per-device via:
> 
> ethtool -X eth0 hfunc toeplitz symmetric-xor
> 
> then specify the per-flow type RSS fields as usual:
> 
> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
> 
> The downside is that all flow-types will have to be either symmetric or
> asymmetric.

Why are we making the interface less flexible than it can be with -N?
Ahmed Zaki Oct. 29, 2023, 4:59 p.m. UTC | #28 
On 2023-10-29 06:48, Gal Pressman wrote:
> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>
>>
>> On 2023-10-29 06:25, Gal Pressman wrote:
>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>
>>>>
>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>> I replied to that here:
>>>>>>
>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>
>>>>>> I am kind of confused now so please bear with me. ethtool either sends
>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>> interface
>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>> kind of
>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>
>>>>> I have no strong preference. Sounds like Alex prefers to keep it closer
>>>>> to algo, which is "ethtool_rxfh".
>>>>>
>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>>> that work on the ethtool user interface?
>>>>>
>>>>> I don't know what you're asking of us. If you find the code to
>>>>> confusing
>>>>> maybe someone at Intel can help you :|
>>>>
>>>> The code is straightforward. I am confused by the requirements: don't
>>>> add a new algorithm but use "ethtool_rxfh".
>>>>
>>>> I'll see if I can get more help, may be I am missing something.
>>>>
>>>
>>> What was the decision here?
>>> Is this going to be exposed through ethtool -N or -X?
>>
>> I am working on a new version that uses "ethtool_rxfh" to set the
>> symmetric-xor. The user will set per-device via:
>>
>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>
>> then specify the per-flow type RSS fields as usual:
>>
>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>
>> The downside is that all flow-types will have to be either symmetric or
>> asymmetric.
> 
> Why are we making the interface less flexible than it can be with -N?

Alexander Duyck prefers to implement the "symmetric-xor" interface as an 
algorithm or extension (please refer to previous messages), but ethtool 
does not provide flowtype/RSS fields setting via "-X". The above was the 
best solution that we (at Intel) could think of.


Another solution would be to add a similar flowtype interface to "-X":

ethtool -X eth0 hfunc toeplitz [symmetric-xor rx-flow-hash <flow_type>]

which will allow the user to set "symmetric-xor" per flow-type. IMHO 
such approach is confusing; consider if the user sets:

ethtool -X eth0 ALG-1 symmetric-xor rx-flow-hash tcp4

and then:

ethtool -X eth0 ALG-2

should we switch tcp4 to ALG-2? Also, just the idea of replicating 
"rx-flow-hash" did not sound good overall to me.


Anyway, we thought that, if we are using "-X", then limiting all 
flow-types to whatever is set with "-X" is cleaner and works best with 
the current ethtool design. Any other suggestions are welcome of course.
Gal Pressman Oct. 31, 2023, noon UTC | #29 
On 29/10/2023 18:59, Ahmed Zaki wrote:
> 
> 
> On 2023-10-29 06:48, Gal Pressman wrote:
>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>
>>>>>
>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>> I replied to that here:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>
>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>> sends
>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>> interface
>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>> kind of
>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>
>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>> closer
>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>
>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>>>> that work on the ethtool user interface?
>>>>>>
>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>> confusing
>>>>>> maybe someone at Intel can help you :|
>>>>>
>>>>> The code is straightforward. I am confused by the requirements: don't
>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>
>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>
>>>>
>>>> What was the decision here?
>>>> Is this going to be exposed through ethtool -N or -X?
>>>
>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>> symmetric-xor. The user will set per-device via:
>>>
>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>
>>> then specify the per-flow type RSS fields as usual:
>>>
>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>
>>> The downside is that all flow-types will have to be either symmetric or
>>> asymmetric.
>>
>> Why are we making the interface less flexible than it can be with -N?
> 
> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
> algorithm or extension (please refer to previous messages), but ethtool
> does not provide flowtype/RSS fields setting via "-X". The above was the
> best solution that we (at Intel) could think of.

OK, it's a weird we're deliberately limiting our interface, given
there's already hardware that supports controlling symmetric hashing per
flow type.

I saw you mentioned the way ice hardware implements symmetric-xor
somewhere, it definitely needs to be added somewhere in our
documentation to prevent confusion.
mlx5 hardware also does symmetric hashing with xor, but not exactly as
you described, we need the algorithm to be clear.
Ahmed Zaki Oct. 31, 2023, 2:40 p.m. UTC | #30 
On 2023-10-31 06:00, Gal Pressman wrote:
> On 29/10/2023 18:59, Ahmed Zaki wrote:
>>
>>
>> On 2023-10-29 06:48, Gal Pressman wrote:
>>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>>
>>>>
>>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>>
>>>>>>
>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>>> I replied to that here:
>>>>>>>>
>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>>
>>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>>> sends
>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>>> interface
>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>>> kind of
>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>>
>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>>> closer
>>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>>
>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>>>>> that work on the ethtool user interface?
>>>>>>>
>>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>>> confusing
>>>>>>> maybe someone at Intel can help you :|
>>>>>>
>>>>>> The code is straightforward. I am confused by the requirements: don't
>>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>>
>>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>>
>>>>>
>>>>> What was the decision here?
>>>>> Is this going to be exposed through ethtool -N or -X?
>>>>
>>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>>> symmetric-xor. The user will set per-device via:
>>>>
>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>>
>>>> then specify the per-flow type RSS fields as usual:
>>>>
>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>>
>>>> The downside is that all flow-types will have to be either symmetric or
>>>> asymmetric.
>>>
>>> Why are we making the interface less flexible than it can be with -N?
>>
>> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
>> algorithm or extension (please refer to previous messages), but ethtool
>> does not provide flowtype/RSS fields setting via "-X". The above was the
>> best solution that we (at Intel) could think of.
> 
> OK, it's a weird we're deliberately limiting our interface, given
> there's already hardware that supports controlling symmetric hashing per
> flow type.
> 
> I saw you mentioned the way ice hardware implements symmetric-xor
> somewhere, it definitely needs to be added somewhere in our
> documentation to prevent confusion.
> mlx5 hardware also does symmetric hashing with xor, but not exactly as
> you described, we need the algorithm to be clear.

Sure. I will add more ice-specific doc in:
Documentation/networking/device_drivers/ethernet/intel/ice.rst
Gal Pressman Oct. 31, 2023, 2:45 p.m. UTC | #31 
On 31/10/2023 16:40, Ahmed Zaki wrote:
> 
> 
> On 2023-10-31 06:00, Gal Pressman wrote:
>> On 29/10/2023 18:59, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-29 06:48, Gal Pressman wrote:
>>>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>>>
>>>>>
>>>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>>>> I replied to that here:
>>>>>>>>>
>>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>>>
>>>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>>>> sends
>>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>>>> interface
>>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>>>> kind of
>>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>>>
>>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>>>> closer
>>>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>>>
>>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how
>>>>>>>>> would
>>>>>>>>> that work on the ethtool user interface?
>>>>>>>>
>>>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>>>> confusing
>>>>>>>> maybe someone at Intel can help you :|
>>>>>>>
>>>>>>> The code is straightforward. I am confused by the requirements:
>>>>>>> don't
>>>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>>>
>>>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>>>
>>>>>>
>>>>>> What was the decision here?
>>>>>> Is this going to be exposed through ethtool -N or -X?
>>>>>
>>>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>>>> symmetric-xor. The user will set per-device via:
>>>>>
>>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>>>
>>>>> then specify the per-flow type RSS fields as usual:
>>>>>
>>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>>>
>>>>> The downside is that all flow-types will have to be either
>>>>> symmetric or
>>>>> asymmetric.
>>>>
>>>> Why are we making the interface less flexible than it can be with -N?
>>>
>>> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
>>> algorithm or extension (please refer to previous messages), but ethtool
>>> does not provide flowtype/RSS fields setting via "-X". The above was the
>>> best solution that we (at Intel) could think of.
>>
>> OK, it's a weird we're deliberately limiting our interface, given
>> there's already hardware that supports controlling symmetric hashing per
>> flow type.
>>
>> I saw you mentioned the way ice hardware implements symmetric-xor
>> somewhere, it definitely needs to be added somewhere in our
>> documentation to prevent confusion.
>> mlx5 hardware also does symmetric hashing with xor, but not exactly as
>> you described, we need the algorithm to be clear.
> 
> Sure. I will add more ice-specific doc in:
> Documentation/networking/device_drivers/ethernet/intel/ice.rst

I was thinking of somewhere more generic, where ethtool users (not
necessarily ice users) can refer to.

Perhaps Documentation/networking/ethtool-netlink.rst? Or ethtool man page?
Alexander Duyck Oct. 31, 2023, 2:59 p.m. UTC | #32 
On Tue, Oct 31, 2023 at 5:01 AM Gal Pressman <gal@nvidia.com> wrote:
>
> On 29/10/2023 18:59, Ahmed Zaki wrote:
> >
> >
> > On 2023-10-29 06:48, Gal Pressman wrote:
> >> On 29/10/2023 14:42, Ahmed Zaki wrote:
> >>>
> >>>
> >>> On 2023-10-29 06:25, Gal Pressman wrote:
> >>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
> >>>>>
> >>>>>
> >>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
> >>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
> >>>>>>> I replied to that here:
> >>>>>>>
> >>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
> >>>>>>>
> >>>>>>> I am kind of confused now so please bear with me. ethtool either
> >>>>>>> sends
> >>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
> >>>>>>> interface
> >>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
> >>>>>>> kind of
> >>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
> >>>>>>> "ethtool_rxnfc" (as implemented in this series).
> >>>>>>
> >>>>>> I have no strong preference. Sounds like Alex prefers to keep it
> >>>>>> closer
> >>>>>> to algo, which is "ethtool_rxfh".
> >>>>>>
> >>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
> >>>>>>> that work on the ethtool user interface?
> >>>>>>
> >>>>>> I don't know what you're asking of us. If you find the code to
> >>>>>> confusing
> >>>>>> maybe someone at Intel can help you :|
> >>>>>
> >>>>> The code is straightforward. I am confused by the requirements: don't
> >>>>> add a new algorithm but use "ethtool_rxfh".
> >>>>>
> >>>>> I'll see if I can get more help, may be I am missing something.
> >>>>>
> >>>>
> >>>> What was the decision here?
> >>>> Is this going to be exposed through ethtool -N or -X?
> >>>
> >>> I am working on a new version that uses "ethtool_rxfh" to set the
> >>> symmetric-xor. The user will set per-device via:
> >>>
> >>> ethtool -X eth0 hfunc toeplitz symmetric-xor
> >>>
> >>> then specify the per-flow type RSS fields as usual:
> >>>
> >>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
> >>>
> >>> The downside is that all flow-types will have to be either symmetric or
> >>> asymmetric.
> >>
> >> Why are we making the interface less flexible than it can be with -N?
> >
> > Alexander Duyck prefers to implement the "symmetric-xor" interface as an
> > algorithm or extension (please refer to previous messages), but ethtool
> > does not provide flowtype/RSS fields setting via "-X". The above was the
> > best solution that we (at Intel) could think of.
>
> OK, it's a weird we're deliberately limiting our interface, given
> there's already hardware that supports controlling symmetric hashing per
> flow type.
>
> I saw you mentioned the way ice hardware implements symmetric-xor
> somewhere, it definitely needs to be added somewhere in our
> documentation to prevent confusion.
> mlx5 hardware also does symmetric hashing with xor, but not exactly as
> you described, we need the algorithm to be clear.

It is precisely because of the way the symmetric-xor implements it
that I suggested that they change the algo type instead of the input
fields.

Instead of doing something such as rearranging the inputs, what they
do is start XORing them together and then using those values for both
the source and destination ports. It would be one thing if they
swapped them, but instead they destroy the entropy provided by XORing
the values together and then doubling them up in the source and
destination fields. The result is the hash value becomes predictable
in that once you know the target you just have to offset the source
and destination port/IP by the same amount so that they hash out to
the same values, and as a result it would make DDoS attacks based on
the RSS hash much easier.

Where I draw the line in this is if we start losing entropy without
explicitly removing the value then it is part of the algo, whereas if
it is something such as placement or us explicitly saying we don't
want certain fields in there then it would be part of the input.
Adding fields to the input should increase or at least maintain the
entropy is my point of view.
Ahmed Zaki Oct. 31, 2023, 3:14 p.m. UTC | #33 
On 2023-10-31 08:45, Gal Pressman wrote:
> On 31/10/2023 16:40, Ahmed Zaki wrote:
>>
>>
>> On 2023-10-31 06:00, Gal Pressman wrote:
>>> On 29/10/2023 18:59, Ahmed Zaki wrote:
>>>>
>>>>
>>>> On 2023-10-29 06:48, Gal Pressman wrote:
>>>>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>>>>
>>>>>>
>>>>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>>>>> I replied to that here:
>>>>>>>>>>
>>>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>>>>
>>>>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>>>>> sends
>>>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>>>>> interface
>>>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>>>>> kind of
>>>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>>>>
>>>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>>>>> closer
>>>>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>>>>
>>>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how
>>>>>>>>>> would
>>>>>>>>>> that work on the ethtool user interface?
>>>>>>>>>
>>>>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>>>>> confusing
>>>>>>>>> maybe someone at Intel can help you :|
>>>>>>>>
>>>>>>>> The code is straightforward. I am confused by the requirements:
>>>>>>>> don't
>>>>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>>>>
>>>>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>>>>
>>>>>>>
>>>>>>> What was the decision here?
>>>>>>> Is this going to be exposed through ethtool -N or -X?
>>>>>>
>>>>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>>>>> symmetric-xor. The user will set per-device via:
>>>>>>
>>>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>>>>
>>>>>> then specify the per-flow type RSS fields as usual:
>>>>>>
>>>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>>>>
>>>>>> The downside is that all flow-types will have to be either
>>>>>> symmetric or
>>>>>> asymmetric.
>>>>>
>>>>> Why are we making the interface less flexible than it can be with -N?
>>>>
>>>> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
>>>> algorithm or extension (please refer to previous messages), but ethtool
>>>> does not provide flowtype/RSS fields setting via "-X". The above was the
>>>> best solution that we (at Intel) could think of.
>>>
>>> OK, it's a weird we're deliberately limiting our interface, given
>>> there's already hardware that supports controlling symmetric hashing per
>>> flow type.
>>>
>>> I saw you mentioned the way ice hardware implements symmetric-xor
>>> somewhere, it definitely needs to be added somewhere in our
>>> documentation to prevent confusion.
>>> mlx5 hardware also does symmetric hashing with xor, but not exactly as
>>> you described, we need the algorithm to be clear.
>>
>> Sure. I will add more ice-specific doc in:
>> Documentation/networking/device_drivers/ethernet/intel/ice.rst
> 
> I was thinking of somewhere more generic, where ethtool users (not
> necessarily ice users) can refer to.
> 
> Perhaps Documentation/networking/ethtool-netlink.rst? Or ethtool man page?

Do you mean add vendor-specific implementation details to common docs? 
Not sure if I have seen this before. Any examples?

Or, we can add a note in ethtool doc that each vendor's implementation 
is different and "Refer to your vendor's specifications for more info".
Jakub Kicinski Oct. 31, 2023, 3:20 p.m. UTC | #34 
On Tue, 31 Oct 2023 09:14:58 -0600 Ahmed Zaki wrote:
> Do you mean add vendor-specific implementation details to common docs? 
> Not sure if I have seen this before. Any examples?
> 
> Or, we can add a note in ethtool doc that each vendor's implementation 
> is different and "Refer to your vendor's specifications for more info".

Gal, can you shed any more detail on who your implementation differs?
It will help the discussion, and also - I'm not sure how you can do
xor differently..
Gal Pressman Oct. 31, 2023, 4:11 p.m. UTC | #35 
On 31/10/2023 16:59, Alexander Duyck wrote:
> On Tue, Oct 31, 2023 at 5:01 AM Gal Pressman <gal@nvidia.com> wrote:
>>
>> On 29/10/2023 18:59, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-29 06:48, Gal Pressman wrote:
>>>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>>>
>>>>>
>>>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>>>> I replied to that here:
>>>>>>>>>
>>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>>>
>>>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>>>> sends
>>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>>>> interface
>>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>>>> kind of
>>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>>>
>>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>>>> closer
>>>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>>>
>>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>>>>>> that work on the ethtool user interface?
>>>>>>>>
>>>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>>>> confusing
>>>>>>>> maybe someone at Intel can help you :|
>>>>>>>
>>>>>>> The code is straightforward. I am confused by the requirements: don't
>>>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>>>
>>>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>>>
>>>>>>
>>>>>> What was the decision here?
>>>>>> Is this going to be exposed through ethtool -N or -X?
>>>>>
>>>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>>>> symmetric-xor. The user will set per-device via:
>>>>>
>>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>>>
>>>>> then specify the per-flow type RSS fields as usual:
>>>>>
>>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>>>
>>>>> The downside is that all flow-types will have to be either symmetric or
>>>>> asymmetric.
>>>>
>>>> Why are we making the interface less flexible than it can be with -N?
>>>
>>> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
>>> algorithm or extension (please refer to previous messages), but ethtool
>>> does not provide flowtype/RSS fields setting via "-X". The above was the
>>> best solution that we (at Intel) could think of.
>>
>> OK, it's a weird we're deliberately limiting our interface, given
>> there's already hardware that supports controlling symmetric hashing per
>> flow type.
>>
>> I saw you mentioned the way ice hardware implements symmetric-xor
>> somewhere, it definitely needs to be added somewhere in our
>> documentation to prevent confusion.
>> mlx5 hardware also does symmetric hashing with xor, but not exactly as
>> you described, we need the algorithm to be clear.
> 
> It is precisely because of the way the symmetric-xor implements it
> that I suggested that they change the algo type instead of the input
> fields.
> 
> Instead of doing something such as rearranging the inputs, what they
> do is start XORing them together and then using those values for both
> the source and destination ports. It would be one thing if they
> swapped them, but instead they destroy the entropy provided by XORing
> the values together and then doubling them up in the source and
> destination fields. The result is the hash value becomes predictable
> in that once you know the target you just have to offset the source
> and destination port/IP by the same amount so that they hash out to
> the same values, and as a result it would make DDoS attacks based on
> the RSS hash much easier.
> 
> Where I draw the line in this is if we start losing entropy without
> explicitly removing the value then it is part of the algo, whereas if
> it is something such as placement or us explicitly saying we don't
> want certain fields in there then it would be part of the input.
> Adding fields to the input should increase or at least maintain the
> entropy is my point of view.

Thanks for the detailed summary, that was helpful.

Though, if a vendor chooses to implement symmetric by sorting, we will
still have it as part of the algorithm, not input.

My main concern was about losing the ability to control symmetric per
flow-type, but I guess we can resolve that if the need arises.
Gal Pressman Oct. 31, 2023, 4:12 p.m. UTC | #36 
On 31/10/2023 17:14, Ahmed Zaki wrote:
> 
> 
> On 2023-10-31 08:45, Gal Pressman wrote:
>> On 31/10/2023 16:40, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-31 06:00, Gal Pressman wrote:
>>>> On 29/10/2023 18:59, Ahmed Zaki wrote:
>>>>>
>>>>>
>>>>> On 2023-10-29 06:48, Gal Pressman wrote:
>>>>>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>>>>>> I replied to that here:
>>>>>>>>>>>
>>>>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>>>>>
>>>>>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>>>>>> sends
>>>>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>>>>>> interface
>>>>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>>>>>> kind of
>>>>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that
>>>>>>>>>>> uses
>>>>>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>>>>>
>>>>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>>>>>> closer
>>>>>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>>>>>
>>>>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how
>>>>>>>>>>> would
>>>>>>>>>>> that work on the ethtool user interface?
>>>>>>>>>>
>>>>>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>>>>>> confusing
>>>>>>>>>> maybe someone at Intel can help you :|
>>>>>>>>>
>>>>>>>>> The code is straightforward. I am confused by the requirements:
>>>>>>>>> don't
>>>>>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>>>>>
>>>>>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>>>>>
>>>>>>>>
>>>>>>>> What was the decision here?
>>>>>>>> Is this going to be exposed through ethtool -N or -X?
>>>>>>>
>>>>>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>>>>>> symmetric-xor. The user will set per-device via:
>>>>>>>
>>>>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>>>>>
>>>>>>> then specify the per-flow type RSS fields as usual:
>>>>>>>
>>>>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>>>>>
>>>>>>> The downside is that all flow-types will have to be either
>>>>>>> symmetric or
>>>>>>> asymmetric.
>>>>>>
>>>>>> Why are we making the interface less flexible than it can be with -N?
>>>>>
>>>>> Alexander Duyck prefers to implement the "symmetric-xor" interface
>>>>> as an
>>>>> algorithm or extension (please refer to previous messages), but
>>>>> ethtool
>>>>> does not provide flowtype/RSS fields setting via "-X". The above
>>>>> was the
>>>>> best solution that we (at Intel) could think of.
>>>>
>>>> OK, it's a weird we're deliberately limiting our interface, given
>>>> there's already hardware that supports controlling symmetric hashing
>>>> per
>>>> flow type.
>>>>
>>>> I saw you mentioned the way ice hardware implements symmetric-xor
>>>> somewhere, it definitely needs to be added somewhere in our
>>>> documentation to prevent confusion.
>>>> mlx5 hardware also does symmetric hashing with xor, but not exactly as
>>>> you described, we need the algorithm to be clear.
>>>
>>> Sure. I will add more ice-specific doc in:
>>> Documentation/networking/device_drivers/ethernet/intel/ice.rst
>>
>> I was thinking of somewhere more generic, where ethtool users (not
>> necessarily ice users) can refer to.
>>
>> Perhaps Documentation/networking/ethtool-netlink.rst? Or ethtool man
>> page?
> 
> Do you mean add vendor-specific implementation details to common docs?
> Not sure if I have seen this before. Any examples?
> 
> Or, we can add a note in ethtool doc that each vendor's implementation
> is different and "Refer to your vendor's specifications for more info".

It's a generic ethtool flag, its documentation shouldn't be vendor specific.
The documentation should reflect the exact details about the algorithm,
then other vendors can either use it, or add a new symmetric flag and
document it separately.
Gal Pressman Oct. 31, 2023, 4:13 p.m. UTC | #37 
On 31/10/2023 17:20, Jakub Kicinski wrote:
> On Tue, 31 Oct 2023 09:14:58 -0600 Ahmed Zaki wrote:
>> Do you mean add vendor-specific implementation details to common docs? 
>> Not sure if I have seen this before. Any examples?
>>
>> Or, we can add a note in ethtool doc that each vendor's implementation 
>> is different and "Refer to your vendor's specifications for more info".
> 
> Gal, can you shed any more detail on who your implementation differs?
> It will help the discussion, and also - I'm not sure how you can do
> xor differently..

Sure, IIUC, ice's implementation does a:
(SRC_IP ^ DST_IP, SRC_IP ^ DST_IP, SRC_PORT ^ DST_PORT, SRC_PORT ^ DST_PORT)

Our implementation isn't exactly xor, it is:
(SRC_IP | DST_IP, SRC_IP ^ DST_IP, SRC_PORT | DST_PORT, SRC_PORT ^ DST_PORT)

The way I see it, the xor implementation should be clearly documented,
so no one uses the same flag with a different implementation by mistake.
Jakub Kicinski Oct. 31, 2023, 7:57 p.m. UTC | #38 
On Tue, 31 Oct 2023 18:13:20 +0200 Gal Pressman wrote:
> Sure, IIUC, ice's implementation does a:
> (SRC_IP ^ DST_IP, SRC_IP ^ DST_IP, SRC_PORT ^ DST_PORT, SRC_PORT ^ DST_PORT)
> 
> Our implementation isn't exactly xor, it is:
> (SRC_IP | DST_IP, SRC_IP ^ DST_IP, SRC_PORT | DST_PORT, SRC_PORT ^ DST_PORT)
> 
> The way I see it, the xor implementation should be clearly documented,
> so no one uses the same flag with a different implementation by mistake.

Got it, thanks!
diff mbox series

Patch

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).
+
 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..4e8d38fb55ce 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2018,14 +2018,19 @@  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 */
+/* XOR the corresponding source and destination fields of each specified
+ * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
+ * calculation.
+ */
+#define	RXH_SYMMETRIC_XOR	(1 << 30)
+#define	RXH_DISCARD		(1 << 31)
 
 #define	RX_CLS_FLOW_DISC	0xffffffffffffffffULL
 #define RX_CLS_FLOW_WAKE	0xfffffffffffffffeULL
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..b1bd0d4b48e8 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -980,6 +980,17 @@  static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 	if (rc)
 		return rc;
 
+	/* If a symmetric hash is requested, then:
+	 * 1 - no other fields besides IP src/dst and/or L4 src/dst
+	 * 2 - If src is set, dst must also be set
+	 */
+	if ((info.data & RXH_SYMMETRIC_XOR) &&
+	    ((info.data & ~(RXH_SYMMETRIC_XOR | RXH_IP_SRC | RXH_IP_DST |
+	      RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
+	     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
+	     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
+		return -EINVAL;
+
 	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
 	if (rc)
 		return rc;