Fix up riscom8 driver to use work queues instead of task queueing.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 10 lines · Linux Kernel Mailing List
(quoted diff) +1
-1@@ -425,7 +414,7 @@ *tty->flip.char_buf_ptr++ = ch; tty->flip.count++; - queue_task(&tty->flip.tqueue, &tq_timer); + schedule_delayed_work(&tty->flip.work, 1); } static inline void rc_receive(struct riscom_board const * bp)
Review line 451
Quoted 8 lines
(quoted diff) +1
-1@@ -456,7 +445,7 @@ *tty->flip.flag_buf_ptr++ = 0; tty->flip.count++; } - queue_task(&tty->flip.tqueue, &tq_timer); + schedule_delayed_work(&tty->flip.work, 1); } static inline void rc_transmit(struct riscom_board const * bp)
Should we just make schedule_delayed_work(foo, 1) the default for a schedule_work() call?
I'm seeing this construct pop up more and more... and would rather have it fixed at the source, not in every driver.
Jeff
Quoted 3 lines · Jeff Garzik
Should we just make
schedule_delayed_work(foo, 1)the default for aschedule_work()call?
Why? There are cases where you may really want to get the work done asap, so the regular "schedule_work()" is the right thing. While the "delayed" thing is for stuff that explicitly doesn't want to happen immediately, because we expect to aggregate more into it.
Having done a few serial drivers (not because I want to, but because nobody else seems to be doing them), I definitely see the need for both.
For example, the hangup case wants to be done asap, not delayed.
Linus
Quoted 2 lines · Linus Torvalds
Having done a few serial drivers (not because I want to, but because nobody else seems to be doing them), I definitely see the need for both.
Of course, the more correct approach is for someone to convert them to use the new serial driver core (and fix the driver core interface to allow them to work with it.)
Unfortunately I don't have the hardware for a lot of these specialist serial cards, so it's a job I've steared clear of. I've been hoping that leaving them broken will make the ones which are used stand out, and hopefully someone with the hardware would've picked them up and done the necessary conversion.
But alas no.
(I did my best to convert dz.c without the hardware, but it seems to have been lost into the depths of the MIPS tree... *hint*)
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 4 lines · Russell King
Of course, the more correct approach is for someone to convert them to use the new serial driver core (and fix the driver core interface to allow them to work with it.)
Hey, I'm all for that for 2.7.x. In the meantime, they've been broken for a year, so let's just try to fix them up into a "limping along" state.
I don't have the hardware either, but at least now they should be testable on UP configurations (SMP is generally still broken due to the drivers expecting to be able to disable all interrups globally instead of using proper locking).
I'd be interested to hear whether the dang things work. Of course, there probably aren't that many people around with the hardware any more. I could just have added them to the BROKEN list, but since they _might_ work it seemed like a better idea to be hugely optimistic instead ;)
Linus
Quoted 4 lines · Linus Torvalds
I'd be interested to hear whether the dang things work. Of course, there probably aren't that many people around with the hardware any more. I could just have added them to the BROKEN list, but since they _might_ work it seemed like a better idea to be hugely optimistic instead ;)
True. However, there is the opposite point of view which is equally valid. There aren't many people with the hardware, and the people that there are aren't interested in development kernel series, so even if we did convert them during 2.7, we wouldn't hear about it until 2.8.
IMO its far better to do these types of conversions when users are interested in the driver (and can therefore give you bug reports) than when they are actively ignoring the development series.
Both positions are equally valid. I'm not going to argue that one is more valid than the other because I have enough on my plate for the time being. 8)
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 6 lines · Russell King
True. However, there is the opposite point of view which is equally valid. There aren't many people with the hardware, and the people that there are aren't interested in development kernel series, so even if we did convert them during 2.7, we wouldn't hear about it until 2.8.
Yes. However, what worries me more is that there are people who have the hardware, but because the driver won't even compile for them, they just go "oh, well, I'll try it again when the _real_ 2.6.0 hits the streets".
Which obviously won't work.
So I'm trying to make sure that all the broken drivers are gotten to a working state. Right now, considering how long they've been broken, that means "it must compile" so that people can test them.
The "leave it broken, so that somebody will fix it properly some day" approach is a fine one for early development series. But right now I'd prefer to see patches to make drivers compile cleanly, even if people can't test them on real hardware.
The intersection of people who have the hardware, and people who have the time/knowledge to convert a driver, may be empty. Expecially for odd hardware. So let's get those drivers compiling, even if we can't test them, so that others _can_ test them.
Linus
Quoted 8 lines · Linus Torvalds
Quoted 3 lines · Jeff Garzik
Should we just make
schedule_delayed_work(foo, 1)the default for aschedule_work()call?Why? There are cases where you may really want to get the work done asap, so the regular "
schedule_work()" is the right thing. While the "delayed"
schedule_work() is _not_ for that. As currently implemented, you have no guarantees that your schedule_work()-initiated work will even begin in this century.
Drivers are using schedule_work() to create process contexts where they can sleep, potentially for many seconds. On a single cpu system, or a loaded SMP system, schedule_work() from one of the drivers you converted could easily be delayed for 30 seconds or more. schedule_work() is not a fast path.
If work needs to happen ASAP, then you really want to replace those schedule_work() calls with schedule_tasklet() calls that do the "must be done asap" work, and then schedule_work() the stuff that requires process context... So, too, I would have thought that bottom-half completion routines mapped more directly to schedule_tasklet() anyway.
Jeff
Quoted 4 lines · Jeff Garzik
schedule_work()is _not_ for that. As currently implemented, you have no guarantees that your schedule_work()-initiated work will even begin in this century.
In theory yes. In practice no. schedule_work() tries to wake up the worker process immediately, and as such usually gets the work done asap.
But hey, if you want to improve on the drivers, please go wild. I care more about "real life working" than "theoretical but doesn't work".
Linus
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · Jeff Garzik
schedule_work()is _not_ for that. As currently implemented, you have no guarantees that your schedule_work()-initiated work will even begin in this century.
Quoted 5 lines
In theory yes. In practice no.
schedule_work()tries to wake up the worker process immediately, and as such usually gets the work done asap.But hey, if you want to improve on the drivers, please go wild. I care more about "real life working" than "theoretical but doesn't work".
hehe ;-) well,
* I've hit this situation in real life (waiting on a driver doing error handling, hogging the single shared workqueue on UP). It actually gets nasty if a bunch of drivers all throw errors at the same time... (more below)
* re "improve the drivers" -- if you mean "fixing" those hogging the shared workqueue, I think that's a workqueue API flaw. If you mean further fix up the drivers you just modified, well... I think slackness on my part will win there :)
There was talk in another thread about fixing up workqueue to create a new kernel thread, if one isn't available within five seconds. That seemed reasonable to me. Another useful addition to workqueue, for drivers, would be a one-shot thread "runme(func, func_data)" API. I think a lot of the device driver error handling is more appropriate to this one-shot API than the current workqueue API. Error handling is already a slow path, so the overhead of thread creation on each call is mitigated.
Jeff
Quoted 3 lines · Jeff Garzik
There was talk in another thread about fixing up workqueue to create a new kernel thread, if one isn't available within five seconds.
That's probably reasonable. Together with some upper limit to active threads, along with timeouting old queues when idle it would be fairly flexible. It's how basically all servers end up working, after all. It shouldn't be that hard to do.
Linus
Quoted 4 lines · Linus Torvalds
Quoted 1 line · Jeff Garzik
Quoted 8 lines
Quoted 2 lines
There was talk in another thread about fixing up workqueue to create a new kernel thread, if one isn't available within five seconds.
That's probably reasonable. Together with some upper limit to active threads, along with timeouting old queues when idle it would be fairly flexible. It's how basically all servers end up working, after all. It shouldn't be that hard to do.
Note that pdflush is already doing this. It doesn't seem to work very well though:
a) new threads seem to start up too late
b) they probably don't hang around for long enough
c) now that pdflush avoids blocking on queues it's all rather overkill anyway.
pdflush could kinda-sorta be converted to use workqueues, but it doesn't want a thread per cpu.
Quoted 2 lines · Andrew Morton
pdflush could kinda-sorta be converted to use workqueues, but it doesn't want a thread per cpu.
That was another item in the recent thread about workqueues: other kernel code is very suited to the workqueue API, but only needs one thread, not a thread per cpu. Would be nice to have a JUST_ONE_THREAD flag to pass to create_workqueue().
I bet adding such a flag would help alleviate some of the "I have 1001 kthreads on my 16-way" complaints ;-)
Jeff