| Message ID | 1307129073-3769-1-git-send-email-nhorman@tuxdriver.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Le vendredi 03 juin 2011 à 15:24 -0400, Neil Horman a écrit : > The bonding driver is multiqueue enabled, in which each queue represents a slave > to enable optional steering of output frames to given slaves against the default > output policy. However, it needs to reset the skb->queue_mapping prior to > queuing to the physical device or the physical slave (if it is multiqueue) could > wind up transmitting on an unintended tx queue > > Change Notes: > v2) Based on first pass review, updated the patch to restore the origional queue > mapping that was found in bond_select_queue, rather than simply resetting to > zero. This preserves the value of queue_mapping when it was set on receive in > the forwarding case which is desireable. > > v3) Fixed spelling an casting error in skb->cb > > v4) fixed to store raw queue_mapping to avoid double decrement > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > CC: Jay Vosburgh <fubar@us.ibm.com> > CC: Andy Gospodarek <andy@greyhouse.net> > CC: "David S. Miller" <davem@davemloft.net> > --- > drivers/net/bonding/bond_main.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 17b4dd9..76adf27 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -400,6 +400,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, > { > skb->dev = slave_dev; > skb->priority = 1; > + > + skb->queue_mapping = ((u16 *)skb->cb)[0]; Please dont do that. Use a helper. For example : #define bond_queue_mapping(skb) (*(unsigned int *)((skb)->cb)) skb->queue_mapping = bond_queue_mapping(skb); > + > if (unlikely(netpoll_tx_running(slave_dev))) > bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); > else > @@ -4216,6 +4219,11 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb) > */ > u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; > > + /* > + * Save the original txq to restore before passing to the driver > + */ > + ((u16 *)skb->cb)[0] = skb->queue_mapping; bond_queue_mapping(skb) = skb->queue_mapping; > + > if (unlikely(txq >= dev->real_num_tx_queues)) { > do { > txq -= dev->real_num_tx_queues; -- 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
On Fri, Jun 03, 2011 at 09:48:51PM +0200, Eric Dumazet wrote: > Le vendredi 03 juin 2011 à 15:24 -0400, Neil Horman a écrit : > > The bonding driver is multiqueue enabled, in which each queue represents a slave > > to enable optional steering of output frames to given slaves against the default > > output policy. However, it needs to reset the skb->queue_mapping prior to > > queuing to the physical device or the physical slave (if it is multiqueue) could > > wind up transmitting on an unintended tx queue > > > > Change Notes: > > v2) Based on first pass review, updated the patch to restore the origional queue > > mapping that was found in bond_select_queue, rather than simply resetting to > > zero. This preserves the value of queue_mapping when it was set on receive in > > the forwarding case which is desireable. > > > > v3) Fixed spelling an casting error in skb->cb > > > > v4) fixed to store raw queue_mapping to avoid double decrement > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > CC: Jay Vosburgh <fubar@us.ibm.com> > > CC: Andy Gospodarek <andy@greyhouse.net> > > CC: "David S. Miller" <davem@davemloft.net> > > --- > > drivers/net/bonding/bond_main.c | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 17b4dd9..76adf27 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -400,6 +400,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, > > { > > skb->dev = slave_dev; > > skb->priority = 1; > > + > > + skb->queue_mapping = ((u16 *)skb->cb)[0]; > > Please dont do that. Use a helper. > Why? It seems to be reasonably common practice for drivers to access queue_mapping directly. Examples can be found in: ixgbe_xmit_frame mlx4_en_xmit qlge_send igb_xmit_frame_adv gfar_start_xmit among others. Not saying its correct to do so necessecarily, but it seems a helper doesn't buy us much here, particularly a per-driver helper. If a helper really should be used, why not just consistently use skb_get_queue_mapping? Neil -- 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
Le vendredi 03 juin 2011 à 15:57 -0400, Neil Horman a écrit : > On Fri, Jun 03, 2011 at 09:48:51PM +0200, Eric Dumazet wrote: > > Le vendredi 03 juin 2011 à 15:24 -0400, Neil Horman a écrit : > > > The bonding driver is multiqueue enabled, in which each queue represents a slave > > > to enable optional steering of output frames to given slaves against the default > > > output policy. However, it needs to reset the skb->queue_mapping prior to > > > queuing to the physical device or the physical slave (if it is multiqueue) could > > > wind up transmitting on an unintended tx queue > > > > > > Change Notes: > > > v2) Based on first pass review, updated the patch to restore the origional queue > > > mapping that was found in bond_select_queue, rather than simply resetting to > > > zero. This preserves the value of queue_mapping when it was set on receive in > > > the forwarding case which is desireable. > > > > > > v3) Fixed spelling an casting error in skb->cb > > > > > > v4) fixed to store raw queue_mapping to avoid double decrement > > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > CC: Jay Vosburgh <fubar@us.ibm.com> > > > CC: Andy Gospodarek <andy@greyhouse.net> > > > CC: "David S. Miller" <davem@davemloft.net> > > > --- > > > drivers/net/bonding/bond_main.c | 8 ++++++++ > > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > > index 17b4dd9..76adf27 100644 > > > --- a/drivers/net/bonding/bond_main.c > > > +++ b/drivers/net/bonding/bond_main.c > > > @@ -400,6 +400,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, > > > { > > > skb->dev = slave_dev; > > > skb->priority = 1; > > > + > > > + skb->queue_mapping = ((u16 *)skb->cb)[0]; > > > > Please dont do that. Use a helper. > > > Why? It seems to be reasonably common practice for drivers to access > queue_mapping directly. > > Examples can be found in: > ixgbe_xmit_frame > mlx4_en_xmit > qlge_send > igb_xmit_frame_adv > gfar_start_xmit > > among others. > > Not saying its correct to do so necessecarily, but it seems a helper doesn't buy > us much here, particularly a per-driver helper. If a helper really should be > used, why not just consistently use skb_get_queue_mapping? > Neil I was speaking of skb->cb access, of course, sorry if you missed my point ;) Please take a look at various files using helpers for this. For example : net/ipv4/igmp.c #define igmp_skb_size(skb) (*(unsigned int *)((skb)->cb)) -- 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 --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 17b4dd9..76adf27 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -400,6 +400,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, { skb->dev = slave_dev; skb->priority = 1; + + skb->queue_mapping = ((u16 *)skb->cb)[0]; + if (unlikely(netpoll_tx_running(slave_dev))) bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); else @@ -4216,6 +4219,11 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb) */ u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; + /* + * Save the original txq to restore before passing to the driver + */ + ((u16 *)skb->cb)[0] = skb->queue_mapping; + if (unlikely(txq >= dev->real_num_tx_queues)) { do { txq -= dev->real_num_tx_queues;
The bonding driver is multiqueue enabled, in which each queue represents a slave to enable optional steering of output frames to given slaves against the default output policy. However, it needs to reset the skb->queue_mapping prior to queuing to the physical device or the physical slave (if it is multiqueue) could wind up transmitting on an unintended tx queue Change Notes: v2) Based on first pass review, updated the patch to restore the origional queue mapping that was found in bond_select_queue, rather than simply resetting to zero. This preserves the value of queue_mapping when it was set on receive in the forwarding case which is desireable. v3) Fixed spelling an casting error in skb->cb v4) fixed to store raw queue_mapping to avoid double decrement Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> CC: "David S. Miller" <davem@davemloft.net> --- drivers/net/bonding/bond_main.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)