ixgbe select_queue broken in 2.6.28

ixgbe select_queue broken in 2.6.28

[Stephen Hemminger]

The ixgbe driver in 2.6.28 modifies select_queue. This is bad
in a couple of ways:
  1) select_queue is now in net_device_ops so the driver won't build
     unless CONFIG_COMPAT_NET_DEV_OPS is defined
  2) it is changing data that is shared and supposed to be immutable.

Looks like a some of the DCB stuff forgot to look at net_device_ops

Re: ixgbe select_queue broken in 2.6.28

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 1 Jan 2009 22:54:56 -0800

> The ixgbe driver in 2.6.28 modifies select_queue. This is bad
> in a couple of ways:
>   1) select_queue is now in net_device_ops so the driver won't build
>      unless CONFIG_COMPAT_NET_DEV_OPS is defined
>   2) it is changing data that is shared and supposed to be immutable.
> 
> Looks like a some of the DCB stuff forgot to look at net_device_ops

Drivers shouldn't be overriding the ->select_queue() method unless
there is a very specific reason to do so.  Ethernet drivers will not
have such a reason.

This is a very bad trend and I think I'm going to need to simply
remove the ->select_queue() method to stop the bleeding.

If people keep overriding this method in their drivers, then when we
move to more sophisticated methods of computing the hash we will have
to hit all of these override spots which makes the generic queue
selection function pointless.

Re: ixgbe select_queue broken in 2.6.28

[Waskiewicz Jr, Peter P]

On Fri, 2 Jan 2009, David Miller wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 1 Jan 2009 22:54:56 -0800
> 
> > The ixgbe driver in 2.6.28 modifies select_queue. This is bad
> > in a couple of ways:
> >   1) select_queue is now in net_device_ops so the driver won't build
> >      unless CONFIG_COMPAT_NET_DEV_OPS is defined
> >   2) it is changing data that is shared and supposed to be immutable.
> > 
> > Looks like a some of the DCB stuff forgot to look at net_device_ops
> 
> Drivers shouldn't be overriding the ->select_queue() method unless
> there is a very specific reason to do so.  Ethernet drivers will not
> have such a reason.

Agreed.  I stumbled across this override in the DCB code in ixgbe right 
before the holidays.  I plan to find out why that was put in and submit a 
patch to correct it.

> This is a very bad trend and I think I'm going to need to simply
> remove the ->select_queue() method to stop the bleeding.

I understand why you want to remove this, but at the same time it is 
something that is useful if we don't abuse it.  I know for a fact this 
function pointer is very useful for routing performance setups, where 
binding a Tx queue to the CPU is very useful.  Jesse actually did the work 
on testing and getting good results with this.

> If people keep overriding this method in their drivers, then when we
> move to more sophisticated methods of computing the hash we will have
> to hit all of these override spots which makes the generic queue
> selection function pointless.

Understood.  If I clean up the use in ixgbe, and we can influence others 
to not abuse the function pointer, can we please keep it?  It really does 
provide flexibility with certain advanced features in future devices in 
specific configurations, which would be impossible to support without this 
ops pointer.

Cheers,
-PJ Waskiewicz

Re: ixgbe select_queue broken in 2.6.28

[David Miller]

From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
Date: Sun, 4 Jan 2009 19:23:20 -0800 (Pacific Standard Time)

> I understand why you want to remove this, but at the same time it is 
> something that is useful if we don't abuse it.  I know for a fact this 
> function pointer is very useful for routing performance setups, where 
> binding a Tx queue to the CPU is very useful.  Jesse actually did the work 
> on testing and getting good results with this.

This can be done generically, and in fact we have plans to make
this work.

All you want for routing is to remember the RX hash, and feed
that in (in some form) as the TX hash.

Re: ixgbe select_queue broken in 2.6.28

[Herbert Xu]

Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com> wrote:
> 
> I understand why you want to remove this, but at the same time it is 
> something that is useful if we don't abuse it.  I know for a fact this 
> function pointer is very useful for routing performance setups, where 
> binding a Tx queue to the CPU is very useful.  Jesse actually did the work 
> on testing and getting good results with this.

Sure.  But that is not driver-specific.  The only reason to do
an override is if the hash needs to be driver-specific.  So far
the only reasons that have been given are deficiencies in the
current hash, rather than a genuine need by the driver.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ixgbe select_queue broken in 2.6.28

[Waskiewicz Jr, Peter P]

On Tue, 6 Jan 2009, Herbert Xu wrote:

> Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com> wrote:
> > 
> > I understand why you want to remove this, but at the same time it is 
> > something that is useful if we don't abuse it.  I know for a fact this 
> > function pointer is very useful for routing performance setups, where 
> > binding a Tx queue to the CPU is very useful.  Jesse actually did the work 
> > on testing and getting good results with this.
> 
> Sure.  But that is not driver-specific.  The only reason to do
> an override is if the hash needs to be driver-specific.  So far
> the only reasons that have been given are deficiencies in the
> current hash, rather than a genuine need by the driver.

I agree with your statement 100%.  I do have a genuine need though, now 
that I understand why the select_queue was being overridden for DCB.  When 
ixgbe is running in DCB mode, traffic will be filtered into the traffic 
classes (i.e. Tx queues) via tc filters.  Since each queue has a specific 
priority in the hardware queueing, it's important that traffic isn't 
distributed across queues without specifically being filtered there.  The 
best way to address this is to have select_queue return Tx queue 0, which 
is the lowest priority queue for DCB.  Then if we exit DCB mode, we return 
select_queue to NULL, so the kernel's hash takes back control.

If that sounds reasonable, I can fix the original problem Stephen pointed 
out that select_queue in DCB isn't using the netdev_ops pointer.  I do 
still believe the existence of this function pointer is necessary; please 
consider the DCB case and let me know if you agree or not.

Cheers,
-PJ Waskiewicz