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

[RFC 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.
Patch 2/2, which updates the ETHTOOL_GRXFH handling to report the new RSS hash
settings, uses a somewhat unorthodox construct to control fall-through flow in
a switch statement - is this acceptable style?

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    | 14 ++++--
 drivers/net/ethernet/sfc/net_driver.h |  2 +
 3 files changed, 96 insertions(+), 4 deletions(-)

[RFC 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;
 }
 

[RFC 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    | 14 ++++++++++----
 drivers/net/ethernet/sfc/net_driver.h |  2 ++
 3 files changed, 16 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..4a58899 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -968,20 +968,26 @@ 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 */
+			/* else fall further! */
 		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 */
+			/* else fall further! */
 		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: [RFC PATCH net-next 2/2] sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

[Mintz, Yuval]

>                  info->data = 0;
>                  switch (info->flow_type) {
> +               case UDP_V4_FLOW:
> +                       if (efx->rx_hash_udp_4tuple)
> +                               /* fall through */
> +                       /* else fall further! */
>                  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;

Well, you sure fulfilled your cover letter's promise. ;-)

Do you really prefer this conditional mayham over copy-pasting some lines?

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

[David Laight]

From: Edward Cree
> Sent: 27 September 2016 17:36
...
> +		case UDP_V4_FLOW:
> +			if (efx->rx_hash_udp_4tuple)
> +				/* fall through */
> +			/* else fall further! */

If you invert the above and add a goto...
			if (!efx->rx_hash_udp_4tuple)
				goto set_ip;

>  		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:
    set_ip:
>  			info->data |= RXH_IP_SRC | RXH_IP_DST;
>  			min_revision = EFX_REV_FALCON_B0;
>  			break;

It might look better.

	David

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

[Edward Cree]

On 28/09/16 10:12, David Laight wrote:
> If you invert the above and add a goto...
> 			if (!efx->rx_hash_udp_4tuple)
> 				goto set_ip;
I don't mind gotos...
>>  		case SCTP_V4_FLOW:
>>  		case AH_ESP_V4_FLOW:
>>  		case IPV4_FLOW:
>     set_ip:
...but this adds a label where we effectively already have one.
I wish C allowed goto case labels.
> It might look better.
> 	David

It just bugs me that it would have this unnecessary goto and label.

Alternate ways to maybe make it look better, or not:

* Remove the /* else fall further */ comment, does this make the
  indentation more or less confusing?
* Include braces on the if, even though there's only one statement
  inside.

Also, how strong are people's reaction to this?  If it's just "I
personally wouldn't do it that way", then I'm tempted to go ahead
anyway.  But if it's "NAK NAK NAK burn the heretic", that's
another matter.

-Ed