[PATCH] NETIF_F_LLTX for devices 2

[PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

New version of the NETIF_F_LLTX for network devices patch. 

This allows network drivers to set the NETIF_F_LLTX flag
and then do their own locking in start_queue_xmit. 
This lowers locking overhead in this critical path. 

The drivers can use try lock if they want and return -1
when the lock wasn't grabbed. In this case the packet
will be requeued. For better compatibility this is only
done for drivers with LLTX set, others don't give a special
meaning to -1.

Most of the modern drivers who have a lock around hard_start_xmit
can just set this flag. It may be a good idea to convert the spin
lock there to a try lock. The only thing that should be audited
is that they do enough locking in the set_multicast_list function
too, and not also rely on xmit_lock here.

Now doesn't move any code around and does things with gotos instead.

The loop printk is also still there even for NETIF_F_LLTX

For drivers that don't set the new flag nothing changes.

Please consider applying.

-Andi


diff -u linux-2.6.8/net/core/pktgen.c-LLTX linux-2.6.8/net/core/pktgen.c
--- linux-2.6.8/net/core/pktgen.c-LLTX	2004-09-04 12:47:05.000000000 +0000
+++ linux-2.6.8/net/core/pktgen.c	2004-09-04 14:07:12.000000000 +0000
@@ -634,7 +634,8 @@
 
 		nr_frags = skb_shinfo(skb)->nr_frags;
 		   
-		spin_lock_bh(&odev->xmit_lock);
+		if (!(odev->features & NETIF_F_LLTX))
+			spin_lock_bh(&odev->xmit_lock);
 		if (!netif_queue_stopped(odev)) {
 
 			atomic_inc(&skb->users);
@@ -659,8 +660,8 @@
 			last_ok = 0;
 		}
 		
-
-		spin_unlock_bh(&odev->xmit_lock);
+		if (!(odev->features & NETIF_F_LLTX))
+			spin_unlock_bh(&odev->xmit_lock);
 
 		if (info->ipg) {
 			/* Try not to busy-spin if we have larger sleep times.
diff -u linux-2.6.8/net/sched/sch_generic.c-LLTX linux-2.6.8/net/sched/sch_generic.c
--- linux-2.6.8/net/sched/sch_generic.c-LLTX	2004-09-04 12:47:05.000000000 +0000
+++ linux-2.6.8/net/sched/sch_generic.c	2004-09-07 11:58:45.595363313 +0000
@@ -97,46 +97,73 @@
 
 	/* Dequeue packet */
 	if ((skb = q->dequeue(q)) != NULL) {
-		if (spin_trylock(&dev->xmit_lock)) {
+		unsigned nolock = (dev->features & NETIF_F_LLTX);
+		/*
+		 * When the driver has LLTX set it does its own locking
+		 * in start_xmit. No need to add additional overhead by
+		 * locking again. These checks are worth it because
+		 * even uncongested locks can be quite expensive.
+		 * The driver can do trylock like here too, in case
+		 * of lock congestion it should return -1 and the packet
+		 * will be requeued.
+		 */
+		if (!nolock) {
+			if (!spin_trylock(&dev->xmit_lock)) {
+			collision:
+				/* So, someone grabbed the driver. */
+				
+				/* It may be transient configuration error,
+				   when hard_start_xmit() recurses. We detect
+				   it by checking xmit owner and drop the
+				   packet when deadloop is detected.
+				*/
+				if (dev->xmit_lock_owner == smp_processor_id()) {
+					kfree_skb(skb);
+					if (net_ratelimit())
+						printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
+					return -1;
+				}
+				__get_cpu_var(netdev_rx_stat).cpu_collision++;
+				goto requeue;
+			}
 			/* Remember that the driver is grabbed by us. */
 			dev->xmit_lock_owner = smp_processor_id();
-
+		}
+		
+		{
 			/* And release queue */
 			spin_unlock(&dev->queue_lock);
 
 			if (!netif_queue_stopped(dev)) {
+				int ret;
 				if (netdev_nit)
 					dev_queue_xmit_nit(skb, dev);
 
-				if (dev->hard_start_xmit(skb, dev) == 0) {
-					dev->xmit_lock_owner = -1;
-					spin_unlock(&dev->xmit_lock);
-
+				/* hard_start_xmit returns: 
+				   0  device not ready
+				   1  everything ok
+				   -1 didn't get device lock (for LLTX)
+				*/ 
+				ret = dev->hard_start_xmit(skb, dev);
+				if (ret == 0) { 
+					if (!nolock) {
+						dev->xmit_lock_owner = -1;
+						spin_unlock(&dev->xmit_lock);
+					}
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
+				if (ret == -1 && nolock)
+					goto collision; 
 			}
 
 			/* Release the driver */
-			dev->xmit_lock_owner = -1;
-			spin_unlock(&dev->xmit_lock);
+			if (!nolock) { 
+				dev->xmit_lock_owner = -1;
+				spin_unlock(&dev->xmit_lock);
+			} 
 			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
-		} else {
-			/* So, someone grabbed the driver. */
-
-			/* It may be transient configuration error,
-			   when hard_start_xmit() recurses. We detect
-			   it by checking xmit owner and drop the
-			   packet when deadloop is detected.
-			 */
-			if (dev->xmit_lock_owner == smp_processor_id()) {
-				kfree_skb(skb);
-				if (net_ratelimit())
-					printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
-				return -1;
-			}
-			__get_cpu_var(netdev_rx_stat).cpu_collision++;
 		}
 
 		/* Device kicked us out :(
@@ -149,6 +176,7 @@
 		   3. device is buggy (ppp)
 		 */
 
+	requeue:
 		q->ops->requeue(skb, q);
 		netif_schedule(dev);
 		return 1;

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

This missed the current round of merges, I'll try to get it
in by the end of the week.

I'm probably going to put the infrastructure patch in first,
then the tg3/e1000 bits.  Can you resend me the e1000 patch
under private cover btw?  I've lost my copy, thanks.

Re: [PATCH] NETIF_F_LLTX for devices 2

[jamal]

On Tue, 2004-09-07 at 08:05, Andi Kleen wrote:
> New version of the NETIF_F_LLTX for network devices patch. 
> 
> This allows network drivers to set the NETIF_F_LLTX flag

> The drivers can use try lock if they want and return -1
> when the lock wasn't grabbed. In this case the packet
> will be requeued. For better compatibility this is only
> done for drivers with LLTX set, others don't give a special
> meaning to -1.

Are you reinventing the rules or changing them?

hard_start_xmit() return codes are intepreted as follows:

0: typically means the packet was put in the ring. 
It is being abused by a few drivers to mean a retry depending on the
device state (which while may work results in a longer code path). 
1: means packet was not put on the ring. i.e if you return
1, the toplayer will retry later with the same skb. 
[of course If you stash it on the ring, the danger is tx complete will
try to free it later while the toplayer code is still referencing it. A
good oops].


cheers,
jamal

Re: [PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

On Tue, Sep 07, 2004 at 05:53:04PM -0400, jamal wrote:
> On Tue, 2004-09-07 at 08:05, Andi Kleen wrote:
> > New version of the NETIF_F_LLTX for network devices patch. 
> > 
> > This allows network drivers to set the NETIF_F_LLTX flag
> 
> > The drivers can use try lock if they want and return -1
> > when the lock wasn't grabbed. In this case the packet
> > will be requeued. For better compatibility this is only
> > done for drivers with LLTX set, others don't give a special
> > meaning to -1.
> 
> Are you reinventing the rules or changing them?

I'm just offering the driver an additional choice .
> 
> hard_start_xmit() return codes are intepreted as follows:
> 
> 0: typically means the packet was put in the ring. 
> It is being abused by a few drivers to mean a retry depending on the
> device state (which while may work results in a longer code path). 
> 1: means packet was not put on the ring. i.e if you return
> 1, the toplayer will retry later with the same skb. 
> [of course If you stash it on the ring, the danger is tx complete will
> try to free it later while the toplayer code is still referencing it. A
> good oops].

Actually when you return 1 then the kernel prints an ugly 
message and it is considered a bug.  Here -1 is legal.
For safety -1 should be only used for locking purposes though.

-Andi

Re: [PATCH] NETIF_F_LLTX for devices 2

[Herbert Xu]

Andi Kleen <ak@suse.de> wrote:
>
>> 1: means packet was not put on the ring. i.e if you return
>> 1, the toplayer will retry later with the same skb. 
>> [of course If you stash it on the ring, the danger is tx complete will
>> try to free it later while the toplayer code is still referencing it. A
>> good oops].
> 
> Actually when you return 1 then the kernel prints an ugly 
> message and it is considered a bug.  Here -1 is legal.

1 is legal in contexts where queueing occurs.  See for example
qdisc_restart().

It's only illegal here because this is the direct xmit path without
queueing.

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: [PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

On Wed, Sep 08, 2004 at 05:07:56PM +1000, Herbert Xu wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> >> 1: means packet was not put on the ring. i.e if you return
> >> 1, the toplayer will retry later with the same skb. 
> >> [of course If you stash it on the ring, the danger is tx complete will
> >> try to free it later while the toplayer code is still referencing it. A
> >> good oops].
> > 
> > Actually when you return 1 then the kernel prints an ugly 
> > message and it is considered a bug.  Here -1 is legal.
> 
> 1 is legal in contexts where queueing occurs.  See for example
> qdisc_restart().

I agree my sentence was misleading. 

Basically the distinction here is:

1 is for flow control 
-1 is for lock contention

I think it's better to separate them because it minimizes the risk
of breaking old drivers.

> 
> It's only illegal here because this is the direct xmit path without
> queueing.

You are thinking of the wrong patch. This patch is changing qdisc_restart

-Andi

Re: [PATCH] NETIF_F_LLTX for devices 2

[jamal]

On Wed, 2004-09-08 at 03:24, Andi Kleen wrote:
> On Wed, Sep 08, 2004 at 05:07:56PM +1000, Herbert Xu wrote:
> > Andi Kleen <ak@suse.de> wrote:
> > >
> > >> 1: means packet was not put on the ring. i.e if you return
> > >> 1, the toplayer will retry later with the same skb. 
> > >> [of course If you stash it on the ring, the danger is tx complete will
> > >> try to free it later while the toplayer code is still referencing it. A
> > >> good oops].
> > > 
> > > Actually when you return 1 then the kernel prints an ugly 
> > > message and it is considered a bug.  Here -1 is legal.

Is this an effect of  your code? This is not so in existing code.

> > 1 is legal in contexts where queueing occurs.  See for example
> > qdisc_restart().
> 
> I agree my sentence was misleading. 
> 
> Basically the distinction here is:
> 
> 1 is for flow control 
> -1 is for lock contention
> 
> I think it's better to separate them because it minimizes the risk
> of breaking old drivers.
> 

Dont see how its going to break old drivers since they wont be making
this kind of call anyways.

In both cases it is semantically a flow control i.e "transmit path is
busy"

cheers,
jamal

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

On 08 Sep 2004 03:47:57 -0400
jamal <hadi@cyberus.ca> wrote:

> On Wed, 2004-09-08 at 03:24, Andi Kleen wrote:
> > On Wed, Sep 08, 2004 at 05:07:56PM +1000, Herbert Xu wrote:
> > > Andi Kleen <ak@suse.de> wrote:
> > > >
> > > >> 1: means packet was not put on the ring. i.e if you return
> > > >> 1, the toplayer will retry later with the same skb. 
> > > >> [of course If you stash it on the ring, the danger is tx complete will
> > > >> try to free it later while the toplayer code is still referencing it. A
> > > >> good oops].
> > > > 
> > > > Actually when you return 1 then the kernel prints an ugly 
> > > > message and it is considered a bug.  Here -1 is legal.
> 
> Is this an effect of  your code? This is not so in existing code.

We are merely moving the sch_generic.c locking logic into the
drivers.  The behavior is entirely equivalent except that one
level of unnecessary locking has been removed.

I think his change is valid, will not break existing drivers (as
you mentioned as well Jamal), and works well for the cases he has
shown patches of.  So I'm going to apply his patch.

BTW, if we are really concerned about some existing driver returning
-1 from hard_start_xmit() without the new feature flag being enabled,
we can test for that and log a debugging message if it happens.

Re: [PATCH] NETIF_F_LLTX for devices 2

[jamal]

On Wed, 2004-09-08 at 16:47, David S. Miller wrote:

> 
> We are merely moving the sch_generic.c locking logic into the
> drivers.  The behavior is entirely equivalent except that one
> level of unnecessary locking has been removed.
> 
> I think his change is valid, will not break existing drivers (as
> you mentioned as well Jamal), and works well for the cases he has
> shown patches of.  So I'm going to apply his patch.
> 
> BTW, if we are really concerned about some existing driver returning
> -1 from hard_start_xmit() without the new feature flag being enabled,
> we can test for that and log a debugging message if it happens.

I am not 100% happy but let me do some testing on it. Would the best
image be the latest bk snapshot?

cheers,
jamal

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

On 10 Sep 2004 09:33:35 -0400
jamal <hadi@cyberus.ca> wrote:

> On Wed, 2004-09-08 at 16:47, David S. Miller wrote:
> 
> > 
> > We are merely moving the sch_generic.c locking logic into the
> > drivers.  The behavior is entirely equivalent except that one
> > level of unnecessary locking has been removed.
> > 
> > I think his change is valid, will not break existing drivers (as
> > you mentioned as well Jamal), and works well for the cases he has
> > shown patches of.  So I'm going to apply his patch.
> > 
> > BTW, if we are really concerned about some existing driver returning
> > -1 from hard_start_xmit() without the new feature flag being enabled,
> > we can test for that and log a debugging message if it happens.
> 
> I am not 100% happy but let me do some testing on it. Would the best
> image be the latest bk snapshot?

Yes, Andi's code plus e1000 and tg3 driver uses are in current
2.6.x BK.  Just grep for NETIF_F_LLTX in drivers/net/tg3.c to
be sure you've got it in your tree.

Re: [PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

On Fri, Sep 10, 2004 at 09:33:35AM -0400, jamal wrote:
> On Wed, 2004-09-08 at 16:47, David S. Miller wrote:
> 
> > 
> > We are merely moving the sch_generic.c locking logic into the
> > drivers.  The behavior is entirely equivalent except that one
> > level of unnecessary locking has been removed.
> > 
> > I think his change is valid, will not break existing drivers (as
> > you mentioned as well Jamal), and works well for the cases he has
> > shown patches of.  So I'm going to apply his patch.
> > 
> > BTW, if we are really concerned about some existing driver returning
> > -1 from hard_start_xmit() without the new feature flag being enabled,
> > we can test for that and log a debugging message if it happens.

The -1 test is only done when LLTX is set. So even when a existing
driver does that it's fine. Only the drivers that set the new bit
need to be checked.

> 
> I am not 100% happy but let me do some testing on it. Would the best
> image be the latest bk snapshot?

What exactly are you not happy about?


-Andi

Re: [PATCH] NETIF_F_LLTX for devices 2

[jamal]

On Sat, 2004-09-11 at 10:21, Andi Kleen wrote:
> On Fri, Sep 10, 2004 at 09:33:35AM -0400, jamal wrote:

> > 
> > I am not 100% happy but let me do some testing on it. Would the best
> > image be the latest bk snapshot?
> 
> What exactly are you not happy about?
> 

Grr, Andi, Did you really have to force me to explain why i am unhappy?
As a general principle i  think it is against human nature to be happy. 
In this specific case, though:
Remember that famous movie breakup line "its just me not you"? 
Well, this is along those lines;-> I am not as conservative as say
Donald Becker, but it worries me messing with code that might have
repurcassions (as innocent as that code seems). It doesnt help that
recently theres more breakages in the netsched code than there have been
in the last few years put together.
If Alexey(who knows that piece of code better than anybody) was awake
and he ACKed i wouldnt be commenting. 

Having said that, when i saw your patch with the comment:
/* 	hard_start_xmit returns: 
 	0  device not ready
 	1  everything ok
 	-1 didn't get device lock (for LLTX)
*/

my mythical level of unhapiness went up;-> 
Clearly for 0 and 1 your comments are misleading/wrong (even though your
code does the right thing).

If i was the one who had thought of the need for this new lock-riddance
then i would have done it as follows:
- have a devices xmit_lock as an alias to this other lock in case of
NETIF_F_LLTX 
Then you wouldnt have to touch this code. Infact if it is not too late
why not do it like that?

To add to all this, theres well known semantics of what the return code
means:

-> 0 means the packet was swallowed by driver.
-> 1 means packet was not swallowed.

I dont know why you would need to introduce a new return code. 
All the device is saying is "sorry, I am busy, come back later". Infact
in the pre-softnet days the flag was called precisely "tbusy".
Now, it would make a lot of sense to introduce new return code if you
actually have plans to use it. You probably do but havent stated it. Do
you have plans to use that return code?

Does that explain my unhappiness? ;-> Does that make you happy? ;->
Now i need to refill my coffee.

cheers,
jamal

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

On 11 Sep 2004 16:15:32 -0400
jamal <hadi@cyberus.ca> wrote:

> If i was the one who had thought of the need for this new lock-riddance
> then i would have done it as follows:
> - have a devices xmit_lock as an alias to this other lock in case of
> NETIF_F_LLTX 
> Then you wouldnt have to touch this code. Infact if it is not too late
> why not do it like that?

If you turn dev->xmit_lock into a spinlock pointer, that would
incur much deeper changes across the tree than Andi's version
because there are a lot of xmit_lock explicit references out
there.

I think Andi made the right choice for his implementation.
And frankly I don't what is worrying about the
"-1" return value, it can occur in only one spot in a very
specific controlled case and it's behavior is incredibly well
defined (if not by accurate comments then by the code itself :-)

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

On Sat, Sep 11, 2004 at 05:45:35PM -0700, David S. Miller wrote:
> I think Andi made the right choice for his implementation.
> And frankly I don't what is worrying about the
> "-1" return value, it can occur in only one spot in a very
> specific controlled case and it's behavior is incredibly well
> defined (if not by accurate comments then by the code itself :-)

Not commenting on the overall issue, but just the return code:

We already have net drivers getting the current TWO return codes wrong.
Adding one more -magic number- to the mix is just plain silly.

1. Add constants
2. Add clear, unambiguous documentation describing all three return codes
3. (janitor) use constants

Lacking #1 and #2 are design flaws that ignore existing problems,
and create new ones.  Luckily #1 and #2 are simple, human-friendly
fixes.

	Jeff

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

Oh and update Documentation/networking/netdevice.txt.

If people are going to screw with the TX path, at least do it right and
make it non-mysterious for everyone else.

See Al Viro's Documentation/filesystem/directory-locking doc for a
proper way to document a locking change that potentially affects every
net driver in the kernel.

	Jeff

Re: [PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

On Sun, Sep 12, 2004 at 06:01:14AM -0400, Jeff Garzik wrote:
> 
> Oh and update Documentation/networking/netdevice.txt.
> 
> If people are going to screw with the TX path, at least do it right and
> make it non-mysterious for everyone else.
> 
> See Al Viro's Documentation/filesystem/directory-locking doc for a
> proper way to document a locking change that potentially affects every
> net driver in the kernel.

<broken record mode>
No it doesn't. It only affects every driver who sets NETIF_F_LLTX.
For drivers that don't set this flag there is no change at all. 
</record>

I wasn't aware of Documentation/netdevices.txt, but I agree it 
would be a good idea to update it. Patch for that attached.
DaveM, please apply.

-Andi

---------------------------------------------------------------------

Add documentation for new NETIF_F_LLTX feature.


diff -u linux-2.6.8-work/Documentation/networking/netdevices.txt-o linux-2.6.8-work/Documentation/networking/netdevices.txt
--- linux-2.6.8-work/Documentation/networking/netdevices.txt-o	2004-03-21 21:12:11.000000000 +0100
+++ linux-2.6.8-work/Documentation/networking/netdevices.txt	2004-09-12 12:24:02.000000000 +0200
@@ -43,8 +43,21 @@
 
 dev->hard_start_xmit:
 	Synchronization: dev->xmit_lock spinlock.
+	When the driver sets NETIF_F_LLTX in dev->features this will be
+	called without holding xmit_lock. In this case the driver 
+	has to lock by itself when needed. It is recommended to use a try lock
+	for this and return -1 when the spin lock fails. 
+	The locking there should also properly protect against 
+	set_multicast_list
 	Context: BHs disabled
 	Notes: netif_queue_stopped() is guaranteed false
+	Return codes: 
+	0 everything ok. 
+	1 Cannot transmit packet, try later 
+	(usually a bug, means queue start/stop flow control is broken in your 
+	driver) 
+	-1 Locking failed, please retry quickly. Only valid when 
+	NETIF_F_LLTX is set.
 
 dev->tx_timeout:
 	Synchronization: dev->xmit_lock spinlock.

Re: [PATCH] NETIF_F_LLTX for devices 2

[Francois Romieu]

Andi Kleen <ak@suse.de> :
[...]
> I wasn't aware of Documentation/netdevices.txt, but I agree it 
> would be a good idea to update it. Patch for that attached.
> DaveM, please apply.

I have modified your patch so as to include a bit from a message
of Jamal on netdev the 05/07/2004.

diff -u linux-2.6.8-work/Documentation/networking/netdevices.txt-o linux-2.6.8-work/Documentation/networking/netdevices.txt
--- linux-2.6.8-work/Documentation/networking/netdevices.txt-o	2004-03-21 21:12:11.000000000 +0100
+++ linux-2.6.8-work/Documentation/networking/netdevices.txt	2004-09-12 12:24:02.000000000 +0200
@@ -43,8 +43,21 @@
 
 dev->hard_start_xmit:
 	Synchronization: dev->xmit_lock spinlock.
+	When the driver sets NETIF_F_LLTX in dev->features this will be
+	called without holding xmit_lock. In this case the driver 
+	has to lock by itself when needed. It is recommended to use a try lock
+	for this and return -1 when the spin lock fails. 
+	The locking there should also properly protect against 
+	set_multicast_list
 	Context: BHs disabled
 	Notes: netif_queue_stopped() is guaranteed false
+	Return codes: 
+	o 0 everything ok. 
+	o 1 Cannot transmit packet, try later 
+	  Usually a bug, means queue start/stop flow control is broken in
+	  the driver. Note: the driver must NOT put the skb in its DMA ring.
+	o -1 Locking failed, please retry quickly. Only valid when 
+	  NETIF_F_LLTX is set.
 
 dev->tx_timeout:
 	Synchronization: dev->xmit_lock spinlock.

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

On Sun, 12 Sep 2004 13:03:20 +0200
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Andi Kleen <ak@suse.de> :
> [...]
> > I wasn't aware of Documentation/netdevices.txt, but I agree it 
> > would be a good idea to update it. Patch for that attached.
> > DaveM, please apply.
> 
> I have modified your patch so as to include a bit from a message
> of Jamal on netdev the 05/07/2004.

I've enhanced it further to make use of the macro'ized
named Jamal added for these values.

Thanks everyone.

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

On Sun, Sep 12, 2004 at 12:25:29PM +0200, Andi Kleen wrote:
> On Sun, Sep 12, 2004 at 06:01:14AM -0400, Jeff Garzik wrote:
> > 
> > Oh and update Documentation/networking/netdevice.txt.
> > 
> > If people are going to screw with the TX path, at least do it right and
> > make it non-mysterious for everyone else.
> > 
> > See Al Viro's Documentation/filesystem/directory-locking doc for a
> > proper way to document a locking change that potentially affects every
> > net driver in the kernel.
> 
> <broken record mode>
> No it doesn't. It only affects every driver who sets NETIF_F_LLTX.
> For drivers that don't set this flag there is no change at all. 
> </record>

Incorrect, you are changing the callsites, which -does- affect every
driver.


> I wasn't aware of Documentation/netdevices.txt, but I agree it 
> would be a good idea to update it. Patch for that attached.
> DaveM, please apply.

Thanks, but still need a patch for return value constants, otherwise you
are compounding rather than addressing a current problem.

	Jeff

Re: [PATCH] NETIF_F_LLTX for devices 2

[jamal]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

On Sun, 2004-09-12 at 12:16, Jeff Garzik wrote:

> 
> Thanks, but still need a patch for return value constants, otherwise you
> are compounding rather than addressing a current problem.

Something along the lines of attached patch?
Francois needs to update the doc and you need someone to get the
janitors enthusiastic (i just updated e1000)

cheers,
jamal


[-- Attachment #2: andi-patch --]
[-- Type: text/plain, Size: 2686 bytes --]

--- 269-rc1-bk17/include/linux/netdevice.h	2004/09/12 16:58:13	1.1
+++ 269-rc1-bk17/include/linux/netdevice.h	2004/09/12 17:10:45
@@ -73,6 +73,11 @@
 
 #define MAX_ADDR_LEN	32		/* Largest hardware address length */
 
+/* Driver transmit return codes */
+#define NETDEV_TX_OK 0		/* driver took care of packet */
+#define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
+#define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */
+
 /*
  *	Compute the worst case header length according to the protocols
  *	used.
--- 269-rc1-bk17/net/sched/sch_generic.c	2004/09/12 16:55:03	1.1
+++ 269-rc1-bk17/net/sched/sch_generic.c	2004/09/12 17:08:52
@@ -139,13 +139,8 @@
 				if (netdev_nit)
 					dev_queue_xmit_nit(skb, dev);
 
-				/* hard_start_xmit returns: 
-				   0  device not ready
-				   1  everything ok
-				   -1 didn't get device lock (for LLTX)
-				*/ 
 				ret = dev->hard_start_xmit(skb, dev);
-				if (ret == 0) { 
+				if (ret == NETDEV_TX_OK) { 
 					if (!nolock) {
 						dev->xmit_lock_owner = -1;
 						spin_unlock(&dev->xmit_lock);
@@ -153,10 +148,11 @@
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
-				if (ret == -1 && nolock)
+				if (ret == NETDEV_TX_LOCKED && nolock)
 					goto collision; 
 			}
 
+			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
 			if (!nolock) { 
 				dev->xmit_lock_owner = -1;
@@ -176,7 +172,7 @@
 		   3. device is buggy (ppp)
 		 */
 
-	requeue:
+requeue:
 		q->ops->requeue(skb, q);
 		netif_schedule(dev);
 		return 1;
--- 269-rc1-bk17/drivers/net/e1000/e1000_main.c	2004/09/12 17:05:58	1.1
+++ 269-rc1-bk17/drivers/net/e1000/e1000_main.c	2004/09/12 17:07:30
@@ -1778,7 +1778,7 @@
 
 	if(unlikely(skb->len <= 0)) {
 		dev_kfree_skb_any(skb);
-		return 0;
+		return NETDEV_TX_OK;
 	}
 
 #ifdef NETIF_F_TSO
@@ -1817,7 +1817,7 @@
  	if (!spin_trylock(&adapter->tx_lock)) { 
  		/* Collision - tell upper layer to requeue */ 
  		local_irq_restore(flags); 
- 		return -1; 
+ 		return NETDEV_TX_LOCKED; 
  	} 
 
 	/* need: count + 2 desc gap to keep tail from touching
@@ -1825,7 +1825,7 @@
 	if(E1000_DESC_UNUSED(&adapter->tx_ring) < count + 2) {
 		netif_stop_queue(netdev);
 		spin_unlock_irqrestore(&adapter->tx_lock, flags);
-		return 1;
+		return NETDEV_TX_BUSY;
 	}
 
 	if(unlikely(adapter->hw.mac_type == e1000_82547)) {
@@ -1833,7 +1833,7 @@
 			netif_stop_queue(netdev);
 			mod_timer(&adapter->tx_fifo_stall_timer, jiffies);
 			spin_unlock_irqrestore(&adapter->tx_lock, flags);
-			return 1;
+			return NETDEV_TX_BUSY;
 		}
 	}
 
@@ -1856,7 +1856,7 @@
 	netdev->trans_start = jiffies;
 
 	spin_unlock_irqrestore(&adapter->tx_lock, flags);
-	return 0;
+	return NETDEV_TX_OK;
 }
 
 /**

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

jamal wrote:
> On Sun, 2004-09-12 at 12:16, Jeff Garzik wrote:
> 
> 
>>Thanks, but still need a patch for return value constants, otherwise you
>>are compounding rather than addressing a current problem.
> 
> 
> Something along the lines of attached patch?

Yep, thanks, ACK.

	Jeff

Re: [PATCH] NETIF_F_LLTX for devices 2

[David S. Miller]

On 12 Sep 2004 13:34:30 -0400
jamal <hadi@cyberus.ca> wrote:

> On Sun, 2004-09-12 at 12:16, Jeff Garzik wrote:
> 
> > 
> > Thanks, but still need a patch for return value constants, otherwise you
> > are compounding rather than addressing a current problem.
> 
> Something along the lines of attached patch?
> Francois needs to update the doc and you need someone to get the
> janitors enthusiastic (i just updated e1000)

Looks beautiful.  I added this patch and did the tg3 bits
in my local tree as well.

Thanks Jamal.

Re: [PATCH] NETIF_F_LLTX for devices 2

[Andrew Grover]

On 12 Sep 2004 13:34:30 -0400, jamal <hadi@cyberus.ca> wrote:
> On Sun, 2004-09-12 at 12:16, Jeff Garzik wrote:
> > Thanks, but still need a patch for return value constants, otherwise you
> > are compounding rather than addressing a current problem.
> 
> Something along the lines of attached patch?
> Francois needs to update the doc and you need someone to get the
> janitors enthusiastic (i just updated e1000)

+#define NETDEV_TX_OK 0		/* driver took care of packet */
+#define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
+#define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */

Why is TX_LOCKED -1 instead of, say, 2?

(The actual values are now irrelevant, right? Non sequential consts
makes one think their value is still important in some way. Maybe it's
just me.)

-- Andy

Re: [PATCH] NETIF_F_LLTX for devices 2

[Andi Kleen]

On Sun, Sep 12, 2004 at 12:16:05PM -0400, Jeff Garzik wrote:
> Incorrect, you are changing the callsites, which -does- affect every
> driver.

Please read the code before making such claims. 

The new return code is _only_ checked when NETIF_F_LLTX is set. 
A driver that doesn't set this new flag won't ever recognize 
any difference.

-Andi

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

Andi Kleen wrote:
> On Sun, Sep 12, 2004 at 12:16:05PM -0400, Jeff Garzik wrote:
> 
>>Incorrect, you are changing the callsites, which -does- affect every
>>driver.
> 
> 
> Please read the code before making such claims. 
> 
> The new return code is _only_ checked when NETIF_F_LLTX is set. 
> A driver that doesn't set this new flag won't ever recognize 
> any difference.


I read the code :)

The basic premise is that one should be _really_ conservative when 
touching the core RX and TX paths.  Regardless of how safe _you_ feel 
the code is, it is very new, under-analyzed, and untried.

The NAPI-related bug recently fixed in tg3 is an example of the 
unintended consequences of using this new feature.

	Jeff

Re: [PATCH] NETIF_F_LLTX for devices 2

[Jeff Garzik]

Andi Kleen wrote:
> I wasn't aware of Documentation/netdevices.txt, but I agree it 
> would be a good idea to update it. Patch for that attached.
> DaveM, please apply.

Thanks, ACK.

	Jeff