diff mbox series

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

Message ID 20231006224726.443836-2-ahmed.zaki@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Support symmetric RSS (Toeplitz) hash | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3238 this patch: 3238
netdev/cc_maintainers warning 3 maintainers not CCed: mailhol.vincent@wanadoo.fr jdamato@fastly.com d-tatianin@yandex-team.ru
netdev/build_clang success Errors and warnings before: 1572 this patch: 1572
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3486 this patch: 3486
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 36 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ahmed Zaki Oct. 6, 2023, 10:47 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.

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

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         |  1 +
 net/ethtool/ioctl.c                  | 11 +++++++++++
 3 files changed, 18 insertions(+)

Comments

Jakub Kicinski Oct. 7, 2023, 12:22 a.m. UTC | #1
On Fri,  6 Oct 2023 16:47:21 -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.
> 
> 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
> 
> or turn symmetry off (asymmetric) by:
> 
>     # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n

Thanks for the changes, code looks good!

The question left unanswered is whether we should care about the exact
implementation of the symmetry (xor, xor duplicate, sort fields).
Toeplitz-based RSS is very precisely specified, so we may want to carry
that precision into the symmetric behavior. I have a weak preference 
to do so... but no willingness to argue with you, so let me put Willem
on the spot and have him make a decision :)

Please make sure to CC Willem and anyone else who commented on previous
revisions on future versions!
Willem de Bruijn Oct. 7, 2023, 9:01 a.m. UTC | #2
On Fri, Oct 6, 2023 at 7:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri,  6 Oct 2023 16:47:21 -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.
> >
> > 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
> >
> > or turn symmetry off (asymmetric) by:
> >
> >     # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>
> Thanks for the changes, code looks good!
>
> The question left unanswered is whether we should care about the exact
> implementation of the symmetry (xor, xor duplicate, sort fields).
> Toeplitz-based RSS is very precisely specified, so we may want to carry
> that precision into the symmetric behavior. I have a weak preference
> to do so... but no willingness to argue with you, so let me put Willem
> on the spot and have him make a decision :)

I do have a stronger willingness to argue, thanks ;-)

Can we give a more precise name, such as symmetric-xor? In case
another device would implement another mode, such as the symmetric
toeplitz of __flow_hash_consistentify, it would be good to be able to
discern the modes.
Ahmed Zaki Oct. 10, 2023, 8:10 p.m. UTC | #3
On 2023-10-07 03:01, Willem de Bruijn wrote:
> On Fri, Oct 6, 2023 at 7:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> On Fri,  6 Oct 2023 16:47:21 -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.
>>>
>>> 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
>>>
>>> or turn symmetry off (asymmetric) by:
>>>
>>>      # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>> Thanks for the changes, code looks good!
>>
>> The question left unanswered is whether we should care about the exact
>> implementation of the symmetry (xor, xor duplicate, sort fields).
>> Toeplitz-based RSS is very precisely specified, so we may want to carry
>> that precision into the symmetric behavior. I have a weak preference
>> to do so... but no willingness to argue with you, so let me put Willem
>> on the spot and have him make a decision :)
> I do have a stronger willingness to argue, thanks ;-)
>
> Can we give a more precise name, such as symmetric-xor? In case
> another device would implement another mode, such as the symmetric
> toeplitz of __flow_hash_consistentify, it would be good to be able to
> discern the modes.

I agree that implementation matters. I changed "symmetric" to 
"symmetric-xor" in v3.

Thanks for the review.
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..bf67c8094ae0 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2025,6 +2025,7 @@  static inline int ethtool_validate_duplex(__u8 duplex)
 #define	RXH_IP_DST	(1 << 5)
 #define	RXH_L4_B_0_1	(1 << 6) /* src port in case of TCP/UDP/SCTP */
 #define	RXH_L4_B_2_3	(1 << 7) /* dst port in case of TCP/UDP/SCTP */
+#define	RXH_SYMMETRIC	(1 << 30)
 #define	RXH_DISCARD	(1 << 31)
 
 #define	RX_CLS_FLOW_DISC	0xffffffffffffffffULL
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..44742653a4bd 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) &&
+	    ((info.data & ~(RXH_SYMMETRIC | 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;