net core use batching

Quoted 10 lines · David Miller

1) A library for transmit load balancing functions, with an interface that can be made visible to userspace. I can write this and test it on real multiqueue hardware.
The whole purpose of this library is to set skb->queue_mapping based upon the load balancing function.
Facilities will be added to handle virtualization port selection based upon destination MAC address as one of the "load balancing" methods.

Groovy.

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.

Quoted 8 lines

2) Switch the default qdisc away from pfifo_fast to a new DRR fifo with load balancing using the code in #1. I think this is kind of in the territory of what Peter said he is working on.
I know this is controversial, but realistically I doubt users benefit at all from the prioritization that pfifo provides. They will, on the other hand, benefit from TX queue load balancing on fast interfaces.

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.

Quoted 4 lines

3) Work on discovering a way to make the locking on transmit as localized to the current thread of execution as possible. Things like RCU and statistic replication, techniques we use widely elsewhere in the stack, begin to come to mind.

Definitely.

Jeff

David Miller

maintainer

2007-10-09 02:46 UTC #21 lore raw

From: Jeff Garzik <[email protected]>
Date: Mon, 08 Oct 2007 22:12:03 -0400

Quoted 5 lines

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.

Quoted 7 lines

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.

Waskiewicz Jr, Peter P

2007-10-09 18:48 UTC #22 lore raw

In reply to Jeff Garzik

Quoted 4 lines

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.

If I understand what you just said, I disagree. If your hardware is running strict prio, you don't want to enforce strict prio in the qdisc layer; performing two layers of QoS is excessive, and may lead to results you don't want. The reason I added the DRR qdisc is for the Si that has its own queueing strategy that is not RR. For Si that implements RR (like e1000), you can either use the DRR qdisc, or if you want to prioritize your flows, use PRIO.

-PJ Waskiewicz
[email protected]

Jeff Garzik

2007-10-09 19:05 UTC #23 lore raw

Quoted 11 lines · Peter P

Quoted 3 lines
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.
If I understand what you just said, I disagree. If your hardware is running strict prio, you don't want to enforce strict prio in the qdisc layer; performing two layers of QoS is excessive, and may lead to results you don't want. The reason I added the DRR qdisc is for the Si that has its own queueing strategy that is not RR. For Si that implements RR (like e1000), you can either use the DRR qdisc, or if you want to prioritize your flows, use PRIO.

A misunderstanding, I think.

To my brain, DaveM's item #2 seemed to assume/require the NIC hardware to balance fairly across hw TX rings, which seemed to preclude the 8139cp/tg3 style of strict-prio hardware. That's what I was responding to.

As long as there is some modular way to fit 8139cp/tg3 style multi-TX into our universe, I'm happy :)

Jeff

Waskiewicz Jr, Peter P

2007-10-09 19:07 UTC #24 lore raw

Quoted 10 lines

A misunderstanding, I think.
To my brain, DaveM's item #2 seemed to assume/require the NIC hardware to balance fairly across hw TX rings, which seemed to preclude the
8139cp/tg3 style of strict-prio hardware. That's what I was responding to.
As long as there is some modular way to fit 8139cp/tg3 style multi-TX into our universe, I'm happy :)

Ah hah. Yes, a misunderstanding on my part. Thanks for the clarification. Methinks more caffeine is required for today...

-PJ

jamal

author

2007-10-09 02:14 UTC #25 lore raw

Quoted 11 lines · David Miller

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.

Quoted 5 lines

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

Herbert Xu

2007-10-09 02:17 UTC #26 lore raw

Quoted 6 lines · jamal

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2007-10-09 02:47 UTC #27 lore raw

From: Herbert Xu <[email protected]>
Date: Tue, 9 Oct 2007 10:16:20 +0800

Quoted 8 lines · jamal

Quoted 6 lines
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.

Quoted 4 lines

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. :-)

Andi Kleen

2007-10-09 16:51 UTC #28 lore raw

Quoted 4 lines · David Miller

2) Switch the default qdisc away from pfifo_fast to a new DRR fifo with load balancing using the code in #1. I think this is kind of in the territory of what Peter said he is working on.

Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid some locking in this critical path.

Quoted 2 lines

I know this is controversial, but realistically I doubt users benefit at all from the prioritization that pfifo provides.

I agree. For most interfaces the priority is probably dubious. Even for DSL the prioritization will be likely usually done in a router these days.

Also for the fast interfaces where we do TSO priority doesn't work very well anyways -- with large packets there is not too much to prioritize.

Quoted 4 lines

3) Work on discovering a way to make the locking on transmit as localized to the current thread of execution as possible. Things like RCU and statistic replication, techniques we use widely elsewhere in the stack, begin to come to mind.

If the data is just passed on to the hardware queue, why is any locking needed at all? (except for the driver locking of course)

-Andi

Stephen Hemminger

2007-10-09 18:22 UTC #29 lore raw

Quoted 26 lines · On 09 Oct 2007 18:51:51 +0200 Andi Kleen <[email protected]> wrote:

Quoted 4 lines · David Miller
2) Switch the default qdisc away from pfifo_fast to a new DRR fifo with load balancing using the code in #1. I think this is kind of in the territory of what Peter said he is working on.
Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid some locking in this critical path.
Quoted 2 lines
I know this is controversial, but realistically I doubt users benefit at all from the prioritization that pfifo provides.
I agree. For most interfaces the priority is probably dubious. Even for DSL the prioritization will be likely usually done in a router these days.
Also for the fast interfaces where we do TSO priority doesn't work very well anyways -- with large packets there is not too much to prioritize.
Quoted 4 lines
3) Work on discovering a way to make the locking on transmit as localized to the current thread of execution as possible. Things like RCU and statistic replication, techniques we use widely elsewhere in the stack, begin to come to mind.

Quoted 4 lines

If the data is just passed on to the hardware queue, why is any locking needed at all? (except for the driver locking of course)
-Andi

I wonder about the whole idea of queueing in general at such high speeds. Given the normal bi-modal distribution of packets, and the predominance of 1500 byte MTU; does it make sense to even have any queueing in software at all?

Signature · 1 line

Stephen Hemminger <[email protected]>

Andi Kleen

2007-10-09 18:30 UTC #30 lore raw

Quoted 4 lines

I wonder about the whole idea of queueing in general at such high speeds. Given the normal bi-modal distribution of packets, and the predominance of 1500 byte MTU; does it make sense to even have any queueing in software at all?

Yes that is my point -- it should just pass it through directly and the driver can then put it into the different per CPU (or per whatever) queues managed by the hardware.

The only thing the qdisc needs to do is to set some bit that says "it is ok to put this into difference queues; don't need strict ordering"

Otherwise if the drivers did that unconditionally they might cause problems with other qdiscs.

This would also require that the driver exports some hint to the upper layer on how large its internal queues are. A device with a short queue would still require pfifo_fast. Long queue devices could just pass through. That again could be a single flag.

-Andi

David Miller

maintainer

2007-10-09 20:43 UTC #31 lore raw

In reply to Andi Kleen

From: Andi Kleen <[email protected]>
Date: 09 Oct 2007 18:51:51 +0200

Quoted 3 lines

Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid some locking in this critical path.

Indeed, I also realized last night that for the default qdiscs we do a lot of stupid useless work. If the queue is a FIFO and the device can take packets, we should send it directly and not stick it into the qdisc at all.

Quoted 2 lines

If the data is just passed on to the hardware queue, why is any locking needed at all? (except for the driver locking of course)

Absolutely.

Our packet scheduler subsystem is great, but by default it should just get out of the way.

Stephen Hemminger

2007-10-09 20:53 UTC #32 lore raw

Quoted 12 lines · On Tue, 09 Oct 2007 13:43:31 -0700 (PDT) David Miller <[email protected]>…

From: Andi Kleen <[email protected]>
Date: 09 Oct 2007 18:51:51 +0200
Quoted 3 lines
Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid some locking in this critical path.
Indeed, I also realized last night that for the default qdiscs we do a lot of stupid useless work. If the queue is a FIFO and the device can take packets, we should send it directly and not stick it into the qdisc at all.

Quoted 7 lines

Quoted 2 lines
If the data is just passed on to the hardware queue, why is any locking needed at all? (except for the driver locking of course)
Absolutely.
Our packet scheduler subsystem is great, but by default it should just get out of the way.

I was thinking why not have a default transmit queue len of 0 like the virtual devices.

Signature · 1 line

Stephen Hemminger <[email protected]>

David Miller

maintainer

2007-10-09 21:22 UTC #33 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 9 Oct 2007 13:53:40 -0700

Quoted 2 lines

I was thinking why not have a default transmit queue len of 0 like the virtual devices.

I'm not so sure.

Even if the device has "huge queues" I still think we need a software queue for when the hardware one backs up.

It is even beneficial to stick with reasonably sized TX queues because it keeps the total resident state accessed by the CPU within the bounds of the L2 cache. If you go past that it actually hurts to make the TX queue larger instead of helps even if it means you never hit back pressure.

jamal

author

2007-10-09 21:56 UTC #34 lore raw

Quoted 2 lines · David Miller

Even if the device has "huge queues" I still think we need a software queue for when the hardware one backs up.

It should be fine to just "pretend" the qdisc exists despite it sitting in the driver and not have s/ware queues at all to avoid all the challenges that qdiscs bring;
if the h/ware queues are full because of link pressure etc, you drop. We drop today when the s/ware queues are full. The driver txmit lock takes place of the qdisc queue lock etc. I am assuming there is still need for that locking. The filter/classification scheme still works as is and select classes which map to rings. tc still works as is etc.

cheers,
jamal

David Miller

maintainer

2007-10-10 00:04 UTC #35 lore raw

From: jamal <[email protected]>
Date: Tue, 09 Oct 2007 17:56:46 -0400

Quoted 5 lines

if the h/ware queues are full because of link pressure etc, you drop. We drop today when the s/ware queues are full. The driver txmit lock takes place of the qdisc queue lock etc. I am assuming there is still need for that locking. The filter/classification scheme still works as is and select classes which map to rings. tc still works as is etc.

I understand your suggestion.

We have to keep in mind, however, that the sw queue right now is 1000 packets. I heavily discourage any driver author to try and use any single TX queue of that size. Which means that just dropping on back pressure might not work so well.

Or it might be perfect and signal TCP to backoff, who knows! :-)

While working out this issue in my mind, it occured to me that we can put the sw queue into the driver as well.

The idea is that the network stack, as in the pure hw queue scheme, unconditionally always submits new packets to the driver. Therefore even if the hw TX queue is full, the driver can still queue to an internal sw queue with some limit (say 1000 for ethernet, as is used now).

When the hw TX queue gains space, the driver self-batches packets from the sw queue to the hw queue.

It sort of obviates the need for mid-level queue batching in the generic networking. Compared to letting the driver self-batch, the mid-level batching approach is pure overhead.

We seem to be sort of all mentioning similar ideas. For example, you can get the above kind of scheme today by using a mid-level queue length of zero, and I believe this idea was mentioned by Stephen Hemminger earlier.

I may experiment with this in the NIU driver.

Andi Kleen

2007-10-10 00:37 UTC #36 lore raw

Quoted 3 lines · David Miller

We have to keep in mind, however, that the sw queue right now is 1000 packets. I heavily discourage any driver author to try and use any single TX queue of that size.

Why would you discourage them?

If 1000 is ok for a software queue why would it not be ok for a hardware queue?

Quoted 4 lines

Which means that just dropping on back
pressure might not work so well.
Or it might be perfect and signal TCP to backoff, who knows! :-)

1000 packets is a lot. I don't have hard data, but gut feeling is less would also do.

And if the hw queues are not enough a better scheme might be to just manage this in the sockets in sendmsg. e.g. provide a wait queue that drivers can wake up and let them block on more queue.

Quoted 9 lines

The idea is that the network stack, as in the pure hw queue scheme, unconditionally always submits new packets to the driver. Therefore even if the hw TX queue is full, the driver can still queue to an internal sw queue with some limit (say 1000 for ethernet, as is used now).
When the hw TX queue gains space, the driver self-batches packets from the sw queue to the hw queue.

I don't really see the advantage over the qdisc in that scheme. It's certainly not simpler and probably more code and would likely also not require less locks (e.g. a currently lockless driver would need a new lock for its sw queue). Also it is unclear to me it would be really any faster.

-Andi

David Miller

maintainer

2007-10-10 00:50 UTC #37 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 10 Oct 2007 02:37:16 +0200

Quoted 9 lines · David Miller

Quoted 3 lines
We have to keep in mind, however, that the sw queue right now is 1000 packets. I heavily discourage any driver author to try and use any single TX queue of that size.
Why would you discourage them?
If 1000 is ok for a software queue why would it not be ok for a hardware queue?

Because with the software queue, you aren't accessing 1000 slots shared with the hardware device which does shared-ownership transactions on those L2 cache lines with the cpu.

Long ago I did a test on gigabit on a cpu with only 256K of L2 cache. Using a smaller TX queue make things go faster, and it's exactly because of these L2 cache effects.

Quoted 2 lines

1000 packets is a lot. I don't have hard data, but gut feeling is less would also do.

I'll try to see how backlogged my 10Gb tests get when a strong sender is sending to a weak receiver.

Quoted 3 lines

And if the hw queues are not enough a better scheme might be to just manage this in the sockets in sendmsg. e.g. provide a wait queue that drivers can wake up and let them block on more queue.

TCP does this already, but it operates in a lossy manner.

Quoted 5 lines

I don't really see the advantage over the qdisc in that scheme. It's certainly not simpler and probably more code and would likely also not require less locks (e.g. a currently lockless driver would need a new lock for its sw queue). Also it is unclear to me it would be really any faster.

You still need a lock to guard hw TX enqueue from hw TX reclaim.

A 256 entry TX hw queue fills up trivially on 1GB and 10GB, but if you increase the size much more performance starts to go down due to L2 cache thrashing.

Andi Kleen

2007-10-10 09:16 UTC #38 lore raw

Quoted 1 line

A 256 entry TX hw queue fills up trivially on 1GB and 10GB, but if you

With TSO really?

Quoted 2 lines

increase the size much more performance starts to go down due to L2 cache thrashing.

Another possibility would be to consider using cache avoidance instructions while updating the TX ring (e.g. write combining on x86)

-Andi

David Miller

maintainer

2007-10-10 09:25 UTC #39 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 10 Oct 2007 11:16:44 +0200

Quoted 3 lines

Quoted 1 line
A 256 entry TX hw queue fills up trivially on 1GB and 10GB, but if you
With TSO really?

Yes.

Quoted 6 lines

Quoted 2 lines
increase the size much more performance starts to go down due to L2 cache thrashing.
Another possibility would be to consider using cache avoidance instructions while updating the TX ring (e.g. write combining on x86)

The chip I was working with at the time (UltraSPARC-IIi) compressed all the linear stores into 64-byte full cacheline transactions via the store buffer.

It's true that it would allocate in the L2 cache on a miss, which is different from your suggestion.

In fact, such a thing might not pan out well, because most of the time you write a single descriptor or two, and that isn't a full cacheline, which means a read/modify/write is the only coherent way to make such a write to RAM.

Sure you could batch, but I'd rather give the chip work to do unless I unequivocably knew I'd have enough pending to fill a cacheline's worth of descriptors. And since you suggest we shouldn't queue in software... :-)

Andi Kleen

2007-10-10 10:23 UTC #40 lore raw

Quoted 3 lines · David Miller

The chip I was working with at the time (UltraSPARC-IIi) compressed all the linear stores into 64-byte full cacheline transactions via the store buffer.

That's a pretty old CPU. Conclusions on more modern ones might be different.

Quoted 4 lines

In fact, such a thing might not pan out well, because most of the time you write a single descriptor or two, and that isn't a full cacheline, which means a read/modify/write is the only coherent way to make such a write to RAM.

x86 WC does R-M-W and is coherent of course. The main difference is just that the result is not cached. When the hardware accesses the cache line then the cache should be also invalidated.

Quoted 4 lines

Sure you could batch, but I'd rather give the chip work to do unless I unequivocably knew I'd have enough pending to fill a cacheline's worth of descriptors. And since you suggest we shouldn't queue in software... :-)

Hmm, it probably would need to be coupled with batched submission if multiple packets are available you're right. Probably not worth doing explicit queueing though.

I suppose it would be an interesting experiment at least.

-Andi

David Miller

maintainer

2007-10-10 10:44 UTC #41 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 10 Oct 2007 12:23:31 +0200

Quoted 6 lines · David Miller

Quoted 3 lines
The chip I was working with at the time (UltraSPARC-IIi) compressed all the linear stores into 64-byte full cacheline transactions via the store buffer.
That's a pretty old CPU. Conclusions on more modern ones might be different.

Cache matters, just scale the numbers.

Quoted 1 line

I suppose it would be an interesting experiment at least.

Absolutely.

I've always gotten very poor results when increasing the TX queue a lot, for example with NIU the point of diminishing returns seems to be in the range of 256-512 TX descriptor entries and this was with 1.6Ghz cpus.

jamal

author

2007-10-10 13:08 UTC #42 lore raw

Quoted 4 lines · David Miller

I've always gotten very poor results when increasing the TX queue a lot, for example with NIU the point of diminishing returns seems to be in the range of 256-512 TX descriptor entries and this was with 1.6Ghz cpus.

Is it interupt per packet? From my experience, you may find interesting results varying tx interupt mitigation parameters in addition to the ring parameters.
Unfortunately when you do that, optimal parameters also depends on packet size. so what may work for 64B, wont work well for 1400B.

cheers,
jamal

David Miller

maintainer

2007-10-10 22:37 UTC #43 lore raw

From: jamal <[email protected]>
Date: Wed, 10 Oct 2007 09:08:48 -0400

Quoted 7 lines · David Miller

Quoted 5 lines
I've always gotten very poor results when increasing the TX queue a lot, for example with NIU the point of diminishing returns seems to be in the range of 256-512 TX descriptor entries and this was with 1.6Ghz cpus.

Quoted 5 lines

Is it interupt per packet? From my experience, you may find interesting results varying tx interupt mitigation parameters in addition to the ring parameters.
Unfortunately when you do that, optimal parameters also depends on packet size. so what may work for 64B, wont work well for 1400B.

No, it was not interrupt per-packet, I was telling the chip to interrupt me every 1/4 of the ring.

Waskiewicz Jr, Peter P

2007-10-10 15:35 UTC #44 lore raw

In reply to Brandeburg, Jesse

Quoted 15 lines

From: Andi Kleen <[email protected]>
Date: Wed, 10 Oct 2007 12:23:31 +0200
Quoted 2 lines · David Miller
Quoted 1 line
The chip I was working with at the time (UltraSPARC-IIi)
compressed
Quoted 1 line
Quoted 1 line
all the linear stores into 64-byte full cacheline
transactions via
Quoted 3 lines
Quoted 1 line
the store buffer.
That's a pretty old CPU. Conclusions on more modern ones
might be different.
Cache matters, just scale the numbers.

Quoted 8 lines

Quoted 1 line
I suppose it would be an interesting experiment at least.
Absolutely.
I've always gotten very poor results when increasing the TX queue a lot, for example with NIU the point of diminishing returns seems to be in the range of 256-512 TX descriptor entries and this was with 1.6Ghz cpus.

We've done similar testing with ixgbe to push maximum descriptor counts, and we lost performance very quickly in the same range you're quoting on NIU.

Cheers,
-PJ Waskiewicz

Andi Kleen

2007-10-10 16:02 UTC #45 lore raw

Quoted 3 lines

We've done similar testing with ixgbe to push maximum descriptor counts, and we lost performance very quickly in the same range you're quoting on NIU.

Did you try it with WC writes to the ring or CLFLUSH?

-Andi

Waskiewicz Jr, Peter P

2007-10-10 16:42 UTC #46 lore raw

Quoted 16 lines

-----Original Message-----
From: Andi Kleen [mailto:[email protected]]
Sent: Wednesday, October 10, 2007 9:02 AM
To: Waskiewicz Jr, Peter P
Cc: David Miller; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching

Quoted 8 lines

Quoted 2 lines
We've done similar testing with ixgbe to push maximum descriptor counts, and we lost performance very quickly in the same
range you're
Quoted 1 line
quoting on NIU.
Did you try it with WC writes to the ring or CLFLUSH?
-Andi

Hmm, I think it might be slightly different, but it still shows queue depth vs. performance. I was actually referring to how many descriptors we can represent a packet with before it becomes a problem wrt performance. This morning I tried to actually push my ixgbe NIC hard enough to come close to filling the ring with packets (384-byte packets), and even on my 8-core Xeon I can't do it. My system can't generate enough I/O to fill the hardware queues before CPUs max out.

-PJ Waskiewicz

Herbert Xu

2007-10-10 09:54 UTC #47 lore raw

In reply to Andi Kleen

Quoted 3 lines · Andi Kleen

Quoted 1 line
A 256 entry TX hw queue fills up trivially on 1GB and 10GB, but if you
With TSO really?

Hardware queues are generally per-page rather than per-skb so it'd fill up quicker than a software queue even with TSO.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Brandeburg, Jesse

2007-10-12 16:09 UTC #48 lore raw

In reply to Andi Kleen

Quoted 8 lines · Andi Kleen

Quoted 2 lines
When the hw TX queue gains space, the driver self-batches packets from the sw queue to the hw queue.
I don't really see the advantage over the qdisc in that scheme. It's certainly not simpler and probably more code and would likely also not require less locks (e.g. a currently lockless driver would need a new lock for its sw queue). Also it is unclear to me it would be really any faster.

related to this comment, does Linux have a lockless (using atomics) singly linked list element? That would be very useful in a driver hot path.

Stephen Hemminger

2007-10-12 17:10 UTC #49 lore raw

Quoted 10 lines · Jesse

Quoted 8 lines · Andi Kleen
Quoted 2 lines
When the hw TX queue gains space, the driver self-batches packets from the sw queue to the hw queue.
I don't really see the advantage over the qdisc in that scheme. It's certainly not simpler and probably more code and would likely also not require less locks (e.g. a currently lockless driver would need a new lock for its sw queue). Also it is unclear to me it would be really any faster.

Quoted 3 lines

related to this comment, does Linux have a lockless (using atomics) singly linked list element? That would be very useful in a driver hot path.

Use RCU? or write a generic version and get it reviewed. You really want someone with knowledge of all the possible barrier impacts to review it.

Signature · 1 line

Stephen Hemminger <[email protected]>

Andi Kleen

2007-10-12 18:29 UTC #50 lore raw

Quoted 3 lines

Use RCU? or write a generic version and get it reviewed. You really want someone with knowledge of all the possible barrier impacts to review it.

I guess he was thinking of using cmpxchg; but we don't support this in portable code.

RCU is not really suitable for this because it assume writing is relatively rare which is definitely not the case for a qdisc. Also general list management with RCU is quite expensive anyways -- it would require a full copy (that is the 'C' in RCU which Linux generally doesn't use at all)

-Andi

Andi Kleen

2007-10-12 18:27 UTC #51 lore raw

Quoted 3 lines

related to this comment, does Linux have a lockless (using atomics) singly linked list element? That would be very useful in a driver hot path.

No; it doesn't. At least not a portable one.
Besides they tend to be not faster anyways because e.g. cmpxchg tends to be as slow as an explicit spinlock.

-Andi

Bill Fink

2007-10-10 16:03 UTC #52 lore raw

Quoted 9 lines · David Miller

From: jamal <[email protected]>
Date: Tue, 09 Oct 2007 17:56:46 -0400
Quoted 5 lines
if the h/ware queues are full because of link pressure etc, you drop. We drop today when the s/ware queues are full. The driver txmit lock takes place of the qdisc queue lock etc. I am assuming there is still need for that locking. The filter/classification scheme still works as is and select classes which map to rings. tc still works as is etc.

Quoted 8 lines

I understand your suggestion.
We have to keep in mind, however, that the sw queue right now is 1000 packets. I heavily discourage any driver author to try and use any single TX queue of that size. Which means that just dropping on back pressure might not work so well.
Or it might be perfect and signal TCP to backoff, who knows! :-)

I can't remember the details anymore, but for 10-GigE, I have encountered cases where I was able to significantly increase TCP performance by increasing the txqueuelen to 10000, which is the setting I now use for any 10-GigE testing.

-Bill

David Miller

maintainer

2007-10-10 22:53 UTC #53 lore raw

From: Bill Fink <[email protected]>
Date: Wed, 10 Oct 2007 12:02:15 -0400

Quoted 7 lines · David Miller

Quoted 6 lines
We have to keep in mind, however, that the sw queue right now is 1000 packets. I heavily discourage any driver author to try and use any single TX queue of that size. Which means that just dropping on back pressure might not work so well.

Quoted 6 lines

Quoted 1 line
Or it might be perfect and signal TCP to backoff, who knows! :-)
I can't remember the details anymore, but for 10-GigE, I have encountered cases where I was able to significantly increase TCP performance by increasing the txqueuelen to 10000, which is the setting I now use for any 10-GigE testing.

For some reason this does not surprise me.

We bumped the ethernet default up to 1000 for gigabit.

Krishna Kumar2

2007-10-11 06:51 UTC #54 lore raw

In reply to Waskiewicz Jr, Peter P

Hi Dave,

David Miller wrote on 10/10/2007 02:13:31 AM:

Quoted 8 lines

Quoted 3 lines
Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid some locking in this critical path.
Indeed, I also realized last night that for the default qdiscs we do a lot of stupid useless work. If the queue is a FIFO and the device can take packets, we should send it directly and not stick it into the qdisc at all.

Since you are talking of how it should be done in the *current* code, I feel LLTX drivers will not work nicely with this.

Actually I was trying this change a couple of weeks back, but felt that doin go would result in out of order packets (skbs present in q which were not sent out for LLTX failure will be sent out only at next net_tx_action, while other skbs are sent ahead).

One option is to first call qdisc_run() and then process this skb, but that is ugly (requeue handling).

However I guess this can be done cleanly once LLTX is removed.

Thanks,

- KK

Jeff Garzik

2007-10-09 01:31 UTC #55 lore raw

In reply to jamal

Quoted 7 lines · jamal

The challenge to deal with is that netdevices, filters, the queues and
scheduler are closely inter-twined. So it is not just the scheduling
region and QDISC_RUNNING. For example, lets pick just the filters
because they are simple to see: You need to attach them to something -
whatever that is, you then need to synchronize against config and
multiple cpus trying to use them. You could: a) replicate them across cpus and only lock on config, but you are wasting RAM then

I think you've pretty much bought into the cost of wasting RAM, when doing multiple TX rings. So logic implies associated costs, like the ones you describe, come along for the ride.

Quoted 4 lines

b) attach them to rings instead of netdevices - but that makes me wonder if those subqueues are now going to become netdevices. This also means you change all user space interfaces to know about subqueues. If you recall this was a major contention in our earlier discussion.

That's definitely a good question, and I honestly don't see any easy solutions.

Multiple net devices makes a -lot- of things easier, with regards to existing infrastructure, but it also imposes potentially annoying administrative burdens: Not only must each interface be set up individually, but the userland apps must be made aware of this unique method of concurrency.

Jeff

Krishna Kumar2

2007-10-09 10:58 UTC #56 lore raw

Hi Peter,

"Waskiewicz Jr, Peter P" <[email protected]> wrote on 10/09/2007 04:03:42 AM:

Quoted 14 lines

Quoted 14 lines
true, that needs some resolution. Heres a hand-waving thought:
Assuming all packets of a specific map end up in the same qdiscn queue, it seems feasible to ask the qdisc scheduler to give us enough packages (ive seen people use that terms to refer to packets) for each hardware ring's available space. With the patches i posted, i do that via
dev->xmit_win that assumes only one view of the driver; essentially a single ring.
If that is doable, then it is up to the driver to say "i have space for 5 in ring[0], 10 in ring[1] 0 in ring[2]" based on what scheduling scheme the driver implements - the dev->blist can stay the same. Its a handwave, so there may be issues there and there could be better ways to handle this.

Quoted 7 lines

Quoted 3 lines
Note: The other issue that needs resolving that i raised earlier was in regards to multiqueue running on multiple cpus servicing different rings concurently.
I can see the qdisc being modified to send batches per queue_mapping. This shouldn't be too difficult, and if we had the xmit_win per queue (in the subqueue struct like Dave pointed out).

I hope my understanding of multiqueue is correct for this mail to make sense :-)

Isn't it enough that the multiqueue+batching drivers handle skbs belonging to different queue's themselves, instead of qdisc having to figure that out? This will reduce costs for most skbs that are neither batched nor sent to multiqueue devices.

Eg, driver can keep processing skbs and put to the correct tx_queue as long as mapping remains the same. If the mapping changes, it posts earlier skbs (with the correct lock) and then iterates for the other skbs that have the next different mapping, and so on.

(This is required only if driver is supposed to transmit >1 skb in one call, otherwise it is not an issue)

Alternatively, supporting drivers could return a different code on mapping change, like: NETDEV_TX_MAPPING_CHANGED (for batching only) so that qdisc_run() could retry. Would that work?

Secondly having xmit_win per queue: would it help in multiple skb case? Currently there is no way to tell qdisc to dequeue skbs from a particular band - it returns skb from highest priority band.

thanks,

- KK

David Miller

maintainer

2007-10-09 11:02 UTC #57 lore raw

From: Krishna Kumar2 <[email protected]>
Date: Tue, 9 Oct 2007 16:28:27 +0530

Quoted 9 lines

Isn't it enough that the multiqueue+batching drivers handle skbs belonging to different queue's themselves, instead of qdisc having to figure that out? This will reduce costs for most skbs that are neither batched nor sent to multiqueue devices.
Eg, driver can keep processing skbs and put to the correct tx_queue as long as mapping remains the same. If the mapping changes, it posts earlier skbs (with the correct lock) and then iterates for the other skbs that have the next different mapping, and so on.

The complexity in most of these suggestions is beginning to drive me a bit crazy :-)

This should be the simplest thing in the world, when TX queue has space, give it packets. Period.

When I hear suggestions like "have the driver pick the queue in ->hard_start_xmit() and return some special status if the queue becomes different"..... you know, I really begin to wonder :-)

If we have to go back, get into the queueing layer locks, have these special cases, and whatnot, what's the point?

This code should eventually be able to run lockless all the way to the TX queue handling code of the driver. The queueing code should know what TX queue the packet will be bound for, and always precisely invoke the driver in a state where the driver can accept the packet.

Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.

Krishna Kumar2

2007-10-09 11:20 UTC #58 lore raw

Hi Dave,

David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:

Quoted 16 lines

Quoted 9 lines
Isn't it enough that the multiqueue+batching drivers handle skbs belonging to different queue's themselves, instead of qdisc having to figure that out? This will reduce costs for most skbs that are neither batched nor sent to multiqueue devices.
Eg, driver can keep processing skbs and put to the correct tx_queue as long as mapping remains the same. If the mapping changes, it posts earlier skbs (with the correct lock) and then iterates for the other skbs that have the next different mapping, and so on.
The complexity in most of these suggestions is beginning to drive me a bit crazy :-)
This should be the simplest thing in the world, when TX queue has space, give it packets. Period.

Quoted 6 lines

When I hear suggestions like "have the driver pick the queue in ->hard_start_xmit() and return some special status if the queue becomes different"..... you know, I really begin to wonder :-)
If we have to go back, get into the queueing layer locks, have these special cases, and whatnot, what's the point?

I understand your point, but the qdisc code itself needs almost no change, as small as:

qdisc_restart()

{
      ...
      case NETDEV_TX_MAPPING_CHANGED:
      /*
       * Driver sent some skbs from one mapping, and found others
       * are for different queue_mapping. Try again.
       */
      ret = 1; /* guaranteed to have atleast 1 skb in batch list */
      break;
      ...
}

Alternatively if the driver does all the dirty work, qdisc needs no change at all. However, I am not sure if this addresses all the concerns raised by you, Peter, Jamal, others.

Quoted 4 lines

This code should eventually be able to run lockless all the way to the TX queue handling code of the driver. The queueing code should know what TX queue the packet will be bound for, and always precisely invoke the driver in a state where the driver can accept the packet.

This sounds like a good idea :)

I need to think more on this, esp as my batching sends multiple skbs of possibly different mappings to device, and those skbs stay in batch list if driver couldn't send them out.

thanks,

- KK

Krishna Kumar2

2007-10-09 11:21 UTC #59 lore raw

David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:

Quoted 2 lines

Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.

Great, this will make life easy. Any idea how long that would take? It seems simple enough to do.

thanks,

- KK

David Miller

maintainer

2007-10-09 11:24 UTC #60 lore raw

From: Krishna Kumar2 <[email protected]>
Date: Tue, 9 Oct 2007 16:51:14 +0530

Quoted 7 lines

David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:
Quoted 2 lines
Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.
Great, this will make life easy. Any idea how long that would take? It seems simple enough to do.

I'd say we can probably try to get rid of it in 2.6.25, this is assuming we get driver authors to cooperate and do the conversions or alternatively some other motivated person.

I can just threaten to do them all and that should get the driver maintainers going :-)

Jeff Garzik

2007-10-09 12:44 UTC #61 lore raw

Quoted 10 lines · David Miller

From: Krishna Kumar2 <[email protected]>
Date: Tue, 9 Oct 2007 16:51:14 +0530
Quoted 6 lines
David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:
Quoted 2 lines
Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.
Great, this will make life easy. Any idea how long that would take? It seems simple enough to do.

Quoted 6 lines

I'd say we can probably try to get rid of it in 2.6.25, this is assuming we get driver authors to cooperate and do the conversions or alternatively some other motivated person.
I can just threaten to do them all and that should get the driver maintainers going :-)

What, like this? :)

Jeff

 drivers/net/atl1/atl1_main.c   |   16 +++++-----------
 drivers/net/chelsio/cxgb2.c    |    1 -
 drivers/net/chelsio/sge.c      |   20 +++++++++-----------
 drivers/net/e1000/e1000_main.c |    6 +-----
 drivers/net/ixgb/ixgb_main.c   |   24 ------------------------
 drivers/net/pasemi_mac.c       |    2 +-
 drivers/net/rionet.c           |   19 +++++++------------
 drivers/net/spider_net.c       |    2 +-
 drivers/net/sungem.c           |   17 ++++++-----------
 drivers/net/tehuti.c           |   12 +-----------
 drivers/net/tehuti.h           |    3 +--
 11 files changed, 32 insertions(+), 90 deletions(-)

drivers/net/atl1/atl1_main.c +5 ~~-11~~

index 4c728f1..03e94fe 100644
@@ -1665,10 +1665,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 
 	len -= skb->data_len;
 
-	if (unlikely(skb->len == 0)) {
-		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
-	}
+	WARN_ON(skb->len == 0);
 
 	param.data = 0;
 	param.tso.tsopu = 0;

@@ -1703,11 +1700,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		}
 	}
 
-	if (!spin_trylock_irqsave(&adapter->lock, flags)) {
-		/* Can't get lock - tell upper layer to requeue */
-		dev_printk(KERN_DEBUG, &adapter->pdev->dev, "tx locked\n");
-		return NETDEV_TX_LOCKED;
-	}
+	spin_lock_irqsave(&adapter->lock, flags);
 
 	if (atl1_tpd_avail(&adapter->tpd_ring) < count) {
 		/* not enough descriptors */

@@ -1749,8 +1742,11 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	atl1_tx_map(adapter, skb, 1 == val);
 	atl1_tx_queue(adapter, count, &param);
 	netdev->trans_start = jiffies;
+
 	spin_unlock_irqrestore(&adapter->lock, flags);
+
 	atl1_update_mailbox(adapter);
+
 	return NETDEV_TX_OK;
 }

@@ -2301,8 +2297,6 @@ static int __devinit atl1_probe(struct pci_dev *pdev,
 	 */
 	/* netdev->features |= NETIF_F_TSO; */
 
-	netdev->features |= NETIF_F_LLTX;
-
 	/*
 	 * patch for some L1 of old version,
 	 * the final version of L1 may not need these

drivers/net/chelsio/cxgb2.c +0 -1

index 2dbf8dc..0aba7e7 100644
@@ -1084,7 +1084,6 @@ static int __devinit init_one(struct pci_dev *pdev,
 		netdev->mem_end = mmio_start + mmio_len - 1;
 		netdev->priv = adapter;
 		netdev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
-		netdev->features |= NETIF_F_LLTX;
 
 		adapter->flags |= RX_CSUM_ENABLED | TCP_CSUM_CAPABLE;
 		if (pci_using_dac)

drivers/net/chelsio/sge.c +9 ~~-11~~

index ffa7e64..84f5869 100644
@@ -1739,8 +1739,7 @@ static int t1_sge_tx(struct sk_buff *skb, struct adapter *adapter,
 	struct cmdQ *q = &sge->cmdQ[qid];
 	unsigned int credits, pidx, genbit, count, use_sched_skb = 0;
 
-	if (!spin_trylock(&q->lock))
-		return NETDEV_TX_LOCKED;
+	spin_lock(&q->lock);
 
 	reclaim_completed_tx(sge, q);

@@ -1817,12 +1816,12 @@ use_sched:
 	}
 
 	if (use_sched_skb) {
-		if (spin_trylock(&q->lock)) {
-			credits = q->size - q->in_use;
-			skb = NULL;
-			goto use_sched;
-		}
+		spin_lock(&q->lock);
+		credits = q->size - q->in_use;
+		skb = NULL;
+		goto use_sched;
 	}
+
 	return NETDEV_TX_OK;
 }

@@ -1977,13 +1976,12 @@ static void sge_tx_reclaim_cb(unsigned long data)
 	for (i = 0; i < SGE_CMDQ_N; ++i) {
 		struct cmdQ *q = &sge->cmdQ[i];
 
-		if (!spin_trylock(&q->lock))
-			continue;
+		spin_lock(&q->lock);
 
 		reclaim_completed_tx(sge, q);
-		if (i == 0 && q->in_use) {    /* flush pending credits */
+		if (i == 0 && q->in_use)	/* flush pending credits */
 			writel(F_CMDQ0_ENABLE, sge->adapter->regs + A_SG_DOORBELL);
-		}
+
 		spin_unlock(&q->lock);
 	}
 	mod_timer(&sge->tx_reclaim_timer, jiffies + TX_RECLAIM_PERIOD);

drivers/net/e1000/e1000_main.c +1 -5

index 10505de..b64b03b 100644
@@ -992,8 +992,6 @@ e1000_probe(struct pci_dev *pdev,
 	if (pci_using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;
 
-	netdev->features |= NETIF_F_LLTX;
-
 	adapter->en_mng_pt = e1000_enable_mng_pass_thru(&adapter->hw);
 
 	/* initialize eeprom parameters */

@@ -3368,9 +3366,7 @@ e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	    (adapter->hw.mac_type == e1000_82573))
 		e1000_transfer_dhcp_info(adapter, skb);
 
-	if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
-		/* Collision - tell upper layer to requeue */
-		return NETDEV_TX_LOCKED;
+	spin_lock_irqsave(&tx_ring->tx_lock, flags);
 
 	/* need: count + 2 desc gap to keep tail from touching
 	 * head, otherwise try next time */

drivers/net/ixgb/ixgb_main.c +0 ~~-24~~

index d444de5..c04b59b 100644
@@ -449,9 +449,6 @@ ixgb_probe(struct pci_dev *pdev,
 			   NETIF_F_HW_VLAN_RX |
 			   NETIF_F_HW_VLAN_FILTER;
 	netdev->features |= NETIF_F_TSO;
-#ifdef NETIF_F_LLTX
-	netdev->features |= NETIF_F_LLTX;
-#endif
 
 	if(pci_using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;

@@ -1455,16 +1452,7 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		return 0;
 	}
 
-#ifdef NETIF_F_LLTX
-	local_irq_save(flags);
-	if (!spin_trylock(&adapter->tx_lock)) {
-		/* Collision - tell upper layer to requeue */
-		local_irq_restore(flags);
-		return NETDEV_TX_LOCKED;
-	}
-#else
 	spin_lock_irqsave(&adapter->tx_lock, flags);
-#endif
 
 	if (unlikely(ixgb_maybe_stop_tx(netdev, &adapter->tx_ring,
                      DESC_NEEDED))) {

@@ -1473,9 +1461,7 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		return NETDEV_TX_BUSY;
 	}
 
-#ifndef NETIF_F_LLTX
 	spin_unlock_irqrestore(&adapter->tx_lock, flags);
-#endif
 
 	if(adapter->vlgrp && vlan_tx_tag_present(skb)) {
 		tx_flags |= IXGB_TX_FLAGS_VLAN;

@@ -1487,9 +1473,6 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	tso = ixgb_tso(adapter, skb);
 	if (tso < 0) {
 		dev_kfree_skb_any(skb);
-#ifdef NETIF_F_LLTX
-		spin_unlock_irqrestore(&adapter->tx_lock, flags);
-#endif
 		return NETDEV_TX_OK;
 	}

@@ -1503,13 +1486,6 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 
 	netdev->trans_start = jiffies;
 
-#ifdef NETIF_F_LLTX
-	/* Make sure there is space in the ring for the next send. */
-	ixgb_maybe_stop_tx(netdev, &adapter->tx_ring, DESC_NEEDED);
-
-	spin_unlock_irqrestore(&adapter->tx_lock, flags);
-
-#endif
 	return NETDEV_TX_OK;
 }

drivers/net/pasemi_mac.c +1 -1

index 9f9a421..78f939b 100644
@@ -1352,7 +1352,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	netif_napi_add(dev, &mac->napi, pasemi_mac_poll, 64);
 
-	dev->features = NETIF_F_HW_CSUM | NETIF_F_LLTX | NETIF_F_SG;
+	dev->features = NETIF_F_HW_CSUM | NETIF_F_SG;
 
 	/* These should come out of the device tree eventually */
 	mac->dma_txch = index;

drivers/net/rionet.c +7 ~~-12~~

index e7fd08a..cd2d25c 100644
@@ -180,11 +180,7 @@ static int rionet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u16 destid;
 	unsigned long flags;
 
-	local_irq_save(flags);
-	if (!spin_trylock(&rnet->tx_lock)) {
-		local_irq_restore(flags);
-		return NETDEV_TX_LOCKED;
-	}
+	spin_lock_irqsave(&rnet->tx_lock, flags);
 
 	if ((rnet->tx_cnt + 1) > RIONET_TX_RING_SIZE) {
 		netif_stop_queue(ndev);

@@ -259,7 +255,7 @@ static void rionet_outb_msg_event(struct rio_mport *mport, void *dev_id, int mbo
 	struct net_device *ndev = dev_id;
 	struct rionet_private *rnet = ndev->priv;
 
-	spin_lock(&rnet->lock);
+	spin_lock(&rnet->tx_lock);
 
 	if (netif_msg_intr(rnet))
 		printk(KERN_INFO

@@ -278,7 +274,7 @@ static void rionet_outb_msg_event(struct rio_mport *mport, void *dev_id, int mbo
 	if (rnet->tx_cnt < RIONET_TX_RING_SIZE)
 		netif_wake_queue(ndev);
 
-	spin_unlock(&rnet->lock);
+	spin_unlock(&rnet->tx_lock);
 }
 
 static int rionet_open(struct net_device *ndev)

@@ -420,10 +416,10 @@ static void rionet_set_msglevel(struct net_device *ndev, u32 value)
 }
 
 static const struct ethtool_ops rionet_ethtool_ops = {
-	.get_drvinfo = rionet_get_drvinfo,
-	.get_msglevel = rionet_get_msglevel,
-	.set_msglevel = rionet_set_msglevel,
-	.get_link = ethtool_op_get_link,
+	.get_drvinfo	= rionet_get_drvinfo,
+	.get_msglevel	= rionet_get_msglevel,
+	.set_msglevel	= rionet_set_msglevel,
+	.get_link	= ethtool_op_get_link,
 };
 
 static int rionet_setup_netdev(struct rio_mport *mport)

@@ -461,7 +457,6 @@ static int rionet_setup_netdev(struct rio_mport *mport)
 	ndev->hard_start_xmit = &rionet_start_xmit;
 	ndev->stop = &rionet_close;
 	ndev->mtu = RIO_MAX_MSG_SIZE - 14;
-	ndev->features = NETIF_F_LLTX;
 	SET_ETHTOOL_OPS(ndev, &rionet_ethtool_ops);
 
 	spin_lock_init(&rnet->lock);

drivers/net/spider_net.c +1 -1

index fab055f..22193a9 100644
@@ -2329,7 +2329,7 @@ spider_net_setup_netdev(struct spider_net_card *card)
 
 	spider_net_setup_netdev_ops(netdev);
 
-	netdev->features = NETIF_F_IP_CSUM | NETIF_F_LLTX;
+	netdev->features = NETIF_F_IP_CSUM;
 	/* some time: NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX |
 	 *		NETIF_F_HW_VLAN_FILTER */

drivers/net/sungem.c +6 ~~-11~~

index 53b8344..6aaf56d 100644
@@ -932,7 +932,6 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct gem *gp = dev->priv;
-	unsigned long flags;
 
 	/* Swallow interrupts when shutting the chip down, though
 	 * that shouldn't happen, we should have done free_irq() at

@@ -941,14 +940,14 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
 	if (!gp->running)
 		return IRQ_HANDLED;
 
-	spin_lock_irqsave(&gp->lock, flags);
+	spin_lock(&gp->lock);
 
 	if (netif_rx_schedule_prep(dev, &gp->napi)) {
 		u32 gem_status = readl(gp->regs + GREG_STAT);
 
 		if (gem_status == 0) {
 			napi_enable(&gp->napi);
-			spin_unlock_irqrestore(&gp->lock, flags);
+			spin_unlock(&gp->lock);
 			return IRQ_NONE;
 		}
 		gp->status = gem_status;

@@ -956,7 +955,7 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
 		__netif_rx_schedule(dev, &gp->napi);
 	}
 
-	spin_unlock_irqrestore(&gp->lock, flags);
+	spin_unlock(&gp->lock);
 
 	/* If polling was disabled at the time we received that
 	 * interrupt, we may return IRQ_HANDLED here while we

@@ -1031,12 +1030,8 @@ static int gem_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			(csum_stuff_off << 21));
 	}
 
-	local_irq_save(flags);
-	if (!spin_trylock(&gp->tx_lock)) {
-		/* Tell upper layer to requeue */
-		local_irq_restore(flags);
-		return NETDEV_TX_LOCKED;
-	}
+	spin_lock_irqsave(&gp->tx_lock, flags);
+
 	/* We raced with gem_do_stop() */
 	if (!gp->running) {
 		spin_unlock_irqrestore(&gp->tx_lock, flags);

@@ -3160,7 +3155,7 @@ static int __devinit gem_init_one(struct pci_dev *pdev,
 			gp->phy_mii.def ? gp->phy_mii.def->name : "no");
 
 	/* GEM can do it all... */
-	dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_LLTX;
+	dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
 	if (pci_using_dac)
 		dev->features |= NETIF_F_HIGHDMA;

drivers/net/tehuti.c +1 ~~-11~~

index 2483431..e0da1e0 100644
@@ -1606,7 +1606,6 @@ static inline int bdx_tx_space(struct bdx_priv *priv)
  * o NETDEV_TX_BUSY 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 NETDEV_TX_LOCKED Locking failed, please retry quickly.
  */
 static int bdx_tx_transmit(struct sk_buff *skb, struct net_device *ndev)
 {

@@ -1624,13 +1623,7 @@ static int bdx_tx_transmit(struct sk_buff *skb, struct net_device *ndev)
 	unsigned long flags;
 
 	ENTER;
-	local_irq_save(flags);
-	if (!spin_trylock(&priv->tx_lock)) {
-		local_irq_restore(flags);
-		DBG("%s[%s]: TX locked, returning NETDEV_TX_LOCKED\n",
-		    BDX_DRV_NAME, ndev->name);
-		return NETDEV_TX_LOCKED;
-	}
+	spin_lock_irqsave(&priv->tx_lock, flags);
 
 	/* build tx descriptor */
 	BDX_ASSERT(f->m.wptr >= f->m.memsz);	/* started with valid wptr */

@@ -2048,9 +2041,6 @@ bdx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		 * between transmit and TX irq cleanup.  In addition
 		 * set multicast list callback has to use priv->tx_lock.
 		 */
-#ifdef BDX_LLTX
-		ndev->features |= NETIF_F_LLTX;
-#endif
 		spin_lock_init(&priv->tx_lock);
 
 		/*bdx_hw_reset(priv); */

drivers/net/tehuti.h +1 -2

index efd170f..c55f69a 100644
@@ -35,7 +35,6 @@
 /* Compile Time Switches */
 /* start */
 #define BDX_TSO
-#define BDX_LLTX
 #define BDX_DELAY_WPTR
 /* #define BDX_MSI */
 /* end */

@@ -270,7 +269,7 @@ struct bdx_priv {
 	int tx_update_mark;
 	int tx_noupd;
 #endif
-	spinlock_t tx_lock;	/* NETIF_F_LLTX mode */
+	spinlock_t tx_lock;
 
 	/* rarely used */
 	u8 port;

Herbert Xu

2007-10-09 12:55 UTC #62 lore raw

Quoted 6 lines · Jeff Garzik

Quoted 3 lines · David Miller
I can just threaten to do them all and that should get the driver maintainers going :-)
What, like this? :)

Awsome :)

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Jeff Garzik

2007-10-09 13:00 UTC #63 lore raw

Quoted 7 lines · Herbert Xu

Quoted 4 lines · Jeff Garzik
Quoted 2 lines · David Miller
I can just threaten to do them all and that should get the driver maintainers going :-)
What, like this? :)
Awsome :)

Note my patch is just to get the maintainers going. :) I'm not going to commit that, since I don't have any way to test any of the drivers I touched (but I wouldn't scream if it appeared in net-2.6.24 either)

Jeff

David Miller

maintainer

2007-10-09 20:14 UTC #64 lore raw

In reply to Jeff Garzik

From: Jeff Garzik <[email protected]>
Date: Tue, 09 Oct 2007 08:44:25 -0400

Quoted 11 lines

Quoted 10 lines · David Miller
From: Krishna Kumar2 <[email protected]>
Date: Tue, 9 Oct 2007 16:51:14 +0530
Quoted 6 lines
David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:
Quoted 2 lines
Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.
Great, this will make life easy. Any idea how long that would take? It seems simple enough to do.

Quoted 8 lines

Quoted 6 lines
I'd say we can probably try to get rid of it in 2.6.25, this is assuming we get driver authors to cooperate and do the conversions or alternatively some other motivated person.
I can just threaten to do them all and that should get the driver maintainers going :-)
What, like this? :)

Thanks, but it's probably going to need some corrections and/or an audit.

If you unconditionally take those locks in the transmit function, there is probably an ABBA deadlock elsewhere in the driver now, most likely in the TX reclaim processing, and you therefore need to handle that too.

Jeff Garzik

2007-10-09 20:20 UTC #65 lore raw

Quoted 17 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 09 Oct 2007 08:44:25 -0400
Quoted 14 lines
Quoted 13 lines · David Miller
From: Krishna Kumar2 <[email protected]>
Date: Tue, 9 Oct 2007 16:51:14 +0530
Quoted 6 lines
David Miller <[email protected]> wrote on 10/09/2007 04:32:55 PM:
Quoted 2 lines
Ignore LLTX, it sucks, it was a big mistake, and we will get rid of it.
Great, this will make life easy. Any idea how long that would take? It seems simple enough to do.
I'd say we can probably try to get rid of it in 2.6.25, this is assuming we get driver authors to cooperate and do the conversions or alternatively some other motivated person.

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
I can just threaten to do them all and that should get the driver maintainers going :-)
What, like this? :)
Thanks, but it's probably going to need some corrections and/or an audit.

I would be happy if someone wanted to audit that patch.

Quoted 4 lines

If you unconditionally take those locks in the transmit function, there is probably an ABBA deadlock elsewhere in the driver now, most likely in the TX reclaim processing, and you therefore need to handle that too.

And I most certainly checked the relevant transmit paths and other locking to make sure lock ordering was correct.

Jeff

David Miller

maintainer

2007-10-09 21:25 UTC #66 lore raw

From: Jeff Garzik <[email protected]>
Date: Tue, 09 Oct 2007 16:20:14 -0400

Quoted 8 lines

Quoted 4 lines · David Miller
If you unconditionally take those locks in the transmit function, there is probably an ABBA deadlock elsewhere in the driver now, most likely in the TX reclaim processing, and you therefore need to handle that too.
And I most certainly checked the relevant transmit paths and other locking to make sure lock ordering was correct.

Awesome.

Roland Dreier

2007-10-09 20:22 UTC #67 lore raw

Quoted 6 lines

I'd say we can probably try to get rid of it in 2.6.25, this is assuming we get driver authors to cooperate and do the conversions or alternatively some other motivated person.
I can just threaten to do them all and that should get the driver maintainers going :-)

I can definitely kill LLTX for IPoIB by 2.6.25 and I just added it to my TODO list so I don't forget.

In fact if 2.6.23 drags on long enough I may do it for 2.6.24....

David Miller

maintainer

2007-10-09 20:51 UTC #68 lore raw

From: Roland Dreier <[email protected]>
Date: Tue, 09 Oct 2007 13:22:44 -0700

Quoted 4 lines

I can definitely kill LLTX for IPoIB by 2.6.25 and I just added it to my TODO list so I don't forget.
In fact if 2.6.23 drags on long enough I may do it for 2.6.24....

Before you add new entries to your list, how is that ibm driver NAPI conversion coming along? :-)

Right now that's a more pressing task to complete.

Roland Dreier

2007-10-09 21:40 UTC #69 lore raw

Quoted 2 lines

Before you add new entries to your list, how is that ibm driver NAPI conversion coming along? :-)

I still haven't done much. OK, I will try to get my board booting again this week.

Roland Dreier

2007-10-09 22:44 UTC #70 lore raw

Quoted 2 lines

Before you add new entries to your list, how is that ibm driver NAPI conversion coming along? :-)

OK, thanks for the kick in the pants, I have a couple of patches for net-2.6.24 coming (including an unrelated trivial warning fix for IPoIB).

- R.

Roland Dreier

2007-10-09 22:46 UTC #71 lore raw

PATCH 1/4 IPoIB: Fix unused variable warning

The conversion to use netdevice internal stats left an unused variable in ipoib_neigh_free(), since there's no longer any reason to get netdev_priv() in order to increment dropped packets. Delete the unused priv variable.

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -1

index 6b1b4b2..855c9de 100644
@@ -854,7 +854,6 @@ struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour)
 
 void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh)
 {
-	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	struct sk_buff *skb;
 	*to_ipoib_neigh(neigh->neighbour) = NULL;
 	while ((skb = __skb_dequeue(&neigh->queue))) {

Roland Dreier

2007-10-09 22:47 UTC #72 lore raw

PATCH 2/4 ibm_emac: Convert to use napi_struct independent of struct net_device

Commit da3dedd9 ("[NET]: Make NAPI polling independent of struct net_device objects.") changed the interface to NAPI polling. Fix up the ibm_emac driver so that it works with this new interface. This is actually a nice cleanup because ibm_emac is one of the drivers that wants to have multiple NAPI structures for a single net_device.

Tested with the internal MAC of a PowerPC 440SPe SoC with an AMCC 'Yucca' evaluation board.

---
 drivers/net/ibm_emac/ibm_emac_mal.c |   48 ++++++++++++----------------------
 drivers/net/ibm_emac/ibm_emac_mal.h |    2 +-
 include/linux/netdevice.h           |   10 +++++++
 3 files changed, 28 insertions(+), 32 deletions(-)

drivers/net/ibm_emac/ibm_emac_mal.c +17 ~~-31~~

index cabd984..cc3ddc9 100644
@@ -207,10 +207,10 @@ static irqreturn_t mal_serr(int irq, void *dev_instance)
 
 static inline void mal_schedule_poll(struct ibm_ocp_mal *mal)
 {
-	if (likely(netif_rx_schedule_prep(&mal->poll_dev))) {
+	if (likely(napi_schedule_prep(&mal->napi))) {
 		MAL_DBG2("%d: schedule_poll" NL, mal->def->index);
 		mal_disable_eob_irq(mal);
-		__netif_rx_schedule(&mal->poll_dev);
+		__napi_schedule(&mal->napi);
 	} else
 		MAL_DBG2("%d: already in poll" NL, mal->def->index);
 }

@@ -273,11 +273,11 @@ static irqreturn_t mal_rxde(int irq, void *dev_instance)
 	return IRQ_HANDLED;
 }
 
-static int mal_poll(struct net_device *ndev, int *budget)
+static int mal_poll(struct napi_struct *napi, int budget)
 {
-	struct ibm_ocp_mal *mal = ndev->priv;
+	struct ibm_ocp_mal *mal = container_of(napi, struct ibm_ocp_mal, napi);
 	struct list_head *l;
-	int rx_work_limit = min(ndev->quota, *budget), received = 0, done;
+	int received = 0;
 
 	MAL_DBG2("%d: poll(%d) %d ->" NL, mal->def->index, *budget,
 		 rx_work_limit);

@@ -295,38 +295,34 @@ static int mal_poll(struct net_device *ndev, int *budget)
 	list_for_each(l, &mal->poll_list) {
 		struct mal_commac *mc =
 		    list_entry(l, struct mal_commac, poll_list);
-		int n = mc->ops->poll_rx(mc->dev, rx_work_limit);
+		int n = mc->ops->poll_rx(mc->dev, budget);
 		if (n) {
 			received += n;
-			rx_work_limit -= n;
-			if (rx_work_limit <= 0) {
-				done = 0;
+			budget -= n;
+			if (budget <= 0)
 				goto more_work;	// XXX What if this is the last one ?
-			}
 		}
 	}
 
 	/* We need to disable IRQs to protect from RXDE IRQ here */
 	local_irq_disable();
-	__netif_rx_complete(ndev);
+	__napi_complete(napi);
 	mal_enable_eob_irq(mal);
 	local_irq_enable();
 
-	done = 1;
-
 	/* Check for "rotting" packet(s) */
 	list_for_each(l, &mal->poll_list) {
 		struct mal_commac *mc =
 		    list_entry(l, struct mal_commac, poll_list);
 		if (unlikely(mc->ops->peek_rx(mc->dev) || mc->rx_stopped)) {
 			MAL_DBG2("%d: rotting packet" NL, mal->def->index);
-			if (netif_rx_reschedule(ndev, received))
+			if (napi_reschedule(napi))
 				mal_disable_eob_irq(mal);
 			else
 				MAL_DBG2("%d: already in poll list" NL,
 					 mal->def->index);
 
-			if (rx_work_limit > 0)
+			if (budget > 0)
 				goto again;
 			else
 				goto more_work;

@@ -335,12 +331,8 @@ static int mal_poll(struct net_device *ndev, int *budget)
 	}
 
       more_work:
-	ndev->quota -= received;
-	*budget -= received;
-
-	MAL_DBG2("%d: poll() %d <- %d" NL, mal->def->index, *budget,
-		 done ? 0 : 1);
-	return done ? 0 : 1;
+	MAL_DBG2("%d: poll() %d <- %d" NL, mal->def->index, budget, received);
+	return received;
 }
 
 static void mal_reset(struct ibm_ocp_mal *mal)

@@ -425,11 +417,8 @@ static int __init mal_probe(struct ocp_device *ocpdev)
 	mal->def = ocpdev->def;
 
 	INIT_LIST_HEAD(&mal->poll_list);
-	set_bit(__LINK_STATE_START, &mal->poll_dev.state);
-	mal->poll_dev.weight = CONFIG_IBM_EMAC_POLL_WEIGHT;
-	mal->poll_dev.poll = mal_poll;
-	mal->poll_dev.priv = mal;
-	atomic_set(&mal->poll_dev.refcnt, 1);
+	mal->napi.weight = CONFIG_IBM_EMAC_POLL_WEIGHT;
+	mal->napi.poll = mal_poll;
 
 	INIT_LIST_HEAD(&mal->list);

@@ -520,11 +509,8 @@ static void __exit mal_remove(struct ocp_device *ocpdev)
 
 	MAL_DBG("%d: remove" NL, mal->def->index);
 
-	/* Syncronize with scheduled polling, 
-	   stolen from net/core/dev.c:dev_close() 
-	 */
-	clear_bit(__LINK_STATE_START, &mal->poll_dev.state);
-	netif_poll_disable(&mal->poll_dev);
+	/* Synchronize with scheduled polling */
+	napi_disable(&mal->napi);
 
 	if (!list_empty(&mal->list)) {
 		/* This is *very* bad */

drivers/net/ibm_emac/ibm_emac_mal.h +1 -1

index 64bc338..8f54d62 100644
@@ -195,7 +195,7 @@ struct ibm_ocp_mal {
 	dcr_host_t		dcrhost;
 
 	struct list_head	poll_list;
-	struct net_device	poll_dev;
+	struct napi_struct	napi;
 
 	struct list_head	list;
 	u32			tx_chan_mask;

include/linux/netdevice.h +10 -0

index 91cd3f3..4848c7a 100644
@@ -349,6 +349,16 @@ static inline void napi_schedule(struct napi_struct *n)
 		__napi_schedule(n);
 }
 
+/* Try to reschedule poll. Called by dev->poll() after napi_complete().  */
+static inline int napi_reschedule(struct napi_struct *napi)
+{
+	if (napi_schedule_prep(napi)) {
+		__napi_schedule(napi);
+		return 1;
+	}
+	return 0;
+}
+
 /**
  *	napi_complete - NAPI processing complete
  *	@n: napi context

Roland Dreier

2007-10-09 22:48 UTC #73 lore raw

PATCH 3/4 ibm_new_emac: Nuke SET_MODULE_OWNER() use

---
 drivers/net/ibm_newemac/core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/net/ibm_newemac/core.c +0 -1

index ce127b9..8ea5009 100644
@@ -2549,7 +2549,6 @@ static int __devinit emac_probe(struct of_device *ofdev,
 	dev->ndev = ndev;
 	dev->ofdev = ofdev;
 	dev->blist = blist;
-	SET_MODULE_OWNER(ndev);
 	SET_NETDEV_DEV(ndev, &ofdev->dev);
 
 	/* Initialize some embedded data structures */

Roland Dreier

2007-10-09 22:48 UTC #74 lore raw

PATCH 4/4 ibm_emac: Convert to use napi_struct independent of struct net_device

Commit da3dedd9 ("[NET]: Make NAPI polling independent of struct net_device objects.") changed the interface to NAPI polling. Fix up the ibm_newemac driver so that it works with this new interface. This is actually a nice cleanup because ibm_newemac is one of the drivers that wants to have multiple NAPI structures for a single net_device.

Compile-tested only as I don't have a system that uses the ibm_newemac driver. This conversion the conversion for the ibm_emac driver that was tested on real PowerPC 440SPe hardware.

---
 drivers/net/ibm_newemac/mal.c |   55 ++++++++++++++--------------------------
 drivers/net/ibm_newemac/mal.h |    2 +-
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/net/ibm_newemac/mal.c +19 ~~-36~~

index c4335b7..5885411 100644
@@ -235,10 +235,10 @@ static irqreturn_t mal_serr(int irq, void *dev_instance)
 
 static inline void mal_schedule_poll(struct mal_instance *mal)
 {
-	if (likely(netif_rx_schedule_prep(&mal->poll_dev))) {
+	if (likely(napi_schedule_prep(&mal->napi))) {
 		MAL_DBG2(mal, "schedule_poll" NL);
 		mal_disable_eob_irq(mal);
-		__netif_rx_schedule(&mal->poll_dev);
+		__napi_schedule(&mal->napi);
 	} else
 		MAL_DBG2(mal, "already in poll" NL);
 }

@@ -318,8 +318,7 @@ void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
 		msleep(1);
 
 	/* Synchronize with the MAL NAPI poller. */
-	while (test_bit(__LINK_STATE_RX_SCHED, &mal->poll_dev.state))
-		msleep(1);
+	napi_disable(&mal->napi);
 }
 
 void mal_poll_enable(struct mal_instance *mal, struct mal_commac *commac)

@@ -330,11 +329,11 @@ void mal_poll_enable(struct mal_instance *mal, struct mal_commac *commac)
 	// XXX might want to kick a poll now...
 }
 
-static int mal_poll(struct net_device *ndev, int *budget)
+static int mal_poll(struct napi_struct *napi, int budget)
 {
-	struct mal_instance *mal = netdev_priv(ndev);
+	struct mal_instance *mal = container_of(napi, struct mal_instance, napi);
 	struct list_head *l;
-	int rx_work_limit = min(ndev->quota, *budget), received = 0, done;
+	int received = 0;
 	unsigned long flags;
 
 	MAL_DBG2(mal, "poll(%d) %d ->" NL, *budget,

@@ -358,26 +357,21 @@ static int mal_poll(struct net_device *ndev, int *budget)
 		int n;
 		if (unlikely(test_bit(MAL_COMMAC_POLL_DISABLED, &mc->flags)))
 			continue;
-		n = mc->ops->poll_rx(mc->dev, rx_work_limit);
+		n = mc->ops->poll_rx(mc->dev, budget);
 		if (n) {
 			received += n;
-			rx_work_limit -= n;
-			if (rx_work_limit <= 0) {
-				done = 0;
-				// XXX What if this is the last one ?
-				goto more_work;
-			}
+			budget -= n;
+			if (budget <= 0)
+				goto more_work; // XXX What if this is the last one ?
 		}
 	}
 
 	/* We need to disable IRQs to protect from RXDE IRQ here */
 	spin_lock_irqsave(&mal->lock, flags);
-	__netif_rx_complete(ndev);
+	__napi_complete(napi);
 	mal_enable_eob_irq(mal);
 	spin_unlock_irqrestore(&mal->lock, flags);
 
-	done = 1;
-
 	/* Check for "rotting" packet(s) */
 	list_for_each(l, &mal->poll_list) {
 		struct mal_commac *mc =

@@ -387,12 +381,12 @@ static int mal_poll(struct net_device *ndev, int *budget)
 		if (unlikely(mc->ops->peek_rx(mc->dev) ||
 			     test_bit(MAL_COMMAC_RX_STOPPED, &mc->flags))) {
 			MAL_DBG2(mal, "rotting packet" NL);
-			if (netif_rx_reschedule(ndev, received))
+			if (napi_reschedule(napi))
 				mal_disable_eob_irq(mal);
 			else
 				MAL_DBG2(mal, "already in poll list" NL);
 
-			if (rx_work_limit > 0)
+			if (budget > 0)
 				goto again;
 			else
 				goto more_work;

@@ -401,13 +395,8 @@ static int mal_poll(struct net_device *ndev, int *budget)
 	}
 
  more_work:
-	ndev->quota -= received;
-	*budget -= received;
-
-	MAL_DBG2(mal, "poll() %d <- %d" NL, *budget,
-		 done ? 0 : 1);
-
-	return done ? 0 : 1;
+	MAL_DBG2(mal, "poll() %d <- %d" NL, budget, received);
+	return received;
 }
 
 static void mal_reset(struct mal_instance *mal)

@@ -538,11 +527,8 @@ static int __devinit mal_probe(struct of_device *ofdev,
 	}
 
 	INIT_LIST_HEAD(&mal->poll_list);
-	set_bit(__LINK_STATE_START, &mal->poll_dev.state);
-	mal->poll_dev.weight = CONFIG_IBM_NEW_EMAC_POLL_WEIGHT;
-	mal->poll_dev.poll = mal_poll;
-	mal->poll_dev.priv = mal;
-	atomic_set(&mal->poll_dev.refcnt, 1);
+	mal->napi.weight = CONFIG_IBM_NEW_EMAC_POLL_WEIGHT;
+	mal->napi.poll = mal_poll;
 	INIT_LIST_HEAD(&mal->list);
 	spin_lock_init(&mal->lock);

@@ -653,11 +639,8 @@ static int __devexit mal_remove(struct of_device *ofdev)
 
 	MAL_DBG(mal, "remove" NL);
 
-	/* Syncronize with scheduled polling,
-	   stolen from net/core/dev.c:dev_close()
-	 */
-	clear_bit(__LINK_STATE_START, &mal->poll_dev.state);
-	netif_poll_disable(&mal->poll_dev);
+	/* Synchronize with scheduled polling */
+	napi_disable(&mal->napi);
 
 	if (!list_empty(&mal->list)) {
 		/* This is *very* bad */

drivers/net/ibm_newemac/mal.h +1 -1

index 57b69dc..cb1a16d 100644
@@ -197,7 +197,7 @@ struct mal_instance {
 	int			serr_irq;	/* MAL System Error IRQ    */
 
 	struct list_head	poll_list;
-	struct net_device	poll_dev;
+	struct napi_struct	napi;
 
 	struct list_head	list;
 	u32			tx_chan_mask;

Roland Dreier

2007-10-09 22:51 UTC #75 lore raw

Re: [ofa-general] [PATCH 4/4] ibm_emac: Convert to use napi_struct independent of struct net_device

Sorry... wrong subject here; it should have been "ibm_newemac: ..."

- R.

David Miller

maintainer

2007-10-09 23:17 UTC #76 lore raw

Re: [PATCH 1/4] IPoIB: Fix unused variable warning

From: Roland Dreier <[email protected]>
Date: Tue, 09 Oct 2007 15:46:13 -0700

Quoted 6 lines

The conversion to use netdevice internal stats left an unused variable in ipoib_neigh_free(), since there's no longer any reason to get netdev_priv() in order to increment dropped packets. Delete the unused priv variable.
Signed-off-by Roland Dreier <[email protected]>

Jeff, do you want to merge in Roland's 4 patches to your tree then do a sync with me so I can pull it all in from you?

Alternative I can merge in Roland's work directly if that's easier for you.

Just let me know.

Jeff Garzik

2007-10-10 00:32 UTC #77 lore raw

Re: [PATCH 1/4] IPoIB: Fix unused variable warning

Quoted 8 lines · David Miller

From: Roland Dreier <[email protected]>
Date: Tue, 09 Oct 2007 15:46:13 -0700
Quoted 5 lines
The conversion to use netdevice internal stats left an unused variable in ipoib_neigh_free(), since there's no longer any reason to get netdev_priv() in order to increment dropped packets. Delete the unused priv variable.

Quoted 4 lines

Quoted 1 line
Signed-off-by Roland Dreier <[email protected]>
Jeff, do you want to merge in Roland's 4 patches to your tree then do a sync with me so I can pull it all in from you?

Grabbing them now...

Jeff Garzik

2007-10-10 00:47 UTC #78 lore raw

[ofa-general] Re: [PATCH 1/4] IPoIB: Fix unused variable warning