Re: [PATCH] bonding: bond_select_queue off by one

[Ben Hutchings <bhutchings@solarflare.com>]

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.