[PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Louis Peens]

From: Jaco Coetzee <jaco.coetzee@corigine.com>

Add layer 4 RSS hashing on UDP traffic to allow for the
utilization of multiple queues for multiple connections on
the same IP address.

Previously, since the introduction of the driver, RSS hashing
was only performed on the source and destination IP addresses
of UDP packets thereby limiting UDP traffic to a single queue
for multiple connections on the same IP address. The transport
layer is now included in RSS hashing for UDP traffic, which
was not previously the case. The reason behind the previous
limitation is unclear - either a historic limitation of the
NFP device, or an oversight.

Signed-off-by: Jaco Coetzee <jaco.coetzee@corigine.com>
Acked-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 62f0bf91d1e1..b7cce746b5c0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -2418,6 +2418,8 @@ static void nfp_net_rss_init(struct nfp_net *nn)
 	/* Enable IPv4/IPv6 TCP by default */
 	nn->rss_cfg = NFP_NET_CFG_RSS_IPV4_TCP |
 		      NFP_NET_CFG_RSS_IPV6_TCP |
+		      NFP_NET_CFG_RSS_IPV4_UDP |
+		      NFP_NET_CFG_RSS_IPV6_UDP |
 		      FIELD_PREP(NFP_NET_CFG_RSS_HFUNC, nn->rss_hfunc) |
 		      NFP_NET_CFG_RSS_MASK;
 }
-- 
2.34.1

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Paolo Abeni]

On Mon, 2023-05-22 at 16:13 +0200, Louis Peens wrote:
> From: Jaco Coetzee <jaco.coetzee@corigine.com>
> 
> Add layer 4 RSS hashing on UDP traffic to allow for the
> utilization of multiple queues for multiple connections on
> the same IP address.
> 
> Previously, since the introduction of the driver, RSS hashing
> was only performed on the source and destination IP addresses
> of UDP packets thereby limiting UDP traffic to a single queue
> for multiple connections on the same IP address. The transport
> layer is now included in RSS hashing for UDP traffic, which
> was not previously the case. The reason behind the previous
> limitation is unclear - either a historic limitation of the
> NFP device, or an oversight.

FTR including the transport header in RSS hash for UDP will damage
fragmented traffic, but whoever is relaying on fragments nowadays
should have already at least a dedicated setup.

So patch LGTM,

thanks,

Paolo

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Jakub Kicinski]

On Tue, 23 May 2023 12:49:06 +0200 Paolo Abeni wrote:
> > Previously, since the introduction of the driver, RSS hashing
> > was only performed on the source and destination IP addresses
> > of UDP packets thereby limiting UDP traffic to a single queue
> > for multiple connections on the same IP address. The transport
> > layer is now included in RSS hashing for UDP traffic, which
> > was not previously the case. The reason behind the previous
> > limitation is unclear - either a historic limitation of the
> > NFP device, or an oversight.  
> 
> FTR including the transport header in RSS hash for UDP will damage
> fragmented traffic, but whoever is relaying on fragments nowadays
> should have already at least a dedicated setup.

Yup, that's the exact reason it was disabled by default, FWIW.

The Microsoft spec is not crystal clear on how to handles this:
https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ndis_hash_ipv4
There is a note saying:

  If a NIC receives a packet that has both IP and TCP headers,
  NDIS_HASH_TCP_IPV4 should not always be used. In the case of a
  fragmented IP packet, NDIS_HASH_IPV4 must be used. This includes
  the first fragment which contains both IP and TCP headers.

While NDIS_HASH_UDP_IPV4 makes no such distinction and talks only about
"presence" of the header.

Maybe we should document that device is expected not to use the UDP
header if MF is set?

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 22 May 2023 16:13:35 +0200 you wrote:
> From: Jaco Coetzee <jaco.coetzee@corigine.com>
> 
> Add layer 4 RSS hashing on UDP traffic to allow for the
> utilization of multiple queues for multiple connections on
> the same IP address.
> 
> Previously, since the introduction of the driver, RSS hashing
> was only performed on the source and destination IP addresses
> of UDP packets thereby limiting UDP traffic to a single queue
> for multiple connections on the same IP address. The transport
> layer is now included in RSS hashing for UDP traffic, which
> was not previously the case. The reason behind the previous
> limitation is unclear - either a historic limitation of the
> NFP device, or an oversight.
> 
> [...]

Here is the summary with links:
  - [net-next] nfp: add L4 RSS hashing on UDP traffic
    https://git.kernel.org/netdev/net-next/c/57910a47ffe9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Simon Horman]

On Tue, May 23, 2023 at 02:20:05PM -0700, Jakub Kicinski wrote:
> On Tue, 23 May 2023 12:49:06 +0200 Paolo Abeni wrote:
> > > Previously, since the introduction of the driver, RSS hashing
> > > was only performed on the source and destination IP addresses
> > > of UDP packets thereby limiting UDP traffic to a single queue
> > > for multiple connections on the same IP address. The transport
> > > layer is now included in RSS hashing for UDP traffic, which
> > > was not previously the case. The reason behind the previous
> > > limitation is unclear - either a historic limitation of the
> > > NFP device, or an oversight.  
> > 
> > FTR including the transport header in RSS hash for UDP will damage
> > fragmented traffic, but whoever is relaying on fragments nowadays
> > should have already at least a dedicated setup.
> 
> Yup, that's the exact reason it was disabled by default, FWIW.
> 
> The Microsoft spec is not crystal clear on how to handles this:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ndis_hash_ipv4
> There is a note saying:
> 
>   If a NIC receives a packet that has both IP and TCP headers,
>   NDIS_HASH_TCP_IPV4 should not always be used. In the case of a
>   fragmented IP packet, NDIS_HASH_IPV4 must be used. This includes
>   the first fragment which contains both IP and TCP headers.
> 
> While NDIS_HASH_UDP_IPV4 makes no such distinction and talks only about
> "presence" of the header.
> 
> Maybe we should document that device is expected not to use the UDP
> header if MF is set?

Yes, maybe.

Could you suggest where such documentation should go?

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Jakub Kicinski]

On Wed, 24 May 2023 13:30:06 +0200 Simon Horman wrote:
> On Tue, May 23, 2023 at 02:20:05PM -0700, Jakub Kicinski wrote:
> > Yup, that's the exact reason it was disabled by default, FWIW.
> > 
> > The Microsoft spec is not crystal clear on how to handles this:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ndis_hash_ipv4
> > There is a note saying:
> > 
> >   If a NIC receives a packet that has both IP and TCP headers,
> >   NDIS_HASH_TCP_IPV4 should not always be used. In the case of a
> >   fragmented IP packet, NDIS_HASH_IPV4 must be used. This includes
> >   the first fragment which contains both IP and TCP headers.
> > 
> > While NDIS_HASH_UDP_IPV4 makes no such distinction and talks only about
> > "presence" of the header.
> > 
> > Maybe we should document that device is expected not to use the UDP
> > header if MF is set?  
> 
> Yes, maybe.
> 
> Could you suggest where such documentation should go?

That's the hardest question, perhaps :)

Documentation/networking/scaling.rst and/or OCP NIC spec:

https://ocp-all.groups.io/g/OCP-Networking/topic/nic_software_core_offloads/98930671?p=,,,20,0,0,0::recentpostdate/sticky,,,20,2,0,98930671,previd%3D1684255676674808204,nextid%3D1676673801962532335&previd=1684255676674808204&nextid=1676673801962532335

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Willem de Bruijn]

On Wed, May 24, 2023 at 11:22 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 24 May 2023 13:30:06 +0200 Simon Horman wrote:
> > On Tue, May 23, 2023 at 02:20:05PM -0700, Jakub Kicinski wrote:
> > > Yup, that's the exact reason it was disabled by default, FWIW.
> > >
> > > The Microsoft spec is not crystal clear on how to handles this:
> > > https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ndis_hash_ipv4
> > > There is a note saying:
> > >
> > >   If a NIC receives a packet that has both IP and TCP headers,
> > >   NDIS_HASH_TCP_IPV4 should not always be used. In the case of a
> > >   fragmented IP packet, NDIS_HASH_IPV4 must be used. This includes
> > >   the first fragment which contains both IP and TCP headers.
> > >
> > > While NDIS_HASH_UDP_IPV4 makes no such distinction and talks only about
> > > "presence" of the header.
> > >
> > > Maybe we should document that device is expected not to use the UDP
> > > header if MF is set?
> >
> > Yes, maybe.
> >
> > Could you suggest where such documentation should go?
>
> That's the hardest question, perhaps :)
>
> Documentation/networking/scaling.rst and/or OCP NIC spec:
>
> https://ocp-all.groups.io/g/OCP-Networking/topic/nic_software_core_offloads/98930671?p=,,,20,0,0,0::recentpostdate/sticky,,,20,2,0,98930671,previd%3D1684255676674808204,nextid%3D1676673801962532335&previd=1684255676674808204&nextid=1676673801962532335

The OCP draft spec already has this wording, which covers UDP:

"RSS defines two rules to derive queue selection input in a
flow-affine manner from packet headers. Selected fields of the headers
are extracted and concatenated into a byte array. If the packet is
IPv4 or IPv6, not fragmented, and followed by a transport layer
protocol with ports, such as TCP and UDP, then extract the
concatenated 4-field byte array { source address, destination address,
source port, destination port }. Else, if the packet is IPv4 or IPv6,
extract 2-field byte array { source address, destination address }.
IPv4 packets are considered fragmented if the more fragments bit is
set or the fragment offset field is non-zero."

Non google docs version:
https://www.opencompute.org/w/index.php?title=Core_Offloads

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Jakub Kicinski]

On Wed, 24 May 2023 11:33:15 -0400 Willem de Bruijn wrote:
> The OCP draft spec already has this wording, which covers UDP:
> 
> "RSS defines two rules to derive queue selection input in a
> flow-affine manner from packet headers. Selected fields of the headers
> are extracted and concatenated into a byte array. If the packet is
> IPv4 or IPv6, not fragmented, and followed by a transport layer
> protocol with ports, such as TCP and UDP, then extract the
> concatenated 4-field byte array { source address, destination address,
> source port, destination port }. Else, if the packet is IPv4 or IPv6,
> extract 2-field byte array { source address, destination address }.
> IPv4 packets are considered fragmented if the more fragments bit is
> set or the fragment offset field is non-zero."

Ugh, that's what I thought. I swear I searched it for "fragment"
yesterday and the search came up empty. I blame google docs :|

We should probably still document the recommendation that if the NIC
does not comply and hashes on ports with MF set - it should disable 
UDP hashing by default (in kernel docs).

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Paolo Abeni]

On Wed, 2023-05-24 at 08:38 -0700, Jakub Kicinski wrote:
> On Wed, 24 May 2023 11:33:15 -0400 Willem de Bruijn wrote:
> > The OCP draft spec already has this wording, which covers UDP:
> > 
> > "RSS defines two rules to derive queue selection input in a
> > flow-affine manner from packet headers. Selected fields of the headers
> > are extracted and concatenated into a byte array. If the packet is
> > IPv4 or IPv6, not fragmented, and followed by a transport layer
> > protocol with ports, such as TCP and UDP, then extract the
> > concatenated 4-field byte array { source address, destination address,
> > source port, destination port }. Else, if the packet is IPv4 or IPv6,
> > extract 2-field byte array { source address, destination address }.
> > IPv4 packets are considered fragmented if the more fragments bit is
> > set or the fragment offset field is non-zero."
> 
> Ugh, that's what I thought. I swear I searched it for "fragment"
> yesterday and the search came up empty. I blame google docs :|
> 
> We should probably still document the recommendation that if the NIC
> does not comply and hashes on ports with MF set - it should disable 
> UDP hashing by default (in kernel docs).

FTR, the above schema could still move the same flow on different
queues - if some datagrams in the given flow are fragmented and some
are not.

Out of sheer ignorance I really don't know if/how many NICs implement
RSS hashing with the above schema (using different data according to
the IP header fragments related fields). I'm guessing some (most?) use
a simpler schema (always L4 if available or never L4).

I *think* we could as well suggest always using L4 for UDP. If users
care about fragments they will have to explicitly deal with them
anyway.

Cheers,

Paolo

Re: [PATCH net-next] nfp: add L4 RSS hashing on UDP traffic

[Jakub Kicinski]

On Wed, 24 May 2023 18:14:55 +0200 Paolo Abeni wrote:
> > Ugh, that's what I thought. I swear I searched it for "fragment"
> > yesterday and the search came up empty. I blame google docs :|
> > 
> > We should probably still document the recommendation that if the NIC
> > does not comply and hashes on ports with MF set - it should disable 
> > UDP hashing by default (in kernel docs).  
> 
> FTR, the above schema could still move the same flow on different
> queues - if some datagrams in the given flow are fragmented and some
> are not.

Ah, you're right.

> Out of sheer ignorance I really don't know if/how many NICs implement
> RSS hashing with the above schema (using different data according to
> the IP header fragments related fields). I'm guessing some (most?) use
> a simpler schema (always L4 if available or never L4).
> 
> I *think* we could as well suggest always using L4 for UDP. If users
> care about fragments they will have to explicitly deal with them
> anyway.

Makes sense. QUIC changed the math on how likely UDP fragmentation is.