diff mbox

[net-next,2/8] bnx2x: add select queue callback

Message ID 1291896556.8745.7.camel@lb-tlvb-dmitry
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Dmitry Kravkov Dec. 9, 2010, 12:09 p.m. UTC 
The callback required to allow FCoE traffic to be
sent on separate priority queue from other L2 traffic,
which is managed by PFC in HW.

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/bnx2x/bnx2x_cmn.c  |   31 +++++++++++++++++++++++++++++++
 drivers/net/bnx2x/bnx2x_cmn.h  |    3 +++
 drivers/net/bnx2x/bnx2x_main.c |    1 +
 3 files changed, 35 insertions(+), 0 deletions(-)

Comments

David Miller Dec. 10, 2010, 10:11 p.m. UTC | #1 
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Thu, 9 Dec 2010 14:09:16 +0200

> +#endif
> +	/* Select queue (if defined) adjust for fcoe */
> +	fp_index = skb_tx_hash(dev, skb) - FCOE_CONTEXT_USE;
> +
> +	return (fp_index >= 0 ? fp_index : 0);

This doesn't make any sense, and it's one of the reasons I really
hate the fact that drivers sometimes need to override the
select_queue method.

Because 9 times out of 10 they get it wrong.

You're mapping queues 0 --> FCOE_CONTEXT_USE to zero.

But that's not what you actually want.

You're actually trying to make non-FCOE traffic hash only amongst the
non-FCOE queues.

So make your code actually do that instead of screwing up the
distribution of the hash result.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eilon Greenstein Dec. 12, 2010, 10:25 a.m. UTC | #2 
On Fri, 2010-12-10 at 14:11 -0800, David Miller wrote:
> From: "Dmitry Kravkov" <dmitry@broadcom.com>
> Date: Thu, 9 Dec 2010 14:09:16 +0200
> 
> > +#endif
> > +	/* Select queue (if defined) adjust for fcoe */
> > +	fp_index = skb_tx_hash(dev, skb) - FCOE_CONTEXT_USE;
> > +
> > +	return (fp_index >= 0 ? fp_index : 0);
> 
> This doesn't make any sense, and it's one of the reasons I really
> hate the fact that drivers sometimes need to override the
> select_queue method.
> 
> Because 9 times out of 10 they get it wrong.
> 
> You're mapping queues 0 --> FCOE_CONTEXT_USE to zero.
> 
> But that's not what you actually want.
> 
> You're actually trying to make non-FCOE traffic hash only amongst the
> non-FCOE queues.
> 
> So make your code actually do that instead of screwing up the
> distribution of the hash result.
> 

Indeed, the current suggestion is giving queue 0 double weight compared
to all other queues just so the skb_tx_hash will be used as is to
benefit from future enhancement changes to it. We will re-spin this
patch and move the content of the skb_tx_hash to __skb_tx_hash that will
receive the netdev->skb_real_num_tx_queues as a parameter - this way, we
can call the __skb_tx_hash with a lower number.

Thanks,
Eilon




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

Patch

diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index a553fba..862b29e 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -1167,6 +1167,37 @@  void bnx2x_netif_stop(struct bnx2x *bp, int disable_hw)
 	netif_tx_disable(bp->dev);
 }
 
+u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb)
+{
+	int fp_index = 0;
+
+#ifdef BCM_CNIC
+	struct bnx2x *bp = netdev_priv(dev);
+	if (NO_FCOE(bp))
+		return skb_tx_hash(dev, skb);
+	else {
+		struct ethhdr *hdr = (struct ethhdr *)skb->data;
+		u16 ether_type = ntohs(hdr->h_proto);
+
+		/* Skip VLAN tag if present */
+		if (ether_type == ETH_P_8021Q) {
+			struct vlan_ethhdr *vhdr =
+				(struct vlan_ethhdr *)skb->data;
+
+			ether_type = ntohs(vhdr->h_vlan_encapsulated_proto);
+		}
+
+		/* If ethertype is FCoE or FIP - use FCoE ring */
+		if ((ether_type == ETH_P_FCOE) || (ether_type == ETH_P_FIP))
+			return bnx2x_fcoe(bp, index);
+	}
+#endif
+	/* Select queue (if defined) adjust for fcoe */
+	fp_index = skb_tx_hash(dev, skb) - FCOE_CONTEXT_USE;
+
+	return (fp_index >= 0 ? fp_index : 0);
+}
+
 void bnx2x_set_num_queues(struct bnx2x *bp)
 {
 	switch (bp->multi_mode) {
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 4bb0113..258f0c0 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -343,6 +343,9 @@  int bnx2x_nic_load(struct bnx2x *bp, int load_mode);
 /* hard_xmit callback */
 netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev);
 
+/* select_queue callback */
+u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb);
+
 int bnx2x_change_mac_addr(struct net_device *dev, void *p);
 
 /* NAPI poll Rx part */
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index 6c70ee5..c4dfb42 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -8977,6 +8977,7 @@  static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_open		= bnx2x_open,
 	.ndo_stop		= bnx2x_close,
 	.ndo_start_xmit		= bnx2x_start_xmit,
+	.ndo_select_queue	= bnx2x_select_queue,
 	.ndo_set_multicast_list	= bnx2x_set_rx_mode,
 	.ndo_set_mac_address	= bnx2x_change_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,