* [PATCH] bonding: bond_select_queue off by one
@ 2011-02-18 2:07 Phil Oester
2011-02-18 3:46 ` Ben Hutchings
2011-02-18 4:41 ` Jay Vosburgh
0 siblings, 2 replies; 6+ messages in thread
From: Phil Oester @ 2011-02-18 2:07 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 506 bytes --]
The bonding driver's bond_select_queue function simply returns
skb->queue_mapping. However queue_mapping could be == 16
for queue #16. This causes the following message to be flooded
to syslog:
kernel: bondx selects TX queue 16, but real number of TX queues is 16
ndo_select_queue wants a zero-based number, so bonding driver needs
to subtract one to return the proper queue number. Also fix grammar in
a comment while in the vicinity.
Phil Oester
Signed-off-by: Phil Oester <kernel@linuxace.com>
[-- Attachment #2: patch-bond-txq --]
[-- Type: text/plain, Size: 691 bytes --]
--- linux-2.6/drivers/net/bonding/bond_main.c.orig 2011-01-30 09:15:09.813843817 -0800
+++ linux-2.6/drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800
@@ -4537,11 +4537,11 @@
{
/*
* This helper function exists to help dev_pick_tx get the correct
- * destination queue. Using a helper function skips the a call to
+ * destination queue. Using a helper function skips a call to
* skb_tx_hash and will put the skbs in the queue we expect on their
* way down to the bonding driver.
*/
- return skb->queue_mapping;
+ return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
}
static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] bonding: bond_select_queue off by one 2011-02-18 2:07 [PATCH] bonding: bond_select_queue off by one Phil Oester @ 2011-02-18 3:46 ` Ben Hutchings 2011-02-18 4:41 ` Jay Vosburgh 1 sibling, 0 replies; 6+ messages in thread From: Ben Hutchings @ 2011-02-18 3:46 UTC (permalink / raw) To: Phil Oester; +Cc: netdev On Thu, 2011-02-17 at 18:07 -0800, Phil Oester wrote: > The bonding driver's bond_select_queue function simply returns > skb->queue_mapping. However queue_mapping could be == 16 > for queue #16. This causes the following message to be flooded > to syslog: > > kernel: bondx selects TX queue 16, but real number of TX queues is 16 > > ndo_select_queue wants a zero-based number, so bonding driver needs > to subtract one to return the proper queue number. Also fix grammar in > a comment while in the vicinity. > > Phil Oester > > Signed-off-by: Phil Oester <kernel@linuxace.com> > --- linux-2.6/drivers/net/bonding/bond_main.c.orig 2011-01-30 09:15:09.813843817 -0800 > +++ linux-2.6/drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800 > @@ -4537,11 +4537,11 @@ > { > /* > * This helper function exists to help dev_pick_tx get the correct > - * destination queue. Using a helper function skips the a call to > + * destination queue. Using a helper function skips a call to > * skb_tx_hash and will put the skbs in the queue we expect on their > * way down to the bonding driver. > */ > - return skb->queue_mapping; > + return skb->queue_mapping ? skb->queue_mapping - 1 : 0; > } > > static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev) This looks basically correct, but it should use the proper functions: skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_select_queue off by one 2011-02-18 2:07 [PATCH] bonding: bond_select_queue off by one Phil Oester 2011-02-18 3:46 ` Ben Hutchings @ 2011-02-18 4:41 ` Jay Vosburgh 2011-02-18 22:49 ` Andy Gospodarek 1 sibling, 1 reply; 6+ messages in thread From: Jay Vosburgh @ 2011-02-18 4:41 UTC (permalink / raw) To: Andy Gospodarek, Phil Oester; +Cc: netdev Phil Oester <kernel@linuxace.com> wrote: >The bonding driver's bond_select_queue function simply returns >skb->queue_mapping. However queue_mapping could be == 16 >for queue #16. This causes the following message to be flooded >to syslog: > >kernel: bondx selects TX queue 16, but real number of TX queues is 16 > >ndo_select_queue wants a zero-based number, so bonding driver needs >to subtract one to return the proper queue number. Also fix grammar in >a comment while in the vicinity. Andy, can you comment on this? If memory serves, the omission of queue ID zero was on purpose; is this patch going to break any of the functionality added by: commit bb1d912323d5dd50e1079e389f4e964be14f0ae3 Author: Andy Gospodarek <andy@greyhouse.net> Date: Wed Jun 2 08:40:18 2010 +0000 bonding: allow user-controlled output slave selection Ben Hutchings <bhutchings@solarflare.com> wrote: >This looks basically correct, but it should use the proper functions: > > skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue, et al, do the offset by one internally, but the bond_slave_override function is comparing the slave's queue_id to the skb->queue_mapping. That makes me wonder if this patch is going to mess things up, and if bond_slave_override should also use the skb_rx_queue_recorded, et al, functions. -J >Phil Oester > >Signed-off-by: Phil Oester <kernel@linuxace.com> > > >--- linux-2.6/drivers/net/bonding/bond_main.c.orig 2011-01-30 09:15:09.813843817 -0800 >+++ linux-2.6/drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800 >@@ -4537,11 +4537,11 @@ > { > /* > * This helper function exists to help dev_pick_tx get the correct >- * destination queue. Using a helper function skips the a call to >+ * destination queue. Using a helper function skips a call to > * skb_tx_hash and will put the skbs in the queue we expect on their > * way down to the bonding driver. > */ >- return skb->queue_mapping; >+ return skb->queue_mapping ? skb->queue_mapping - 1 : 0; > } > > static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev) --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_select_queue off by one 2011-02-18 4:41 ` Jay Vosburgh @ 2011-02-18 22:49 ` Andy Gospodarek 2011-02-18 23:06 ` Ben Hutchings 0 siblings, 1 reply; 6+ messages in thread From: Andy Gospodarek @ 2011-02-18 22:49 UTC (permalink / raw) To: Jay Vosburgh; +Cc: Andy Gospodarek, Phil Oester, netdev On Thu, Feb 17, 2011 at 08:41:48PM -0800, Jay Vosburgh wrote: > Phil Oester <kernel@linuxace.com> wrote: > > >The bonding driver's bond_select_queue function simply returns > >skb->queue_mapping. However queue_mapping could be == 16 > >for queue #16. This causes the following message to be flooded > >to syslog: > > > >kernel: bondx selects TX queue 16, but real number of TX queues is 16 > > > >ndo_select_queue wants a zero-based number, so bonding driver needs > >to subtract one to return the proper queue number. Also fix grammar in > >a comment while in the vicinity. > > Andy, can you comment on this? > > If memory serves, the omission of queue ID zero was on purpose; > is this patch going to break any of the functionality added by: > > commit bb1d912323d5dd50e1079e389f4e964be14f0ae3 > Author: Andy Gospodarek <andy@greyhouse.net> > Date: Wed Jun 2 08:40:18 2010 +0000 > > bonding: allow user-controlled output slave selection > My original intent was that a queue_mapping == 0 would indicate that the mode's default transmit routine would be used. We could still operate under this assumption, however. I think the patch below will work. > Ben Hutchings <bhutchings@solarflare.com> wrote: > > >This looks basically correct, but it should use the proper functions: > > > > skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; > > As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue, > et al, do the offset by one internally, but the bond_slave_override > function is comparing the slave's queue_id to the skb->queue_mapping. > > That makes me wonder if this patch is going to mess things up, > and if bond_slave_override should also use the skb_rx_queue_recorded, et > al, functions. > They could be use them, but I really dislike using functions with 'rx' in the name for options that are clearly for transmit. I would rather get rid of access to queue_id or queue_mapping in the code in question. How about something like this? I have not fully tested this, but will and would appreciate feedback from Phil or anyone else. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> --- drivers/net/bonding/bond_main.c | 11 ++++++----- drivers/net/bonding/bond_sysfs.c | 2 +- drivers/net/bonding/bonding.h | 16 ++++++++++++++++ include/linux/skbuff.h | 15 +++++++++++++++ net/core/dev.c | 4 ++-- 5 files changed, 40 insertions(+), 8 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 77e3c6a..02d8161 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4511,20 +4511,21 @@ static inline int bond_slave_override(struct bonding *bond, read_lock(&bond->lock); - if (!BOND_IS_OK(bond) || !skb->queue_mapping) + if (!BOND_IS_OK(bond) || !skb_tx_queue_recorded(skb)) goto out; /* Find out if any slaves have the same mapping as this skb. */ bond_for_each_slave(bond, check_slave, i) { - if (check_slave->queue_id == skb->queue_mapping) { + if (slave_tx_queue_recorded(check_slave) && + slave_get_tx_queue(check_slave) == skb_get_tx_queue(skb)) { slave = check_slave; break; } } /* If the slave isn't UP, use default transmit policy. */ - if (slave && slave->queue_id && IS_UP(slave->dev) && - (slave->link == BOND_LINK_UP)) { + if (slave && slave_tx_queue_recorded(slave) && + IS_UP(slave->dev) && (slave->link == BOND_LINK_UP)) { res = bond_dev_queue_xmit(bond, skb, slave->dev); } @@ -4541,7 +4542,7 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb) * skb_tx_hash and will put the skbs in the queue we expect on their * way down to the bonding driver. */ - return skb->queue_mapping; + return skb_tx_queue_recorded(skb) ? skb_get_tx_queue(skb) : 0; } static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 72bb0f6..b3bc092 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -1499,7 +1499,7 @@ static ssize_t bonding_store_queue_id(struct device *d, /* Check buffer length, valid ifname and queue id */ if (strlen(buffer) > IFNAMSIZ || !dev_valid_name(buffer) || - qid > bond->params.tx_queues) + qid >= bond->params.tx_queues) goto err_no_cmd; /* Get the pointer to that interface if it exists */ diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 31fe980..75b5798 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -422,4 +422,20 @@ static inline void bond_unregister_ipv6_notifier(void) } #endif +static inline void slave_record_tx_queue(struct slave *slave, u16 tx_queue) +{ + slave->queue_id = tx_queue + 1; +} + +static inline u16 slave_get_tx_queue(const struct slave *slave) +{ + return slave->queue_id - 1; +} + +static inline bool slave_tx_queue_recorded(const struct slave *slave) +{ + return slave->queue_id != 0; +} + + #endif /* _LINUX_BONDING_H */ diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 31f02d0..49d101c 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb) return skb->queue_mapping != 0; } +static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue) +{ + skb->queue_mapping = tx_queue + 1; +} + +static inline u16 skb_get_tx_queue(const struct sk_buff *skb) +{ + return skb->queue_mapping - 1; +} + +static inline bool skb_tx_queue_recorded(const struct sk_buff *skb) +{ + return skb->queue_mapping != 0; +} + extern u16 __skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb, unsigned int num_tx_queues); diff --git a/net/core/dev.c b/net/core/dev.c index a413276..50aa490 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2211,8 +2211,8 @@ u16 __skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb, u16 qoffset = 0; u16 qcount = num_tx_queues; - if (skb_rx_queue_recorded(skb)) { - hash = skb_get_rx_queue(skb); + if (skb_tx_queue_recorded(skb)) { + hash = skb_get_tx_queue(skb); while (unlikely(hash >= num_tx_queues)) hash -= num_tx_queues; return hash; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_select_queue off by one 2011-02-18 22:49 ` Andy Gospodarek @ 2011-02-18 23:06 ` Ben Hutchings 2011-02-21 18:06 ` Andy Gospodarek 0 siblings, 1 reply; 6+ messages in thread From: Ben Hutchings @ 2011-02-18 23:06 UTC (permalink / raw) To: Andy Gospodarek; +Cc: Jay Vosburgh, Phil Oester, netdev On Fri, 2011-02-18 at 17:49 -0500, Andy Gospodarek wrote: > On Thu, Feb 17, 2011 at 08:41:48PM -0800, Jay Vosburgh wrote: > > Phil Oester <kernel@linuxace.com> wrote: > > > > >The bonding driver's bond_select_queue function simply returns > > >skb->queue_mapping. However queue_mapping could be == 16 > > >for queue #16. This causes the following message to be flooded > > >to syslog: > > > > > >kernel: bondx selects TX queue 16, but real number of TX queues is 16 > > > > > >ndo_select_queue wants a zero-based number, so bonding driver needs > > >to subtract one to return the proper queue number. Also fix grammar in > > >a comment while in the vicinity. > > > > Andy, can you comment on this? > > > > If memory serves, the omission of queue ID zero was on purpose; > > is this patch going to break any of the functionality added by: > > > > commit bb1d912323d5dd50e1079e389f4e964be14f0ae3 > > Author: Andy Gospodarek <andy@greyhouse.net> > > Date: Wed Jun 2 08:40:18 2010 +0000 > > > > bonding: allow user-controlled output slave selection > > > > My original intent was that a queue_mapping == 0 would indicate that the > mode's default transmit routine would be used. We could still operate > under this assumption, however. I think the patch below will work. > > > Ben Hutchings <bhutchings@solarflare.com> wrote: > > > > >This looks basically correct, but it should use the proper functions: > > > > > > skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; > > > > As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue, > > et al, do the offset by one internally, but the bond_slave_override > > function is comparing the slave's queue_id to the skb->queue_mapping. > > > > That makes me wonder if this patch is going to mess things up, > > and if bond_slave_override should also use the skb_rx_queue_recorded, et > > al, functions. > > > > They could be use them, but I really dislike using functions with 'rx' > in the name for options that are clearly for transmit. This isn't an option for transmit, it is a record of the result of RX hashing (or steering). It may or may not then be used to select a TX queue. [...] > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb) > return skb->queue_mapping != 0; > } > > +static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue) > +{ > + skb->queue_mapping = tx_queue + 1; > +} > + > +static inline u16 skb_get_tx_queue(const struct sk_buff *skb) > +{ > + return skb->queue_mapping - 1; > +} > + > +static inline bool skb_tx_queue_recorded(const struct sk_buff *skb) > +{ > + return skb->queue_mapping != 0; > +} > + [...] This is nonsense. After the TX queue has been selected, it's recorded in queue_mapping *without* the offset (skb_set_queue_mapping()). Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_select_queue off by one 2011-02-18 23:06 ` Ben Hutchings @ 2011-02-21 18:06 ` Andy Gospodarek 0 siblings, 0 replies; 6+ messages in thread From: Andy Gospodarek @ 2011-02-21 18:06 UTC (permalink / raw) To: Ben Hutchings; +Cc: Andy Gospodarek, Jay Vosburgh, Phil Oester, netdev On Fri, Feb 18, 2011 at 11:06:12PM +0000, Ben Hutchings wrote: > On Fri, 2011-02-18 at 17:49 -0500, Andy Gospodarek wrote: > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb) > > return skb->queue_mapping != 0; > > } > > > > +static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue) > > +{ > > + skb->queue_mapping = tx_queue + 1; > > +} > > + > > +static inline u16 skb_get_tx_queue(const struct sk_buff *skb) > > +{ > > + return skb->queue_mapping - 1; > > +} > > + > > +static inline bool skb_tx_queue_recorded(const struct sk_buff *skb) > > +{ > > + return skb->queue_mapping != 0; > > +} > > + > [...] > > This is nonsense. After the TX queue has been selected, it's recorded > in queue_mapping *without* the offset (skb_set_queue_mapping()). > I see that now. Yay for symmetry! :) I'm actually looking over this now and will post a tested patch to address the original reporter's problem. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-21 18:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-18 2:07 [PATCH] bonding: bond_select_queue off by one Phil Oester 2011-02-18 3:46 ` Ben Hutchings 2011-02-18 4:41 ` Jay Vosburgh 2011-02-18 22:49 ` Andy Gospodarek 2011-02-18 23:06 ` Ben Hutchings 2011-02-21 18:06 ` Andy Gospodarek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).