* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-09 2:14 UTC (permalink / raw)
To: David Miller
Cc: johnpol, herbert, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, mcarlson, gaagaan, jagana, general, mchan,
tgraf, randy.dunlap, sri, shemminger, kaber
In-Reply-To: <20071008.184126.124062865.davem@davemloft.net>
On Mon, 2007-08-10 at 18:41 -0700, David Miller wrote:
> I also want to point out another issue. Any argument wrt. reordering
> is specious at best because right now reordering from qdisc to device
> happens anyways.
>
> And that's because we drop the qdisc lock first, then we grab the
> transmit lock on the device and submit the packet. So, after we
> drop the qdisc lock, another cpu can get the qdisc lock, get the
> next packet (perhaps a lower priority one) and then sneak in to
> get the device transmit lock before the first thread can, and
> thus the packets will be submitted out of order.
>
You forgot QDISC_RUNNING Dave;-> the above cant happen.
Essentially at any one point in time, we are guaranteed that we can have
multiple cpus enqueueing but only can be dequeueing (the one that
managed to grab QDISC_RUNNING) i.e multiple producers to the qdisc queue
but only one consumer. Only the dequeuer has access to the txlock.
> This, along with other things, makes me believe that ordering really
> doesn't matter in practice. And therefore, in practice, we can treat
> everything from the qdisc to the real hardware as a FIFO even if
> something else is going on inside the black box which might reorder
> packets on the wire.
I think it is important to get the scheduling right - estimations can be
a last resort. For example, If i have voip competing for the wire with
ftp on two different rings/cpus and i specified that voip should be more
important i may consider equipment faulty if it works "most of the
time" (when ftp is not clogging the wire) and at times i am asked to
repeat what i just said.
cheers,
jamal
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-09 2:15 UTC (permalink / raw)
To: Herbert Xu
Cc: johnpol, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, mcarlson, kaber, gaagaan, jagana, general,
mchan, tgraf, randy.dunlap, shemminger, David Miller, sri
In-Reply-To: <20071009020442.GA14746@gondor.apana.org.au>
On Tue, 2007-09-10 at 10:04 +0800, Herbert Xu wrote:
> Please revert
>
> commit 41843197b17bdfb1f97af0a87c06d24c1620ba90
> Author: Jamal Hadi Salim <hadi@cyberus.ca>
> Date: Tue Sep 25 19:27:13 2007 -0700
>
> [NET_SCHED]: explict hold dev tx lock
>
> As this change introduces potential reordering and I don't think
> we've discussed this aspect sufficiently.
How does it introduce reordering?
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Herbert Xu @ 2007-10-09 2:16 UTC (permalink / raw)
To: jamal
Cc: David Miller, jeff, peter.p.waskiewicz.jr, krkumar2, johnpol,
kaber, shemminger, jagana, Robert.Olsson, rick.jones2, xma,
gaagaan, netdev, rdreier, mcarlson, mchan, general, tgraf,
randy.dunlap, sri
In-Reply-To: <1191896071.4373.156.camel@localhost>
On Mon, Oct 08, 2007 at 10:14:30PM -0400, jamal wrote:
>
> You forgot QDISC_RUNNING Dave;-> the above cant happen.
> Essentially at any one point in time, we are guaranteed that we can have
> multiple cpus enqueueing but only can be dequeueing (the one that
> managed to grab QDISC_RUNNING) i.e multiple producers to the qdisc queue
> but only one consumer. Only the dequeuer has access to the txlock.
Good point. You had me worried for a sec :)
Dave, Jamal's patch is fine as it is and doesn't actually create
any packet reordering.
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
^ permalink raw reply
* Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Herbert Xu @ 2007-10-09 2:16 UTC (permalink / raw)
To: jamal
Cc: David Miller, jeff, peter.p.waskiewicz.jr, krkumar2, johnpol,
kaber, shemminger, jagana, Robert.Olsson, rick.jones2, xma,
gaagaan, netdev, rdreier, mcarlson, mchan, general, tgraf,
randy.dunlap, sri
In-Reply-To: <1191896149.4373.157.camel@localhost>
On Mon, Oct 08, 2007 at 10:15:49PM -0400, jamal wrote:
> On Tue, 2007-09-10 at 10:04 +0800, Herbert Xu wrote:
>
> > Please revert
> >
> > commit 41843197b17bdfb1f97af0a87c06d24c1620ba90
> > Author: Jamal Hadi Salim <hadi@cyberus.ca>
> > Date: Tue Sep 25 19:27:13 2007 -0700
> >
> > [NET_SCHED]: explict hold dev tx lock
> >
> > As this change introduces potential reordering and I don't think
> > we've discussed this aspect sufficiently.
>
> How does it introduce reordering?
No it doesn't. I'd forgotten about the QDISC_RUNNING bit :)
--
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
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-09 2:19 UTC (permalink / raw)
To: Herbert Xu
Cc: johnpol, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, mcarlson, kaber, gaagaan, jagana, general,
mchan, tgraf, randy.dunlap, shemminger, David Miller, sri
In-Reply-To: <20071009021646.GB14917@gondor.apana.org.au>
On Tue, 2007-09-10 at 10:16 +0800, Herbert Xu wrote:
>
> No it doesn't. I'd forgotten about the QDISC_RUNNING bit :)
You should not better, you wrote it and ive been going insane trying to
break for at least a year now ;->
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Herbert Xu @ 2007-10-09 2:20 UTC (permalink / raw)
To: jamal
Cc: David Miller, jeff, peter.p.waskiewicz.jr, krkumar2, johnpol,
kaber, shemminger, jagana, Robert.Olsson, rick.jones2, xma,
gaagaan, netdev, rdreier, mcarlson, mchan, general, tgraf,
randy.dunlap, sri
In-Reply-To: <1191896342.4373.159.camel@localhost>
On Mon, Oct 08, 2007 at 10:19:02PM -0400, jamal wrote:
> On Tue, 2007-09-10 at 10:16 +0800, Herbert Xu wrote:
>
> >
> > No it doesn't. I'd forgotten about the QDISC_RUNNING bit :)
>
> You should not better, you wrote it and ive been going insane trying to
> break for at least a year now ;->
Well you've broken me at least :)
--
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
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: David Miller @ 2007-10-09 2:43 UTC (permalink / raw)
To: herbert
Cc: johnpol, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, hadi, mcarlson, gaagaan, jagana, general,
mchan, tgraf, randy.dunlap, shemminger, kaber, sri
In-Reply-To: <20071009020318.GA14708@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 9 Oct 2007 10:03:18 +0800
> On Tue, Oct 09, 2007 at 10:01:15AM +0800, Herbert Xu wrote:
> > On Mon, Oct 08, 2007 at 06:41:26PM -0700, David Miller wrote:
> > >
> > > I also want to point out another issue. Any argument wrt. reordering
> > > is specious at best because right now reordering from qdisc to device
> > > happens anyways.
> >
> > This is not true.
> >
> > If your device has a qdisc at all, then you will end up in the
> > function qdisc_restart, where we release the queue lock only
> > after acquiring the TX lock.
> >
> > So right now this path does not create any reordering.
>
> Argh! Someone's just broken this. I think we should restore
> the original behaviour.
Right, that's Jamal's recent patch. It looked funny to me too.
I think we can't make this change, the acquisition of the device
transmit lock before we release the qdisc is the only thing that
prevents reordering between qdisc and device.
Otherwise all of the prioritization is pretty much for nothing as
I described in another email today.
Jamal, I'm pretty sure we have to revert this, you can't change the
locking in this way.
commit 41843197b17bdfb1f97af0a87c06d24c1620ba90
Author: Jamal Hadi Salim <hadi@cyberus.ca>
Date: Tue Sep 25 19:27:13 2007 -0700
[NET_SCHED]: explict hold dev tx lock
For N cpus, with full throttle traffic on all N CPUs, funneling traffic
to the same ethernet device, the devices queue lock is contended by all
N CPUs constantly. The TX lock is only contended by a max of 2 CPUS.
In the current mode of operation, after all the work of entering the
dequeue region, we may endup aborting the path if we are unable to get
the tx lock and go back to contend for the queue lock. As N goes up,
this gets worse.
The changes in this patch result in a small increase in performance
with a 4CPU (2xdual-core) with no irq binding. Both e1000 and tg3
showed similar behavior;
Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index e970e8e..95ae119 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -134,34 +134,19 @@ static inline int qdisc_restart(struct net_device *dev)
{
struct Qdisc *q = dev->qdisc;
struct sk_buff *skb;
- unsigned lockless;
int ret;
/* Dequeue packet */
if (unlikely((skb = dev_dequeue_skb(dev, q)) == NULL))
return 0;
- /*
- * When the driver has LLTX set, it does its own locking in
- * start_xmit. These checks are worth it because even uncongested
- * locks can be quite expensive. The driver can do a trylock, as
- * is being done here; in case of lock contention it should return
- * NETDEV_TX_LOCKED and the packet will be requeued.
- */
- lockless = (dev->features & NETIF_F_LLTX);
-
- if (!lockless && !netif_tx_trylock(dev)) {
- /* Another CPU grabbed the driver tx lock */
- return handle_dev_cpu_collision(skb, dev, q);
- }
/* And release queue */
spin_unlock(&dev->queue_lock);
+ HARD_TX_LOCK(dev, smp_processor_id());
ret = dev_hard_start_xmit(skb, dev);
-
- if (!lockless)
- netif_tx_unlock(dev);
+ HARD_TX_UNLOCK(dev);
spin_lock(&dev->queue_lock);
q = dev->qdisc;
^ permalink raw reply related
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: David Miller @ 2007-10-09 2:45 UTC (permalink / raw)
To: herbert
Cc: johnpol, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, hadi, mcarlson, gaagaan, jagana, general,
mchan, tgraf, randy.dunlap, shemminger, kaber, sri
In-Reply-To: <20071009020442.GA14746@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 9 Oct 2007 10:04:42 +0800
> On Tue, Oct 09, 2007 at 10:03:18AM +0800, Herbert Xu wrote:
> > On Tue, Oct 09, 2007 at 10:01:15AM +0800, Herbert Xu wrote:
> > > On Mon, Oct 08, 2007 at 06:41:26PM -0700, David Miller wrote:
> > > >
> > > > I also want to point out another issue. Any argument wrt. reordering
> > > > is specious at best because right now reordering from qdisc to device
> > > > happens anyways.
> > >
> > > This is not true.
> > >
> > > If your device has a qdisc at all, then you will end up in the
> > > function qdisc_restart, where we release the queue lock only
> > > after acquiring the TX lock.
> > >
> > > So right now this path does not create any reordering.
> >
> > Argh! Someone's just broken this. I think we should restore
> > the original behaviour.
>
> Please revert
>
> commit 41843197b17bdfb1f97af0a87c06d24c1620ba90
> Author: Jamal Hadi Salim <hadi@cyberus.ca>
> Date: Tue Sep 25 19:27:13 2007 -0700
>
> [NET_SCHED]: explict hold dev tx lock
>
> As this change introduces potential reordering and I don't think
> we've discussed this aspect sufficiently.
Agreed, and done.
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: David Miller @ 2007-10-09 2:46 UTC (permalink / raw)
To: jeff
Cc: johnpol, herbert, gaagaan, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, hadi, mcarlson, jagana, general, mchan,
tgraf, randy.dunlap, sri, shemminger, kaber
In-Reply-To: <470AE373.9020207@garzik.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Mon, 08 Oct 2007 22:12:03 -0400
> I'm interested in working on a load balancer function that approximates
>
> skb->queue_mapping = smp_processor_id()
>
> I'd be happy to code and test in that direction, based on your lib.
It's the second algorithm that will be available :-) Just add
a "% num_tx_queues" to the result.
> IMO the net driver really should provide a hint as to what it wants.
>
> 8139cp and tg3 would probably prefer multiple TX queue behavior to match
> silicon behavior -- strict prio.
>
> And I'll volunteer to write the net driver code for that, if people want
> to see how things would look for that type of hardware packet scheduling.
Ok.
^ permalink raw reply
* Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Herbert Xu @ 2007-10-09 2:46 UTC (permalink / raw)
To: David Miller
Cc: jeff, hadi, peter.p.waskiewicz.jr, krkumar2, johnpol, kaber,
shemminger, jagana, Robert.Olsson, rick.jones2, xma, gaagaan,
netdev, rdreier, mcarlson, mchan, general, tgraf, randy.dunlap,
sri
In-Reply-To: <20071008.194343.52093065.davem@davemloft.net>
On Mon, Oct 08, 2007 at 07:43:43PM -0700, David Miller wrote:
>
> Right, that's Jamal's recent patch. It looked funny to me too.
Hang on Dave. It was too early in the morning for me :)
I'd forgotten about the QDISC_RUNNING bit which did what the
queue lock did without actually holding the queue lock.
So there is no reordering with or without Jamal's patch.
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
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: David Miller @ 2007-10-09 2:47 UTC (permalink / raw)
To: herbert
Cc: johnpol, jeff, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, hadi, mcarlson, gaagaan, jagana, general,
mchan, tgraf, randy.dunlap, shemminger, kaber, sri
In-Reply-To: <20071009021620.GA14917@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 9 Oct 2007 10:16:20 +0800
> On Mon, Oct 08, 2007 at 10:14:30PM -0400, jamal wrote:
> >
> > You forgot QDISC_RUNNING Dave;-> the above cant happen.
> > Essentially at any one point in time, we are guaranteed that we can have
> > multiple cpus enqueueing but only can be dequeueing (the one that
> > managed to grab QDISC_RUNNING) i.e multiple producers to the qdisc queue
> > but only one consumer. Only the dequeuer has access to the txlock.
>
> Good point. You had me worried for a sec :)
>
> Dave, Jamal's patch is fine as it is and doesn't actually create
> any packet reordering.
Ok, then, I'll un-revert. :-)
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Krishna Kumar2 @ 2007-10-09 3:09 UTC (permalink / raw)
To: hadi
Cc: jagana, johnpol, gaagaan, jeff, Robert.Olsson, kumarkr, rdreier,
peter.p.waskiewicz.jr, mcarlson, Patrick McHardy, netdev, general,
mchan, tgraf, randy.dunlap, sri, shemminger, David Miller,
herbert
In-Reply-To: <1191849444.4352.29.camel@localhost>
J Hadi Salim <j.hadi123@gmail.com> wrote on 10/08/2007 06:47:24 PM:
> two, there should _never_ be any requeueing even if LLTX in the previous
> patches when i supported them; if there is, it is a bug. This is because
> we dont send more than what the driver asked for via xmit_win. So if it
> asked for more than it can handle, that is a bug. If its available space
> changes while we are sending to it, that too is a bug.
Driver might ask for 10 and we send 10, but LLTX driver might fail to get
lock and return TX_LOCKED. I haven't seen your code in greater detail, but
don't you requeue in that case too?
- KK
^ permalink raw reply
* Re: raw PF_PACKET protocol selection
From: Herbert Xu @ 2007-10-09 3:17 UTC (permalink / raw)
To: joakim.tjernlund; +Cc: netdev
In-Reply-To: <1191868569.6682.67.camel@gentoo-jocke.transmode.se>
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> I trying to open my own raw PF_PACKET socket to receive
> pkgs sent to this socket. I can only make ETH_P_ALL protocol
> work, but then I receive all pkgs and I want pkgs with a specific
> protocol type. I have tried lots of ETH_P types and none of them work.
> Naturally I make sure the sender is using the same protocol as my test
> program below. I guess I must be doing something wrong???
Your program works fine here. You did run it as root, right?
Did you try stracing it?
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
^ permalink raw reply
* Re: [patch 1/1] 8021q: transfer dev_id from real device
From: David Miller @ 2007-10-09 3:29 UTC (permalink / raw)
To: braunu; +Cc: netdev, linux-s390
In-Reply-To: <20071008150820.837583000@linux.vnet.ibm.com>
From: Ursula Braun <braunu@de.ibm.com>
Date: Mon, 08 Oct 2007 17:05:05 +0200
> From: Ursula Braun <braunu@de.ibm.com>
>
> A net_device struct provides field dev_id. It is used for
> unique ipv6 generation in case of shared network cards
> (as for the OSA network cards of IBM System z).
> If VLAN devices are built on top of such shared network cards,
> this dev_id information needs to be transferred to the VLAN device.
>
> Signed-off-by: Ursula Braun <braunu@de.ibm.com>
Patch applied, thanks!
^ permalink raw reply
* Re: 2.6.23-rc8-mm2 BUG: register_netdevice() issue as (ab)used by ISDN
From: David Miller @ 2007-10-09 3:37 UTC (permalink / raw)
To: kkeil; +Cc: andi, jeff, isdn4linux, netdev, akpm
In-Reply-To: <20071008155836.GA19932@pingi.kke.suse.de>
From: Karsten Keil <kkeil@suse.de>
Date: Mon, 8 Oct 2007 17:58:36 +0200
> On Sun, Oct 07, 2007 at 07:20:37PM +0200, Andreas Mohr wrote:
> > Hi,
> >
> > On Sun, Oct 07, 2007 at 03:17:24PM +0200, Andreas Mohr wrote:
> > > I thus decided to now try plain 2.6.23-rc8 whether it's "corrupted", too.
> >
> > OK, after 3 hours of compilation the ONLY datapoint I can give right now is:
> > 1 (one) boot on 2.6.23-rc8-mm2 had the BUG in /var/log/messages,
> > 1 (one) boot on 2.6.23-rc8 did NOT have it.
> >
> > The two .config:s (I did a make oldconfig when going back to
> > non-mm2) don't seem to have any relevant differences.
> >
> > I could offer to do more testing on this machine,
> > but this carries a slight risk for me since I (personally) won't have physical
> > access to it for a couple days and some people will kill me if the main
> > internet gateway happened to go down unfixably. ;)
> >
>
> You could try following patch with 2.6.23-rc8-mm2, it change I4L to use
> alloc_netdev().
>
> Signed-off-by: Karsten Keil <kkeil@suse.de>
I've added this patch to net-2.6.24, thanks Karsten!
^ permalink raw reply
* Re: [PATCH][NETNS] Move some code into __init section when CONFIG_NET_NS=n (v2)
From: David Miller @ 2007-10-09 3:38 UTC (permalink / raw)
To: xemul; +Cc: ebiederm, netdev, devel
In-Reply-To: <470A2E81.5050006@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Mon, 08 Oct 2007 17:20:01 +0400
> With the net namespaces many code leaved the __init section,
> thus making the kernel occupy more memory than it did before.
> Since we have a config option that prohibits the namespace
> creation, the functions that initialize/finalize some netns
> stuff are simply not needed and can be freed after the boot.
>
> Currently, this is almost not noticeable, since few calls
> are no longer in __init, but when the namespaces will be
> merged it will be possible to free more code. I propose to
> use the __net_init, __net_exit and __net_initdata "attributes"
> for functions/variables that are not used if the CONFIG_NET_NS
> is not set to save more space in memory.
>
> The exiting functions cannot just reside in the __exit section,
> as noticed by David, since the init section will have
> references on it and the compilation will fail due to modpost
> checks. These references can exist, since the init namespace
> never dies and the exit callbacks are never called. So I
> introduce the __exit_refok attribute just like it is already
> done with the __init_refok.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied, thanks for fixing this up Pavel.
^ permalink raw reply
* Re: [PATCH] [IPv6]: use container_of() macro in fib6_clean_node()
From: David Miller @ 2007-10-09 3:40 UTC (permalink / raw)
To: benjamin.thery; +Cc: netdev
In-Reply-To: <20071008163316.16798.19433.stgit@frecb000701.frec.bull.fr>
From: Benjamin Thery <benjamin.thery@bull.net>
Date: Mon, 08 Oct 2007 18:33:16 +0200
> In ip6_fib.c, fib6_clean_node() casts a fib6_walker_t pointer to
> a fib6_cleaner_t pointer assuming a struct fib6_walker_t (field 'w')
> is the first field in struct fib6_walker_t.
>
> To prevent any future problems that may occur if one day a field
> is inadvertently inserted before the 'w' field in struct fib6_cleaner_t,
> (and to improve readability), this patch uses the container_of() macro.
>
> Patch for net-2.6.24
>
> Signed-off-by: Benjamin Thery <benjamin.thery@bull.net>
Applied, thanks Benjamin.
^ permalink raw reply
* Re: [RFC][NET_SCHED] explict hold dev tx lock
From: Herbert Xu @ 2007-10-09 4:00 UTC (permalink / raw)
To: jamal; +Cc: David Miller, netdev, kaber, dada1, johnpol
In-Reply-To: <1190256183.4818.28.camel@localhost>
On Wed, Sep 19, 2007 at 10:43:03PM -0400, jamal wrote:
>
> [NET_SCHED] explict hold dev tx lock
>
> For N cpus, with full throttle traffic on all N CPUs, funneling traffic
> to the same ethernet device, the devices queue lock is contended by all
> N CPUs constantly. The TX lock is only contended by a max of 2 CPUS.
> In the current mode of operation, after all the work of entering the
> dequeue region, we may endup aborting the path if we are unable to get
> the tx lock and go back to contend for the queue lock. As N goes up,
> this gets worse.
>
> The changes in this patch result in a small increase in performance
> with a 4CPU (2xdual-core) with no irq binding. Both e1000 and tg3
> showed similar behavior;
OK, after waking up a bit more I now have another question :)
Both of the drivers you've tested here are special. Firstly
e1000 is lockless so there is no contention here at all. On
the other hand tg3 doesn't take the TX lock on the clean-up
path unless the queue has been stopped.
In other words both drivers only take the TX lock on xmit so
this patch makes very little difference to them.
What I'm worried about is would we see worse behaviour with
drivers that do all their TX clean-up with the TX lock held
(which would cause qdisc_restart to spin while this is happening)?
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
^ permalink raw reply
* Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile
From: Eliezer Tamir @ 2007-10-09 4:13 UTC (permalink / raw)
To: David Miller; +Cc: jeff, netdev, mchan
In-Reply-To: <20071008.174603.55855492.davem@davemloft.net>
David Miller wrote:
> From: "Eliezer Tamir" <eliezert@broadcom.com>
> Date: Mon, 08 Oct 2007 18:15:17 +0200
>
>> Add bnx2x to Kconfig and Makefile
>
> In a patch submission, the tree should build on every step along the
> way in applying your patches, with any given configuration.
>
> Here, the user (or something automated like "make allmodconfig") can
> select the new config option, but because the driver source hasn't
> been added, the compile will fail.
>
> The idea is that if your patch set really is composed of logically
> seperate changes, you submit them one logical compilable piece at a
> time, and if the first few patches are ok, they could go right in
> whilst we work out issues in later patches. But that's not how
> a new driver is, it is logically one change only.
>
> You go on next to add a foo.c file, and then a foo.h file.
> This makes things even more difficult to review.
>
> Really, for a new driver that doesn't make any generic kernel code
> changes, just submit the whole thing in one shot. It's the only
> reasonable way.
>
> Thank you.
>
Due to the size of the patch I can not post it to the list.
Here is an FTP link.
ftp://Net_sys_anon@ftp1.broadcom.com/bnx2x-0.40.10-net-2.6.24-one.patch.txt
Or if you prefer, I can post it gzipped.
Thanks
Eliezer
^ permalink raw reply
* Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile
From: David Miller @ 2007-10-09 4:29 UTC (permalink / raw)
To: eliezert; +Cc: jeff, netdev, mchan
In-Reply-To: <470AFFE3.100@broadcom.com>
From: "Eliezer Tamir" <eliezert@broadcom.com>
Date: Tue, 09 Oct 2007 06:13:23 +0200
> Due to the size of the patch I can not post it to the list.
Understood
> Here is an FTP link.
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2x-0.40.10-net-2.6.24-one.patch.txt
>
> Or if you prefer, I can post it gzipped.
I took a look at what takes up so much space and it's the firmware and
this register init stuff.
The firmware is unavoidable, but wrt. the register init tables there
appears to be a ton of superfluous information in that header file.
It seem extreme overkill and I would guess it exists purely to
simplify your in-house chip validation. I would really wish you
wouldn't do this as these tables take up unswappable kernel memory.
No other driver goes about initializing the chip registers using
tables like this. Many of them are sequences of nothing but zeros or
the same value repeated over and over! The non-zero values that are
specific are "magic constants" with no macros to describe up the
meaning of the bits being set in these registers.
Huge functions like bnx2x_idle_chk() that just validate register
values with hundreds of lines of C code are not appropriate for
submission into a Linux kernel network driver. Again, this looks
like code that assists you with in-house chip validation.
I'm open to putting self tests into the driver, but this adds so much
bloat it really takes things beyond reasonable.
Please remove this self-test code and compress the register
initialization information, preferrably into foo_hw_init() C functions
as is done traditionally in drivers. It will take up less space,
remove the magic values, and make the hardware easier to understand
for other developers.
This should get the driver under the posting limit of 400K, which to
be honest no driver's should be larger than. Unfortunately that
firmware file is 350K so you won't have much left to work with :-)
What we could do is post the driver without the firmware, so that
people can review the actual C-code by just replying to this posting
and it won't go over the posting size limit.
Thanks!
^ permalink raw reply
* Re: [0/7] IPsec: Preparatory patcheds for async crypto on output
From: Evgeniy Polyakov @ 2007-10-09 5:30 UTC (permalink / raw)
To: David Miller; +Cc: herbert, netdev
In-Reply-To: <20071008.172821.08123508.davem@davemloft.net>
On Mon, Oct 08, 2007 at 05:28:21PM -0700, David Miller (davem@davemloft.net) wrote:
> From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> Date: Mon, 8 Oct 2007 16:20:40 +0400
>
> > Dave, is there any chance it can be accepted for upcoming release?
>
> Does a Russian drink vodka? :-)
I strongly feel there is some dirty trick in this question, but can not
find it... Usually I answer: "is it a rhetorical question?" :)
> I'm certainly willing to entertain the idea.
Great!
--
Evgeniy Polyakov
^ permalink raw reply
* RE: Fixed PHY regression
From: Joakim Tjernlund @ 2007-10-09 5:46 UTC (permalink / raw)
To: 'Jeff Garzik', 'David Miller'; +Cc: netdev
In-Reply-To: <470AC3B2.7090802@garzik.org>
> -----Original Message-----
> From: Jeff Garzik [mailto:jeff@garzik.org]
> Sent: den 9 oktober 2007 01:57
> To: David Miller
> Cc: joakim.tjernlund@transmode.se; netdev@vger.kernel.org
> Subject: Re: Fixed PHY regression
>
> David Miller wrote:
> > From: "Joakim Tjernlund" <joakim.tjernlund@transmode.se>
> > Date: Mon, 8 Oct 2007 20:17:33 +0200
> >
> >> David, any chance the second item, Oops while modprobing
> phy fixed module, in
> >> http://marc.info/?l=linux-kernel&m=119178673421891&w=2
> >> will make it into 2.6.23?
> >>
> >> The fix is now in your tree,
> >>
> http://git.kernel.org/?p=linux/kernel/git/davem/bak-net-2.6.24
.git;a=commit;h=bbb4c0c35a4c2aed5e025b668c8dfc99c5b74cff
> >>
> >> It has been in Andrews tree for a while, then in Garzik
> and now it is in your tree.
> >
> > Jeff, any reason you didn't submit this one for 2.6.23?
>
> Two reasons:
> * despite the subject, after discussion it looked like a problem with
> module release, making "build it into your kernel" a valid workaround
I don't use modules and this does not work for me. Basically
Fixed PHY is broken for me.
Jocke
> * the fix was quite invasive this late in the game
>
> If people still think it should go for 2.6.23, that's cool with me.
>
> Jeff
>
>
>
>
>
^ permalink raw reply
* RE: raw PF_PACKET protocol selection
From: Joakim Tjernlund @ 2007-10-09 6:08 UTC (permalink / raw)
To: 'Herbert Xu'; +Cc: netdev
In-Reply-To: <E1If5b3-00043F-00@gondolin.me.apana.org.au>
> -----Original Message-----
> From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
> Sent: den 9 oktober 2007 05:17
> To: joakim.tjernlund@transmode.se
> Cc: netdev@vger.kernel.org
> Subject: Re: raw PF_PACKET protocol selection
>
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >
> > I trying to open my own raw PF_PACKET socket to receive
> > pkgs sent to this socket. I can only make ETH_P_ALL protocol
> > work, but then I receive all pkgs and I want pkgs with a specific
> > protocol type. I have tried lots of ETH_P types and none of
> them work.
> > Naturally I make sure the sender is using the same protocol
> as my test
> > program below. I guess I must be doing something wrong???
>
> Your program works fine here. You did run it as root, right?
Yes and ETH_P_ALL is the only protocol that prints anything
I am on 2.6.22
> Did you try stracing it?
Just did and now it works, it didn't yesterday :(
But if I change protocol to ETH_P_MOBITEX, I don't get any
pkgs(I did change protocol on sending side too)
>
> 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
>
>
>
^ permalink raw reply
* Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
From: TAKANO Ryousei @ 2007-10-09 6:28 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev, y-kodama
In-Reply-To: <Pine.LNX.4.64.0710081258440.31129@kivilampi-30.cs.helsinki.fi>
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
Date: Mon, 8 Oct 2007 14:11:55 +0300 (EEST)
> On Sun, 7 Oct 2007, TAKANO Ryousei wrote:
>
> > From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> > Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
> > Date: Fri, 5 Oct 2007 13:02:07 +0300 (EEST)
> >
> > > On Thu, 4 Oct 2007, TAKANO Ryousei wrote:
> > >
> > > In case sacktag uses fastpath, this code won't be executed for the skb's
> > > that we would like to check (those with SACKED_RETRANS set, that are
> > > below the fastpath_skb_hint). We will eventually deal with the whole queue
> > > when fastpath_skb_hint gets set to NULL, with the next cumulative ACK that
> > > fully ACKs an skb at the latest. Maybe there's a need for a larger surgery
> > > than this to fix it. I think we need additional field to tcp_sock to avoid
> > > doing a full-walk per ACK:
> >
> > I think the problem occurs in slowpath. For example, in case when the receiver
> > detects and sends back a new SACK block, the sender may fail to detect loss
> > of a retransmitted packet.
>
> ...No, the slow path is very likely to _correct_ the problem! The problem
> occurs because fastpath _skips_ processing of skbs below fastpath_skb_hint
> whereas slowpath starts from tcp_write_queue_head. The retransmitted skbs
> we're interested in reside exactly on that skipped range.
>
Sorry, my explanaton is insufficient.
This problem occurs even if we are in the slowpath.
For example, consider the case when the state of the retransmission queue is
as shown in Fig.1 (http://projects.gtrc.aist.go.jp/gnet/meeting/sack-bug.html).
There are 13 packets in the retransmission queue, P(0) and P(7) are
retransmitted, and P(1) to P(9) except P(7) are already SACKed.
If only a SACK block "SACK(8,11)" is received, P(0) is re-transmitted,
because the ack_seq of P(0) is smaller than the end sequence number (11)
of the SACK block. However, if the ACK packet has SACK(8,11) and SACK(1,3),
these SACK blocks are sorted, and SACK(1,3) is process from P(0) to P(2)
first. In this case, P(0) is not detected as lost because the ack_seq of
P(0) is not smaller than the end sequence number (3) of this SACK block.
Next, SACK(8,11) is scanned from P(3) to P(10). P(0) is not checked here,
and the loss is not detected.
Is it corrent?
Ryousei Takano
^ permalink raw reply
* [PATCH net-2.6.24][trivial] fix inconsistency of terms
From: TAKANO Ryousei @ 2007-10-09 6:32 UTC (permalink / raw)
To: netdev; +Cc: y-kodama
Fix inconsistency of terms:
1) D-SACK
2) F-RTO
Signed-off-by: Ryousei Takano <takano-ryousei@aist.go.jp>
---
Documentation/networking/ip-sysctl.txt | 6 +++---
net/ipv4/tcp_input.c | 16 ++++++++--------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 6ae2fef..829a4d8 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -184,14 +184,14 @@ tcp_frto - INTEGER
F-RTO is an enhanced recovery algorithm for TCP retransmission
timeouts. It is particularly beneficial in wireless environments
where packet loss is typically due to random radio interference
- rather than intermediate router congestion. FRTO is sender-side
+ rather than intermediate router congestion. F-RTO is sender-side
only modification. Therefore it does not require any support from
the peer, but in a typical case, however, where wireless link is
the local access link and most of the data flows downlink, the
- faraway servers should have FRTO enabled to take advantage of it.
+ faraway servers should have F-RTO enabled to take advantage of it.
If set to 1, basic version is enabled. 2 enables SACK enhanced
F-RTO if flow uses SACK. The basic version can be used also when
- SACK is in use though scenario(s) with it exists where FRTO
+ SACK is in use though scenario(s) with it exists where F-RTO
interacts badly with the packet counting of the SACK enabled TCP
flow.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e8c3948..372ff6f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -103,7 +103,7 @@ int sysctl_tcp_abc __read_mostly;
#define FLAG_SLOWPATH 0x100 /* Do not skip RFC checks for window update.*/
#define FLAG_ONLY_ORIG_SACKED 0x200 /* SACKs only non-rexmit sent before RTO */
#define FLAG_SND_UNA_ADVANCED 0x400 /* Snd_una was changed (!= FLAG_DATA_ACKED) */
-#define FLAG_DSACKING_ACK 0x800 /* SACK blocks contained DSACK info */
+#define FLAG_DSACKING_ACK 0x800 /* SACK blocks contained D-SACK info */
#define FLAG_NONHEAD_RETRANS_ACKED 0x1000 /* Non-head rexmitted data was ACKed */
#define FLAG_ACKED (FLAG_DATA_ACKED|FLAG_SYN_ACKED)
@@ -866,7 +866,7 @@ static void tcp_disable_fack(struct tcp_sock *tp)
tp->rx_opt.sack_ok &= ~2;
}
-/* Take a notice that peer is sending DSACKs */
+/* Take a notice that peer is sending D-SACKs */
static void tcp_dsack_seen(struct tcp_sock *tp)
{
tp->rx_opt.sack_ok |= 4;
@@ -1058,7 +1058,7 @@ static void tcp_update_reordering(struct sock *sk, const int metric,
*
* With D-SACK the lower bound is extended to cover sequence space below
* SND.UNA down to undo_marker, which is the last point of interest. Yet
- * again, DSACK block must not to go across snd_una (for the same reason as
+ * again, D-SACK block must not to go across snd_una (for the same reason as
* for the normal SACK blocks, explained above). But there all simplicity
* ends, TCP might receive valid D-SACKs below that. As long as they reside
* fully below undo_marker they do not affect behavior in anyway and can
@@ -1080,7 +1080,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
if (!before(start_seq, tp->snd_nxt))
return 0;
- /* In outstanding window? ...This is valid exit for DSACKs too.
+ /* In outstanding window? ...This is valid exit for D-SACKs too.
* start_seq == snd_una is non-sensical (see comments above)
*/
if (after(start_seq, tp->snd_una))
@@ -1581,7 +1581,7 @@ void tcp_enter_frto(struct sock *sk)
!icsk->icsk_retransmits)) {
tp->prior_ssthresh = tcp_current_ssthresh(sk);
/* Our state is too optimistic in ssthresh() call because cwnd
- * is not reduced until tcp_enter_frto_loss() when previous FRTO
+ * is not reduced until tcp_enter_frto_loss() when previous F-RTO
* recovery has not yet completed. Pattern would be this: RTO,
* Cumulative ACK, RTO (2xRTO for the same segment does not end
* up here twice).
@@ -1760,7 +1760,7 @@ void tcp_enter_loss(struct sock *sk, int how)
tcp_set_ca_state(sk, TCP_CA_Loss);
tp->high_seq = tp->snd_nxt;
TCP_ECN_queue_cwr(tp);
- /* Abort FRTO algorithm if one is in progress */
+ /* Abort F-RTO algorithm if one is in progress */
tp->frto_counter = 0;
}
@@ -1905,7 +1905,7 @@ static int tcp_time_to_recover(struct sock *sk)
struct tcp_sock *tp = tcp_sk(sk);
__u32 packets_out;
- /* Do not perform any recovery during FRTO algorithm */
+ /* Do not perform any recovery during F-RTO algorithm */
if (tp->frto_counter)
return 0;
@@ -2921,7 +2921,7 @@ static int tcp_process_frto(struct sock *sk, int flag)
}
if (tp->frto_counter == 1) {
- /* Sending of the next skb must be allowed or no FRTO */
+ /* Sending of the next skb must be allowed or no F-RTO */
if (!tcp_send_head(sk) ||
after(TCP_SKB_CB(tcp_send_head(sk))->end_seq,
tp->snd_una + tp->snd_wnd)) {
--
1.5.0.6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox