login
Search
This patch adds the usage of batching within the core. cheers, jamal
Hey Jamal,
I still have concerns how this will work with Tx multiqueue.
The way the batching code looks right now, you will probably send a
batch of skb's from multiple bands from PRIO or RR to the driver. For
non-Tx multiqueue drivers, this is fine. For Tx multiqueue drivers,
this isn't fine, since the Tx ring is selected by the value of
skb->queue_mapping (set by the qdisc on {prio|rr}_classify()). If the
whole batch comes in with different queue_mappings, this could prove to
be an interesting issue.
Now I see in the driver HOWTO you recently sent that the driver
will be expected to loop over the list and call it's ->hard_start_xmit()
for each skb. I think that should be fine for multiqueue, I just wanted
to see if you had any thoughts on how it should work, any performance
issues you can see (I can't think of any). Since the batching feature
and Tx multiqueue are very new features, I'd like to make sure we can
think of any possible issues with them coexisting before they are both
mainline.
Looking ahead for multiqueue, I'm still working on the per-queue
lock implementation for multiqueue, which I know will not work with
batching as it's designed today. I'm still not sure how to handle this,
because it really would require the batch you send to have the same
queue_mapping in each skb, so you're grabbing the correct queue_lock.
Or, we could have the core grab all the queue locks for each
skb->queue_mapping represented in the batch. That would block another
batch though if it had any of those queues in it's next batch before the
first one completed. Thoughts?
Thanks Jamal,
-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>
-
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. Note: The other issue that needs resolving that i raised earlier was in regards to multiqueue running on multiple cpus servicing different rings It's the core that does that, not the driver; the driver continues to use ->hard_start_xmit() (albeit modified one). The idea is not to have The point behind batching is to reduce the cost of the locks by amortizing across the locks. Even better if one can, they should get rid of locks. Remind me, why do you need the per-queuemap lock? And is it needed from the enqueuing side too? Maybe lets start there to help me Sure, that is doable if the driver can set a per queue_mapping xmit_win I am not understanding the desire to have locks on a per-queuemap. I think the single queuelock we have today should suffice. If the intent is to have concurent cpus running to each hardware ring, then this is what i questioned earlier whether it was the right thing to do(very top of email where i mention it as "other issue"). cheers, jamal -
From: jamal <hadi@cyberus.ca> Add xmit_win to struct net_device_subqueue, problem solved. -
If net_device_subqueue is visible from both driver and core scheduler area (couldnt tell from looking at whats in there already), then that'll do it. cheers, jamal -
Yes, I use the net_device_subqueue structs (the state variable in there) in the prio and rr qdiscs right now. It's an indexed list at the very end of struct netdevice. -PJ Waskiewicz -
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). Addressing your note/issue with different rings being services concurrently: I'd like to remove the QDISC_RUNNING bit from the global device; with Tx multiqueue, this bit should be set on each queue (if at all), allowing multiple Tx rings to be loaded simultaneously. The biggest issue today with the multiqueue implementation is the global queue_lock. I see it being a hot source of contention in my testing; my setup is a 8-core machine (dual quad-core procs) with a 10GbE NIC, using 8 Tx and 8 Rx queues. On transmit, when loading all 8 queues, the enqueue/dequeue are hitting that lock quite a bit for the whole device. I really think that the queue_lock should join the queue_state, so the device no longer manages the top-level state (since we're operating Well, it's in 2.6.23-rc*. I imagine it won't see much action though until 2.6.24, since people will be porting drivers during that time. Plus having the native Rx multiqueue w/NAPI code in 2.6.24 makes sense The multiqueue implementation today enforces the number of qdisc bands (RR or PRIO) to be equal to the number of Tx rings your hardware/driver is supporting. Therefore, the queue_lock and queue_state in the kernel directly relate to the qdisc band management. If the queue stops from the driver, then the qdisc won't try to dequeue from the band. What I'm working on is to move the lock there too, so I can lock the queue when I enqueue (protect the band from multiple sources modifying the skb chain), and lock it when I dequeue. This is purely for concurrency of adding/popping skb's from the qdisc queues. Right now, we take the whole global lock to add and remove skb's. This is the next logical step for separating the queue dependancy on each other. Please let me know if this doesn't make sense, or if you have any questions at ...
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 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 This is the issue i raised - refer to Dave's wording of it. If you run access to the rings simultenously you may not be able to guarantee any ordering or proper qos in contention for wire-resources (think strict prio in hardware) - as long as you have the qdisc area. You may actually get away with it with something like DRR. You could totaly bypass the qdisc region and go to the driver directly and let it worry about the scheduling but youd have to make the qdisc area a "passthrough" while providing the illusion to user space that all Yes, the queuelock is expensive; in your case if all 8 hardware threads are contending for that one device, you will suffer. The txlock on the other hand is not that expensive since the contention is for a max of 2 cpus (tx and rx softirq). I tried to use that fact in the batching to move things that i processed under queue lock into the area for txlock. I'd be very interested in some results on such a piece of hardware with the 10G nic to see if Ok, so the "concurency" aspect is what worries me. What i am saying is that sooner or later you have to serialize (which is anti-concurency) For example, consider CPU0 running a high prio queue and CPU1 running the low prio queue of the same netdevice. Assume CPU0 is getting a lot of interupts or other work while ...
You made sense. I think it is important to note simply that the packet scheduling algorithm itself will dictate the level of concurrency you can achieve. Strict prio is fundamentally an interface to a big imaginary queue, with multiple packet insertion points (the individual bands/rings for each prio band). If you assume a scheduler implementation where each prio band is mapped to a separate CPU, you can certainly see where some CPUs could be substantially idle while others are overloaded, largely depending on the data workload (and priority contained within). Moreover, you increase L1/L2 cache traffic, not just because of locks, but because of data dependencies: user prio packet NIC TX ring process band scheduler cpu7 1 cpu1 1 cpu5 1 cpu1 1 cpu2 0 cpu0 0 At that point, it is probably more cache- and lock-friendly to keep the current TX softirq scheme. In contrast, a pure round-robin approach is more friendly to concurrency. Jeff -
From: Jeff Garzik <jeff@garzik.org> Right, which is why Peter added the prio DRR scheduler stuff for TX multiqueue (see net/sched/sch_prio.c:rr_qdisc_ops) because this is what the chips do. But this doesn't get us to where we want to be as Peter has been explaining a bit these past few days. Ok, we're talking a lot but not pouring much concrete, let's start doing that. I propose: 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. 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. 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. 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 ...
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. 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 -
Argh! Someone's just broken this. I think we should restore the original behaviour. Thanks, -- 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 -
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.
Thanks,
--
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
-
How does it introduce reordering? cheers, jamal -
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 -
You should not better, you wrote it and ive been going insane trying to break for at least a year now ;-> cheers, jamal -
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 -
From: Herbert Xu <herbert@gondor.apana.org.au> Agreed, and done. -
From: Herbert Xu <herbert@gondor.apana.org.au>
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 ...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 -
Groovy. I'm interested in working on a load balancer function that approximates skb->queue_mapping = smp_processor_id() 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 Definitely. Jeff -
From: Jeff Garzik <jeff@garzik.org> It's the second algorithm that will be available :-) Just add Ok. -
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 peter.p.waskiewicz.jr@intel.com -
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 -
Ah hah. Yes, a misunderstanding on my part. Thanks for the clarification. Methinks more caffeine is required for today... -PJ -
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 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 -
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 -
From: Herbert Xu <herbert@gondor.apana.org.au> Ok, then, I'll un-revert. :-) -
Hopefully that new qdisc will just use the TX rings of the hardware directly. They are typically large enough these days. That might avoid 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 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 -
On 09 Oct 2007 18:51:51 +0200 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? -- Stephen Hemminger <shemminger@linux-foundation.org> -
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 -
From: Andi Kleen <andi@firstfloor.org> 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 Absolutely. Our packet scheduler subsystem is great, but by default it should just get out of the way. -
On Tue, 09 Oct 2007 13:43:31 -0700 (PDT) I was thinking why not have a default transmit queue len of 0 like the virtual devices. -- Stephen Hemminger <shemminger@linux-foundation.org> -
From: Stephen Hemminger <shemminger@linux-foundation.org> 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. -
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 -
From: jamal <hadi@cyberus.ca> 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. -
Why would you discourage them? If 1000 is ok for a software queue why would it not be ok 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 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 -
From: Andi Kleen <andi@firstfloor.org> 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, I'll try to see how backlogged my 10Gb tests get when a strong 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. -
> A 256 entry TX hw queue fills up trivially on 1GB and 10GB, but if you Another possibility would be to consider using cache avoidance instructions while updating the TX ring (e.g. write combining on x86) -Andi -
From: Andi Kleen <andi@firstfloor.org> 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... :-) -
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 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 -
From: Andi Kleen <andi@firstfloor.org> 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. -
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 -
From: jamal <hadi@cyberus.ca> No, it was not interrupt per-packet, I was telling the chip to interrupt me every 1/4 of the ring. -
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 -
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 -
Hardware queues are generally per-page rather than per-skb so it'd fill up quicker than a software queue even with TSO. 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 -
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. -
On Fri, 12 Oct 2007 09:08:58 -0700 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. -- Stephen Hemminger <shemminger@linux-foundation.org> -
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 -
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 -
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 -
From: Bill Fink <billfink@mindspring.com> For some reason this does not surprise me. We bumped the ethernet default up to 1000 for gigabit. -
Hi Dave, 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 -
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 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 -
Hi Peter, "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com> wrote on 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 -
From: Krishna Kumar2 <krkumar2@in.ibm.com> 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. -
Hi Dave,
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
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
-
Great, this will make life easy. Any idea how long that would take? It seems simple enough to do. thanks, - KK -
From: Krishna Kumar2 <krkumar2@in.ibm.com> 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 :-) -
Awsome :) -- 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 -
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 -
From: Jeff Garzik <jeff@garzik.org> 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. -
And I most certainly checked the relevant transmit paths and other locking to make sure lock ordering was correct. Jeff -
From: Jeff Garzik <jeff@garzik.org> Awesome. -
> 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.... -
From: Roland Dreier <rdreier@cisco.com> 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. -
> 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. -
> 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. -
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 <rolandd@cisco.com>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 6b1b4b2..855c9de 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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))) {
-
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.
Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
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(-)
diff --git a/drivers/net/ibm_emac/ibm_emac_mal.c b/drivers/net/ibm_emac/ibm_emac_mal.c
index cabd984..cc3ddc9 100644
--- a/drivers/net/ibm_emac/ibm_emac_mal.c
+++ b/drivers/net/ibm_emac/ibm_emac_mal.c
@@ -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, ...Signed-off-by: Roland Dreier <rolandd@cisco.com> --- drivers/net/ibm_newemac/core.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c index ce127b9..8ea5009 100644 --- a/drivers/net/ibm_newemac/core.c +++ b/drivers/net/ibm_newemac/core.c @@ -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 */ -
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.
Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
drivers/net/ibm_newemac/mal.c | 55 ++++++++++++++--------------------------
drivers/net/ibm_newemac/mal.h | 2 +-
2 files changed, 20 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ibm_newemac/mal.c b/drivers/net/ibm_newemac/mal.c
index c4335b7..5885411 100644
--- a/drivers/net/ibm_newemac/mal.c
+++ b/drivers/net/ibm_newemac/mal.c
@@ -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 ...Sorry... wrong subject here; it should have been "ibm_newemac: ..." - R. -
From: Roland Dreier <rdreier@cisco.com> 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. -