[PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[Edward Cree]

EF10 based NICs have configurable RSS hash fields, and can be made to take the
ports into the hash on UDP (they already do so for TCP).  This patch series
enables this, in order to improve spreading of UDP traffic.

Edward Cree (2):
  sfc: enable 4-tuple RSS hashing for UDP
  sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

 drivers/net/ethernet/sfc/ef10.c       | 84 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/ethtool.c    | 12 +++--
 drivers/net/ethernet/sfc/net_driver.h |  2 +
 3 files changed, 94 insertions(+), 4 deletions(-)

[PATCH net-next 1/2] sfc: enable 4-tuple RSS hashing for UDP

[Edward Cree]

This improves UDP spreading, and also slightly improves GRO performance
of encapsulated TCP on 7000 series NICs.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/ef10.c | 82 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 00279da..9f6d769 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -2245,6 +2245,84 @@ static void efx_ef10_tx_write(struct efx_tx_queue *tx_queue)
 	}
 }
 
+#define RSS_MODE_HASH_ADDRS	(1 << RSS_MODE_HASH_SRC_ADDR_LBN |\
+				 1 << RSS_MODE_HASH_DST_ADDR_LBN)
+#define RSS_MODE_HASH_PORTS	(1 << RSS_MODE_HASH_SRC_PORT_LBN |\
+				 1 << RSS_MODE_HASH_DST_PORT_LBN)
+#define RSS_CONTEXT_FLAGS_DEFAULT	(1 << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TOEPLITZ_IPV4_EN_LBN |\
+					 1 << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TOEPLITZ_TCPV4_EN_LBN |\
+					 1 << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TOEPLITZ_IPV6_EN_LBN |\
+					 1 << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TOEPLITZ_TCPV6_EN_LBN |\
+					 (RSS_MODE_HASH_ADDRS | RSS_MODE_HASH_PORTS) << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TCP_IPV4_RSS_MODE_LBN |\
+					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV4_RSS_MODE_LBN |\
+					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_OTHER_IPV4_RSS_MODE_LBN |\
+					 (RSS_MODE_HASH_ADDRS | RSS_MODE_HASH_PORTS) << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_TCP_IPV6_RSS_MODE_LBN |\
+					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN |\
+					 RSS_MODE_HASH_ADDRS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_OTHER_IPV6_RSS_MODE_LBN)
+
+static int efx_ef10_get_rss_flags(struct efx_nic *efx, u32 context, u32 *flags)
+{
+	/* Firmware had a bug (sfc bug 61952) where it would not actually
+	 * fill in the flags field in the response to MC_CMD_RSS_CONTEXT_GET_FLAGS.
+	 * This meant that it would always contain whatever was previously
+	 * in the MCDI buffer.  Fortunately, all firmware versions with
+	 * this bug have the same default flags value for a newly-allocated
+	 * RSS context, and the only time we want to get the flags is just
+	 * after allocating.  Moreover, the response has a 32-bit hole
+	 * where the context ID would be in the request, so we can use an
+	 * overlength buffer in the request and pre-fill the flags field
+	 * with what we believe the default to be.  Thus if the firmware
+	 * has the bug, it will leave our pre-filled value in the flags
+	 * field of the response, and we will get the right answer.
+	 *
+	 * However, this does mean that this function should NOT be used if
+	 * the RSS context flags might not be their defaults - it is ONLY
+	 * reliably correct for a newly-allocated RSS context.
+	 */
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_LEN);
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_LEN);
+	size_t outlen;
+	int rc;
+
+	/* Check we have a hole for the context ID */
+	BUILD_BUG_ON(MC_CMD_RSS_CONTEXT_GET_FLAGS_IN_LEN != MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_FLAGS_OFST);
+	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_FLAGS_IN_RSS_CONTEXT_ID, context);
+	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_GET_FLAGS_OUT_FLAGS,
+		       RSS_CONTEXT_FLAGS_DEFAULT);
+	rc = efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_GET_FLAGS, inbuf,
+			  sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
+	if (rc == 0) {
+		if (outlen < MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_LEN)
+			rc = -EIO;
+		else
+			*flags = MCDI_DWORD(outbuf, RSS_CONTEXT_GET_FLAGS_OUT_FLAGS);
+	}
+	return rc;
+}
+
+/* Attempt to enable 4-tuple UDP hashing on the specified RSS context.
+ * If we fail, we just leave the RSS context at its default hash settings,
+ * which is safe but may slightly reduce performance.
+ * Defaults are 4-tuple for TCP and 2-tuple for UDP and other-IP, so we
+ * just need to set the UDP ports flags (for both IP versions).
+ */
+static void efx_ef10_set_rss_flags(struct efx_nic *efx, u32 context)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_RSS_CONTEXT_SET_FLAGS_IN_LEN);
+	u32 flags;
+
+	BUILD_BUG_ON(MC_CMD_RSS_CONTEXT_SET_FLAGS_OUT_LEN != 0);
+
+	if (efx_ef10_get_rss_flags(efx, context, &flags) != 0)
+		return;
+	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_SET_FLAGS_IN_RSS_CONTEXT_ID, context);
+	flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV4_RSS_MODE_LBN;
+	flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN;
+	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_SET_FLAGS_IN_FLAGS, flags);
+	efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_SET_FLAGS, inbuf, sizeof(inbuf),
+		     NULL, 0, NULL);
+}
+
 static int efx_ef10_alloc_rss_context(struct efx_nic *efx, u32 *context,
 				      bool exclusive, unsigned *context_size)
 {
@@ -2290,6 +2368,10 @@ static int efx_ef10_alloc_rss_context(struct efx_nic *efx, u32 *context,
 	if (context_size)
 		*context_size = rss_spread;
 
+	if (nic_data->datapath_caps &
+	    1 << MC_CMD_GET_CAPABILITIES_OUT_ADDITIONAL_RSS_MODES_LBN)
+		efx_ef10_set_rss_flags(efx, *context);
+
 	return 0;
 }
 

[PATCH net-next 2/2] sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/ef10.c       |  6 ++++--
 drivers/net/ethernet/sfc/ethtool.c    | 12 ++++++++----
 drivers/net/ethernet/sfc/net_driver.h |  2 ++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 9f6d769..e61807e 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -2319,8 +2319,10 @@ static void efx_ef10_set_rss_flags(struct efx_nic *efx, u32 context)
 	flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV4_RSS_MODE_LBN;
 	flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN;
 	MCDI_SET_DWORD(inbuf, RSS_CONTEXT_SET_FLAGS_IN_FLAGS, flags);
-	efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_SET_FLAGS, inbuf, sizeof(inbuf),
-		     NULL, 0, NULL);
+	if (!efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_SET_FLAGS, inbuf, sizeof(inbuf),
+			  NULL, 0, NULL))
+		/* Succeeded, so UDP 4-tuple is now enabled */
+		efx->rx_hash_udp_4tuple = true;
 }
 
 static int efx_ef10_alloc_rss_context(struct efx_nic *efx, u32 *context,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 445ccdb..bf126f9 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -968,20 +968,24 @@ efx_ethtool_get_rxnfc(struct net_device *net_dev,
 
 		info->data = 0;
 		switch (info->flow_type) {
+		case UDP_V4_FLOW:
+			if (efx->rx_hash_udp_4tuple)
+				/* fall through */
 		case TCP_V4_FLOW:
-			info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3;
+				info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3;
 			/* fall through */
-		case UDP_V4_FLOW:
 		case SCTP_V4_FLOW:
 		case AH_ESP_V4_FLOW:
 		case IPV4_FLOW:
 			info->data |= RXH_IP_SRC | RXH_IP_DST;
 			min_revision = EFX_REV_FALCON_B0;
 			break;
+		case UDP_V6_FLOW:
+			if (efx->rx_hash_udp_4tuple)
+				/* fall through */
 		case TCP_V6_FLOW:
-			info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3;
+				info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3;
 			/* fall through */
-		case UDP_V6_FLOW:
 		case SCTP_V6_FLOW:
 		case AH_ESP_V6_FLOW:
 		case IPV6_FLOW:
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 99d8c82..fec51c4 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -853,6 +853,7 @@ struct vfdi_status;
  * @rx_hash_key: Toeplitz hash key for RSS
  * @rx_indir_table: Indirection table for RSS
  * @rx_scatter: Scatter mode enabled for receives
+ * @rx_hash_udp_4tuple: UDP 4-tuple hashing enabled
  * @int_error_count: Number of internal errors seen recently
  * @int_error_expire: Time at which error count will be expired
  * @irq_soft_enabled: Are IRQs soft-enabled? If not, IRQ handler will
@@ -990,6 +991,7 @@ struct efx_nic {
 	u8 rx_hash_key[40];
 	u32 rx_indir_table[128];
 	bool rx_scatter;
+	bool rx_hash_udp_4tuple;
 
 	unsigned int_error_count;
 	unsigned long int_error_expire;

Re: [PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Thu, 3 Nov 2016 22:10:31 +0000

> EF10 based NICs have configurable RSS hash fields, and can be made to take the
> ports into the hash on UDP (they already do so for TCP).  This patch series
> enables this, in order to improve spreading of UDP traffic.

What does the chip do with fragmented traffic?

Re: [PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[Edward Cree]

On 07/11/16 18:20, David Miller wrote:
> From: Edward Cree <ecree@solarflare.com>
> Date: Thu, 3 Nov 2016 22:10:31 +0000
>
>> EF10 based NICs have configurable RSS hash fields, and can be made to take the
>> ports into the hash on UDP (they already do so for TCP).  This patch series
>> enables this, in order to improve spreading of UDP traffic.
> What does the chip do with fragmented traffic?
Only the first fragment will be considered UDP, it will treat the rest as "other
IP" and 2-tuple hash them, probably hitting a different queue.

My understanding is that while that will reduce performance, that shouldn't be a
problem as performance-sensitive users will avoid fragmentation anyway.
It could also lead to out-of-order packet delivery, but it's UDP so that's
supposed to be OK.

Re: [PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Tue, 8 Nov 2016 13:02:05 +0000

> On 07/11/16 18:20, David Miller wrote:
>> From: Edward Cree <ecree@solarflare.com>
>> Date: Thu, 3 Nov 2016 22:10:31 +0000
>>
>>> EF10 based NICs have configurable RSS hash fields, and can be made to take the
>>> ports into the hash on UDP (they already do so for TCP).  This patch series
>>> enables this, in order to improve spreading of UDP traffic.
>> What does the chip do with fragmented traffic?
> Only the first fragment will be considered UDP, it will treat the rest as "other
> IP" and 2-tuple hash them, probably hitting a different queue.
> 
> My understanding is that while that will reduce performance, that shouldn't be a
> problem as performance-sensitive users will avoid fragmentation anyway.
> It could also lead to out-of-order packet delivery, but it's UDP so that's
> supposed to be OK.

Our software hashing never tries to inspect the ports for fragmented
frames.  And I'm pretty sure this is intentional.

We should minimize the difference between what we do in software, which
we fully control, and what we ask the hardware to offload for us.

If you can't configure the chip to skip the ports for fragmented frames
than I'm going to ask you to drop this.

Re: [PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[Edward Cree]

On 09/11/16 18:09, David Miller wrote:
> From: Edward Cree <ecree@solarflare.com>
> Date: Tue, 8 Nov 2016 13:02:05 +0000
>
>> On 07/11/16 18:20, David Miller wrote:
>>> From: Edward Cree <ecree@solarflare.com>
>>> Date: Thu, 3 Nov 2016 22:10:31 +0000
>>>
>>>> EF10 based NICs have configurable RSS hash fields, and can be made to take the
>>>> ports into the hash on UDP (they already do so for TCP).  This patch series
>>>> enables this, in order to improve spreading of UDP traffic.
>>> What does the chip do with fragmented traffic?
>> Only the first fragment will be considered UDP, it will treat the rest as "other
>> IP" and 2-tuple hash them, probably hitting a different queue.
>>
>> My understanding is that while that will reduce performance, that shouldn't be a
>> problem as performance-sensitive users will avoid fragmentation anyway.
>> It could also lead to out-of-order packet delivery, but it's UDP so that's
>> supposed to be OK.
> Our software hashing never tries to inspect the ports for fragmented
> frames.  And I'm pretty sure this is intentional.
>
> We should minimize the difference between what we do in software, which
> we fully control, and what we ask the hardware to offload for us.
>
> If you can't configure the chip to skip the ports for fragmented frames
> than I'm going to ask you to drop this.
I just checked and it turns out I was mistaken, we don't treat the first fragment
differently after all, we skip the ports for all fragments including the first.
Sorry for the misinformation.

Re: [PATCH net-next 0/2] sfc: enable 4-tuple UDP RSS hashing

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Wed, 9 Nov 2016 18:51:15 +0000

> On 09/11/16 18:09, David Miller wrote:
>> From: Edward Cree <ecree@solarflare.com>
>> Date: Tue, 8 Nov 2016 13:02:05 +0000
>>
>>> On 07/11/16 18:20, David Miller wrote:
>>>> From: Edward Cree <ecree@solarflare.com>
>>>> Date: Thu, 3 Nov 2016 22:10:31 +0000
>>>>
>>>>> EF10 based NICs have configurable RSS hash fields, and can be made to take the
>>>>> ports into the hash on UDP (they already do so for TCP).  This patch series
>>>>> enables this, in order to improve spreading of UDP traffic.
>>>> What does the chip do with fragmented traffic?
>>> Only the first fragment will be considered UDP, it will treat the rest as "other
>>> IP" and 2-tuple hash them, probably hitting a different queue.
>>>
>>> My understanding is that while that will reduce performance, that shouldn't be a
>>> problem as performance-sensitive users will avoid fragmentation anyway.
>>> It could also lead to out-of-order packet delivery, but it's UDP so that's
>>> supposed to be OK.
>> Our software hashing never tries to inspect the ports for fragmented
>> frames.  And I'm pretty sure this is intentional.
>>
>> We should minimize the difference between what we do in software, which
>> we fully control, and what we ask the hardware to offload for us.
>>
>> If you can't configure the chip to skip the ports for fragmented frames
>> than I'm going to ask you to drop this.
> I just checked and it turns out I was mistaken, we don't treat the first fragment
> differently after all, we skip the ports for all fragments including the first.
> Sorry for the misinformation.

That's more in line with what is expected, series applied, thanks.