[PATCH 0/3] Clean up users of skb_tx_hash and __skb_tx_hash

[PATCH 0/3] Clean up users of skb_tx_hash and __skb_tx_hash

[Alexander Duyck]

I am in the process of doing some work to try and enable macvlan Tx queue
selection without using ndo_select_queue. As a part of that I will likely
need to make changes to skb_tx_hash. As such this is a clean up or refactor
of the two spots where he function has been used. In both cases it didn't
really seem like the function was being used correctly so I have updated
both code paths to not make use of the function.

My current development environment doesn't have an mlx4 or OPA vnic
available so the changes to those have been build tested only.

---

Alexander Duyck (3):
      opa_vnic: Just use skb_get_hash instead of skb_tx_hash
      mlx4: Don't bother using skb_tx_hash in mlx4_en_select_queue
      net: Revoke export for __skb_tx_hash, update it to just be static skb_tx_hash


 drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c   |   21 ++++++++++----------
 .../infiniband/ulp/opa_vnic/opa_vnic_internal.h    |    2 +-
 drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c  |    2 +-
 drivers/net/ethernet/mellanox/mlx4/en_tx.c         |    2 +-
 include/linux/netdevice.h                          |   13 ------------
 net/core/dev.c                                     |   10 ++++------
 6 files changed, 17 insertions(+), 33 deletions(-)

[PATCH 1/3] opa_vnic: Just use skb_get_hash instead of skb_tx_hash

[Alexander Duyck]

This patch is meant to clean up how the opa_vnic is obtaining entropy from
Tx packets.

The code as it was written was claiming to get 16 bits of hash, but from
what I can tell it was only ever actually getting 14 bits as it was limited
to 0 - (2^15 - 1). It then was folding the result to get a 8 bit value for
entropy.

Instead of throwing away all that input I am cutting out the middle man and
instead having the code call skb_get_hash directly and then folding the 32
bit value into a 8 bit value using a pair of shifts and XOR operations.

Execution wise this new approach should provide more entropy and be faster
since we are bypassing the reciprocal multiplication to reduce the 32b
value to 16b and instead just using a shift/XOR combination.

In addition we can drop the unneeded adapter value from the call to get the
entropy since the netdev itself isn't even needed.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c   |   21 ++++++++++----------
 .../infiniband/ulp/opa_vnic/opa_vnic_internal.h    |    2 +-
 drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c  |    2 +-
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c b/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c
index 4be3aef..267da82 100644
--- a/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c
+++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.c
@@ -443,17 +443,16 @@ static u8 opa_vnic_get_rc(struct __opa_veswport_info *info,
 }
 
 /* opa_vnic_calc_entropy - calculate the packet entropy */
-u8 opa_vnic_calc_entropy(struct opa_vnic_adapter *adapter, struct sk_buff *skb)
+u8 opa_vnic_calc_entropy(struct sk_buff *skb)
 {
-	u16 hash16;
-
-	/*
-	 * Get flow based 16-bit hash and then XOR the upper and lower bytes
-	 * to get the entropy.
-	 * __skb_tx_hash limits qcount to 16 bits. Hence, get 15-bit hash.
-	 */
-	hash16 = __skb_tx_hash(adapter->netdev, skb, BIT(15));
-	return (u8)((hash16 >> 8) ^ (hash16 & 0xff));
+	u32 hash = skb_get_hash(skb);
+
+	/* store XOR of all bytes in lower 8 bits */
+	hash ^= hash >> 8;
+	hash ^= hash >> 16;
+
+	/* return lower 8 bits as entropy */
+	return (u8)(hash & 0xFF);
 }
 
 /* opa_vnic_get_def_port - get default port based on entropy */
@@ -490,7 +489,7 @@ void opa_vnic_encap_skb(struct opa_vnic_adapter *adapter, struct sk_buff *skb)
 
 	hdr = skb_push(skb, OPA_VNIC_HDR_LEN);
 
-	entropy = opa_vnic_calc_entropy(adapter, skb);
+	entropy = opa_vnic_calc_entropy(skb);
 	def_port = opa_vnic_get_def_port(adapter, entropy);
 	len = opa_vnic_wire_length(skb);
 	dlid = opa_vnic_get_dlid(adapter, skb, def_port);
diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_internal.h b/drivers/infiniband/ulp/opa_vnic/opa_vnic_internal.h
index afd95f4..43ac61f 100644
--- a/drivers/infiniband/ulp/opa_vnic/opa_vnic_internal.h
+++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_internal.h
@@ -299,7 +299,7 @@ struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 void opa_vnic_rem_netdev(struct opa_vnic_adapter *adapter);
 void opa_vnic_encap_skb(struct opa_vnic_adapter *adapter, struct sk_buff *skb);
 u8 opa_vnic_get_vl(struct opa_vnic_adapter *adapter, struct sk_buff *skb);
-u8 opa_vnic_calc_entropy(struct opa_vnic_adapter *adapter, struct sk_buff *skb);
+u8 opa_vnic_calc_entropy(struct sk_buff *skb);
 void opa_vnic_process_vema_config(struct opa_vnic_adapter *adapter);
 void opa_vnic_release_mac_tbl(struct opa_vnic_adapter *adapter);
 void opa_vnic_query_mac_tbl(struct opa_vnic_adapter *adapter,
diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
index ce57e0f..0c8aec6 100644
--- a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
+++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
@@ -104,7 +104,7 @@ static u16 opa_vnic_select_queue(struct net_device *netdev, struct sk_buff *skb,
 
 	/* pass entropy and vl as metadata in skb */
 	mdata = skb_push(skb, sizeof(*mdata));
-	mdata->entropy =  opa_vnic_calc_entropy(adapter, skb);
+	mdata->entropy = opa_vnic_calc_entropy(skb);
 	mdata->vl = opa_vnic_get_vl(adapter, skb);
 	rc = adapter->rn_ops->ndo_select_queue(netdev, skb,
 					       accel_priv, fallback);

[PATCH 2/3] mlx4: Don't bother using skb_tx_hash in mlx4_en_select_queue

[Alexander Duyck]

The code in the fallback path has supported XDP in conjunction with the Tx
traffic classification for TCs for over a year now. So instead of just
calling skb_tx_hash for every packet we are better off using the fallback
since that will record the Tx queue to the socket and then that can be used
instead of having to recompute the hash every time.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 6b68537..0227786 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -694,7 +694,7 @@ u16 mlx4_en_select_queue(struct net_device *dev, struct sk_buff *skb,
 	u16 rings_p_up = priv->num_tx_rings_p_up;
 
 	if (netdev_get_num_tc(dev))
-		return skb_tx_hash(dev, skb);
+		return fallback(dev, skb);
 
 	return fallback(dev, skb) % rings_p_up;
 }

RE: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in mlx4_en_select_queue

[Ruhl, Michael J]

>-----Original Message-----
>From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>owner@vger.kernel.org] On Behalf Of Alexander Duyck
>Sent: Friday, April 27, 2018 2:07 PM
>To: netdev@vger.kernel.org; davem@davemloft.net
>Cc: linux-rdma@vger.kernel.org; Dalessandro, Dennis
><dennis.dalessandro@intel.com>; Vishwanathapura, Niranjana
><niranjana.vishwanathapura@intel.com>; tariqt@mellanox.com
>Subject: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in
>mlx4_en_select_queue
>
>The code in the fallback path has supported XDP in conjunction with the Tx
>traffic classification for TCs for over a year now. So instead of just
>calling skb_tx_hash for every packet we are better off using the fallback
>since that will record the Tx queue to the socket and then that can be used
>instead of having to recompute the hash every time.
>
>Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>---
> drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>index 6b68537..0227786 100644
>--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>@@ -694,7 +694,7 @@ u16 mlx4_en_select_queue(struct net_device *dev,
>struct sk_buff *skb,
> 	u16 rings_p_up = priv->num_tx_rings_p_up;
>
> 	if (netdev_get_num_tc(dev))
>-		return skb_tx_hash(dev, skb);
>+		return fallback(dev, skb);
>
> 	return fallback(dev, skb) % rings_p_up;

Hi Alexander,

The final return fallback() call is doing a % rings_p_up.

Do you need to do that for the new fallback() call?

Maybe you can get rid of the netdev_get_num_tc() call altogether?

Thanks,

Mike

> }
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in mlx4_en_select_queue

[Alexander Duyck]

On Wed, May 2, 2018 at 11:09 AM, Ruhl, Michael J
<michael.j.ruhl@intel.com> wrote:
>>-----Original Message-----
>>From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>>owner@vger.kernel.org] On Behalf Of Alexander Duyck
>>Sent: Friday, April 27, 2018 2:07 PM
>>To: netdev@vger.kernel.org; davem@davemloft.net
>>Cc: linux-rdma@vger.kernel.org; Dalessandro, Dennis
>><dennis.dalessandro@intel.com>; Vishwanathapura, Niranjana
>><niranjana.vishwanathapura@intel.com>; tariqt@mellanox.com
>>Subject: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in
>>mlx4_en_select_queue
>>
>>The code in the fallback path has supported XDP in conjunction with the Tx
>>traffic classification for TCs for over a year now. So instead of just
>>calling skb_tx_hash for every packet we are better off using the fallback
>>since that will record the Tx queue to the socket and then that can be used
>>instead of having to recompute the hash every time.
>>
>>Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>---
>> drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>index 6b68537..0227786 100644
>>--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>@@ -694,7 +694,7 @@ u16 mlx4_en_select_queue(struct net_device *dev,
>>struct sk_buff *skb,
>>       u16 rings_p_up = priv->num_tx_rings_p_up;
>>
>>       if (netdev_get_num_tc(dev))
>>-              return skb_tx_hash(dev, skb);
>>+              return fallback(dev, skb);
>>
>>       return fallback(dev, skb) % rings_p_up;
>
> Hi Alexander,
>
> The final return fallback() call is doing a % rings_p_up.
>
> Do you need to do that for the new fallback() call?
>
> Maybe you can get rid of the netdev_get_num_tc() call altogether?
>
> Thanks,
>
> Mike

I contemplated that. The problem I suspect that piece of code is
solving is that there are cases where the number of queues the device
advertises and the number in use may not match. We have similar cases
in the ixgbe and i40e drivers for things like MACVLAN or FCoE offload
where certain queues are reserved for the offloaded traffic. For those
we are just setting up 1 TC in order to limit traffic to a given queue
set.

As time permits one thing I may look at doing is making it so that
this driver always has the number of TCs set to 1 or more. If we do
that then we could drop the modulo fallback case, likely improve the
performance as a result, and get rid of this ndo_select_queue call
entirely. I just don't currently have the time or resources to take on
the research and validation of such a change.

Thanks.

- Alex

[PATCH 3/3] net: Revoke export for __skb_tx_hash, update it to just be static skb_tx_hash

[Alexander Duyck]

I am dropping the export of __skb_tx_hash as after my patches nobody is
using it outside of the net/core/dev.c file. In addition I am renaming and
repurposing it to just be a static declaration of skb_tx_hash since that
was the only user for it at this point. By doing this the compiler can
inline it into __netdev_pick_tx as that will improve performance.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/linux/netdevice.h |   13 -------------
 net/core/dev.c            |   10 ++++------
 2 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cf44503..6da5371 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3213,19 +3213,6 @@ static inline int netif_set_xps_queue(struct net_device *dev,
 }
 #endif
 
-u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
-		  unsigned int num_tx_queues);
-
-/*
- * Returns a Tx hash for the given packet when dev->real_num_tx_queues is used
- * as a distribution range limit for the returned value.
- */
-static inline u16 skb_tx_hash(const struct net_device *dev,
-			      struct sk_buff *skb)
-{
-	return __skb_tx_hash(dev, skb, dev->real_num_tx_queues);
-}
-
 /**
  *	netif_is_multiqueue - test if device has multiple transmit queues
  *	@dev: network device
diff --git a/net/core/dev.c b/net/core/dev.c
index 9b04a9f..7584d9c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2614,17 +2614,16 @@ void netif_device_attach(struct net_device *dev)
  * Returns a Tx hash based on the given packet descriptor a Tx queues' number
  * to be used as a distribution range.
  */
-u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
-		  unsigned int num_tx_queues)
+static u16 skb_tx_hash(const struct net_device *dev, struct sk_buff *skb)
 {
 	u32 hash;
 	u16 qoffset = 0;
-	u16 qcount = num_tx_queues;
+	u16 qcount = dev->real_num_tx_queues;
 
 	if (skb_rx_queue_recorded(skb)) {
 		hash = skb_get_rx_queue(skb);
-		while (unlikely(hash >= num_tx_queues))
-			hash -= num_tx_queues;
+		while (unlikely(hash >= qcount))
+			hash -= qcount;
 		return hash;
 	}
 
@@ -2637,7 +2636,6 @@ u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
 
 	return (u16) reciprocal_scale(skb_get_hash(skb), qcount) + qoffset;
 }
-EXPORT_SYMBOL(__skb_tx_hash);
 
 static void skb_warn_bad_offload(const struct sk_buff *skb)
 {

Re: [PATCH 0/3] Clean up users of skb_tx_hash and __skb_tx_hash

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Fri, 27 Apr 2018 14:06:22 -0400

> I am in the process of doing some work to try and enable macvlan Tx queue
> selection without using ndo_select_queue. As a part of that I will likely
> need to make changes to skb_tx_hash. As such this is a clean up or refactor
> of the two spots where he function has been used. In both cases it didn't
> really seem like the function was being used correctly so I have updated
> both code paths to not make use of the function.
> 
> My current development environment doesn't have an mlx4 or OPA vnic
> available so the changes to those have been build tested only.

Looks good, series applied, thanks.