Re: Fix up riscom8 driver to use work queues instead of task queueing.

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Jeff Garzik]

On Mon, Aug 18, 2003 at 04:59:53PM +0000, Linux Kernel Mailing List wrote:
> @@ -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)
> @@ -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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Linus Torvalds]

On Mon, 18 Aug 2003, Jeff Garzik wrote:
> 
> Should we just make schedule_delayed_work(foo, 1) the default for a 
> schedule_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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Russell King]

On Mon, Aug 18, 2003 at 11:21:07AM -0700, Linus Torvalds wrote:
> 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*)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Jeff Garzik]

On Mon, Aug 18, 2003 at 11:21:07AM -0700, Linus Torvalds wrote:
> 
> On Mon, 18 Aug 2003, Jeff Garzik wrote:
> > 
> > Should we just make schedule_delayed_work(foo, 1) the default for a 
> > schedule_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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Linus Torvalds]

On Mon, 18 Aug 2003, Russell King wrote:
> 
> 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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Russell King]

On Mon, Aug 18, 2003 at 11:47:14AM -0700, Linus Torvalds wrote:
> 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)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Linus Torvalds]

On Mon, 18 Aug 2003, Russell King wrote:
> 
> 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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Linus Torvalds]

On Mon, 18 Aug 2003, Jeff Garzik wrote:
> 
> 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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Jeff Garzik]

On Mon, Aug 18, 2003 at 12:12:19PM -0700, Linus Torvalds wrote:
> 
> On Mon, 18 Aug 2003, Jeff Garzik wrote:
> > 
> > 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".

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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Linus Torvalds]

On Mon, 18 Aug 2003, Jeff Garzik wrote:
> 
> 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

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> On Mon, 18 Aug 2003, Jeff Garzik wrote:
> > 
> > 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.

Re: Fix up riscom8 driver to use work queues instead of task queueing.

[Jeff Garzik]

Andrew Morton wrote:
> 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