| Message ID | 20110301153135.GL11864@gospo.rdu.redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Mar 01, 2011 at 10:31:36AM -0500, Andy Gospodarek wrote: > > The patch works as expected. Do we have any agreement on a final version? > > > > Thanks for the testing, Phil. > > I'm in favor of this patch as it does alert the admin that bonding may > not have enough default queues, but it is not as verbose (backtrace et > al) and likely to create bug reports as a message from WARN_ON. > + if (net_ratelimit()) > + pr_warning("%s selects invalid tx queue %d. Consider" > + " setting module option tx_queues > %d.", > + dev->name, txq, dev->real_num_tx_queues); It is unclear why we need to alert the admin to this situation (repeatedly). Say the incoming nic has 32 queues, and is headed out a bond (with 16). With your patch, we will log 50% of the time, no? What benefit is this log spew? While WARN_ONCE may be a bit extreme due to the backtrace, perhaps we should at least throw a 'static bool warned' variable in there to lessen the nuisance? Phil -- 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
Phil Oester <kernel@linuxace.com> wrote: >On Tue, Mar 01, 2011 at 10:31:36AM -0500, Andy Gospodarek wrote: >> > The patch works as expected. Do we have any agreement on a final version? >> > >> >> Thanks for the testing, Phil. >> >> I'm in favor of this patch as it does alert the admin that bonding may >> not have enough default queues, but it is not as verbose (backtrace et >> al) and likely to create bug reports as a message from WARN_ON. >> + if (net_ratelimit()) >> + pr_warning("%s selects invalid tx queue %d. Consider" >> + " setting module option tx_queues > %d.", >> + dev->name, txq, dev->real_num_tx_queues); > >It is unclear why we need to alert the admin to this situation (repeatedly). >Say the incoming nic has 32 queues, and is headed out a bond (with 16). >With your patch, we will log 50% of the time, no? What benefit is this >log spew? > >While WARN_ONCE may be a bit extreme due to the backtrace, perhaps we >should at least throw a 'static bool warned' variable in there to lessen >the nuisance? I'm also concerned that the log messages will be excessive. Should we instead create a bonding driver-private ethtool statistics and count these events that way? -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- 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 584f97b..acc05d6 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4643,15 +4643,27 @@ out: return res; } +/* + * This helper function exists to help dev_pick_tx get the correct + * destination queue. Using a helper function skips the a call to + * skb_tx_hash and will put the skbs in the queue we expect on their + * way down to the bonding driver. + */ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb) { - /* - * This helper function exists to help dev_pick_tx get the correct - * destination queue. Using a helper function skips the 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; + u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0; + + if (txq >= dev->real_num_tx_queues) { + /* let the user know if we do not have enough tx queues */ + if (net_ratelimit()) + pr_warning("%s selects invalid tx queue %d. Consider" + " setting module option tx_queues > %d.", + dev->name, txq, dev->real_num_tx_queues); + do + txq -= dev->real_num_tx_queues; + while (txq >= dev->real_num_tx_queues); + } + return txq; } static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)