[PATCH 2/2] rps: changes to bnx2x to get device hash

[PATCH 2/2] rps: changes to bnx2x to get device hash

[Tom Herbert]

bnx2x changes to get Toeplitz hash on RX and out into skb.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 drivers/net/bnx2x.h      |    2 +-
 drivers/net/bnx2x_main.c |   47 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
index c3b32f7..a876d78 100644
--- a/drivers/net/bnx2x.h
+++ b/drivers/net/bnx2x.h
@@ -1295,7 +1295,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32
reg, u32 expected, int ms,
 				 AEU_INPUTS_ATTN_BITS_MISC_PARITY_ERROR)


-#define MULTI_FLAGS(bp) \
+#define RSS_FLAGS(bp) \
 		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
 		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
 		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 59b58d8..1729c04 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -83,6 +83,10 @@ module_param(multi_mode, int, 0);
 MODULE_PARM_DESC(multi_mode, " Multi queue mode "
 			     "(0 Disable; 1 Enable (default))");

+static int get_hdrhash = 1;
+module_param(get_hdrhash, int, 0);
+MODULE_PARM_DESC(get_hdrhash, " Get Toeplitz hash from device");
+
 static int num_rx_queues;
 module_param(num_rx_queues, int, 0);
 MODULE_PARM_DESC(num_rx_queues, " Number of Rx queues for multi_mode=1"
@@ -1520,7 +1524,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath
*fp, int budget)
 		struct sw_rx_bd *rx_buf = NULL;
 		struct sk_buff *skb;
 		union eth_rx_cqe *cqe;
-		u8 cqe_fp_flags;
+		u8 cqe_fp_flags, cqe_fp_status_flags, cqe_fp_pars_flags;
 		u16 len, pad;

 		comp_ring_cons = RCQ_BD(sw_comp_cons);
@@ -1536,6 +1540,8 @@ static int bnx2x_rx_int(struct bnx2x_fastpath
*fp, int budget)

 		cqe = &fp->rx_comp_ring[comp_ring_cons];
 		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
+		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
+		cqe_fp_pars_flags = cqe->fast_path_cqe.pars_flags.flags;

 		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
 		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
@@ -1661,7 +1667,32 @@ reuse_rx:
 				goto next_rx;
 			}

-			skb->protocol = eth_type_trans(skb, bp->dev);
+
+			if (get_hdrhash && (cqe_fp_status_flags &
+			    ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG)) {
+				u8 hash_type = cqe_fp_status_flags &
+					ETH_FAST_PATH_RX_CQE_RSS_HASH_TYPE;
+
+				skb->rxhash = le32_to_cpu(
+				      cqe->fast_path_cqe.rss_hash_result);
+				if (!skb->rxhash)
+					skb->rxhash = 1;
+
+				/* unicast IPv4 packet? */
+				if (((hash_type == IPV4_HASH_TYPE) ||
+				     (hash_type == TCP_IPV4_HASH_TYPE)) &&
+				    (cqe_fp_pars_flags &
+					PARSING_FLAGS_ETHERNET_ADDRESS_TYPE)) {
+					skb->dev = bp->dev;
+					skb_reset_mac_header(skb);
+					skb_pull(skb, ETH_HLEN);
+					skb->protocol =
+					    __constant_htons(ETH_P_IP);
+				} else
+					skb->protocol =
+					    eth_type_trans(skb, bp->dev);
+			} else
+				skb->protocol = eth_type_trans(skb, bp->dev);

 			skb->ip_summed = CHECKSUM_NONE;
 			if (bp->rx_csum) {
@@ -5388,8 +5419,11 @@ static void bnx2x_init_internal_func(struct bnx2x *bp)
 	u16 max_agg_size;

 	if (is_multi(bp)) {
-		tstorm_config.config_flags = MULTI_FLAGS(bp);
+		tstorm_config.config_flags = RSS_FLAGS(bp);
 		tstorm_config.rss_result_mask = MULTI_MASK;
+	} else if (get_hdrhash) {
+		/* Set flags so the Toeplitz hash is provided */
+		tstorm_config.config_flags = RSS_FLAGS(bp);
 	}

 	/* Enable TPA if needed */
@@ -6223,9 +6257,10 @@ static int bnx2x_init_common(struct bnx2x *bp)
 	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);

 	REG_WR(bp, SRC_REG_SOFT_RST, 1);
-	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
-		REG_WR(bp, i, 0xc0cac01a);
-		/* TODO: replace with something meaningful */
+	{
+		int i;
+		for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
+			REG_WR(bp, i, random32());
 	}
 	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
 #ifdef BCM_CNIC
-- 
1.5.4.3

Re: [PATCH 2/2] rps: changes to bnx2x to get device hash

[Stephen Hemminger]

On Tue, 10 Nov 2009 22:53:36 -0800
Tom Herbert <therbert@google.com> wrote:

> +
> +			if (get_hdrhash && (cqe_fp_status_flags &
> +			    ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG)) {
> +				u8 hash_type = cqe_fp_status_flags &
> +					ETH_FAST_PATH_RX_CQE_RSS_HASH_TYPE;
> +
> +				skb->rxhash = le32_to_cpu(
> +				      cqe->fast_path_cqe.rss_hash_result);
> +				if (!skb->rxhash)
> +					skb->rxhash = 1;
> +
> +				/* unicast IPv4 packet? */
> +				if (((hash_type == IPV4_HASH_TYPE) ||
> +				     (hash_type == TCP_IPV4_HASH_TYPE)) &&
> +				    (cqe_fp_pars_flags &
> +					PARSING_FLAGS_ETHERNET_ADDRESS_TYPE)) {
> +					skb->dev = bp->dev;
> +					skb_reset_mac_header(skb);
> +					skb_pull(skb, ETH_HLEN);
> +					skb->protocol =
> +					    __constant_htons(ETH_P_IP);
> +				} else
> +					skb->protocol =
> +					    eth_type_trans(skb, bp->dev);
> +			} else
> +				skb->protocol = eth_type_trans(skb, bp->dev);

How about putting this all in an inline function:

static inline u16 bn2x_get_type(...) {
}


	skb->protocol = bn2x_get_type(...);

Then you won't have two calls to eth_type_trans,

Also: __constant_htons() should not be used directly (except switch statements),
macro for htons() does it automatically if argument is constant.

Re: [PATCH 2/2] rps: changes to bnx2x to get device hash

[Matt Carlson]

On Tue, Nov 10, 2009 at 10:53:36PM -0800, Tom Herbert wrote:
> @@ -6223,9 +6257,10 @@ static int bnx2x_init_common(struct bnx2x *bp)
>  	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
> 
>  	REG_WR(bp, SRC_REG_SOFT_RST, 1);
> -	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> -		REG_WR(bp, i, 0xc0cac01a);
> -		/* TODO: replace with something meaningful */
> +	{
> +		int i;
> +		for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> +			REG_WR(bp, i, random32());
>  	}
>  	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
>  #ifdef BCM_CNIC

Is a random hash key really better than arbitrary static values?  
Setting the hash key to random values means, from chip reset to chip
reset, you could get different performance results.  Unless we
we understood how the key affects performance, I would think that
reproducable performance numbers would be more desirable, no?

Re: [PATCH 2/2] rps: changes to bnx2x to get device hash

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Wed, 11 Nov 2009 15:21:03 -0800

> Is a random hash key really better than arbitrary static values?  
> Setting the hash key to random values means, from chip reset to chip
> reset, you could get different performance results.  Unless we
> we understood how the key affects performance, I would think that
> reproducable performance numbers would be more desirable, no?

Security.

If someone knows the key and the hash function, they can purposely
pick address and port combinations that always hash to the same queue,
decresing your horsepower to handle traffic by orders of magnitude.