Index: drivers/base/firmware_class.c
===================================================================
RCS file: /home/cvs/linux-2.5/drivers/base/firmware_class.c,v
retrieving revision 1.3
diff -u -r1.3 drivers/base/firmware_class.c
--- drivers/base/firmware_class.c 4 Jul 2003 02:21:18 -0000 1.3
+++ drivers/base/firmware_class.c 26 Jul 2003 08:38:07 -0000
@@ -22,6 +22,8 @@
MODULE_LICENSE("GPL");
static int loading_timeout = 10; /* In seconds */
+static struct workqueue_struct *firmware_wq;
+
struct firmware_priv {
char fw_id[FIRMWARE_NAME_MAX];
@@ -467,7 +469,7 @@
};
INIT_WORK(&fw_work->work, request_firmware_work_func, fw_work);
- schedule_work(&fw_work->work);
+ queue_work(firmware_wq, &fw_work->work);
return 0;
}
@@ -485,12 +487,20 @@
__FUNCTION__);
class_unregister(&firmware_class);
}
+ firmware_wq = create_workqueue("firmware");
+ if (!firmware_wq) {
+ printk(KERN_ERR "%s: create_workqueue failed\n", __FUNCTION__);
+ class_remove_file(&firmware_class, &class_attr_timeout);
+ class_unregister(&firmware_class);
+ error = -EIO;
+ }
return error;
}
static void __exit
firmware_class_exit(void)
{
+ destroy_workqueue(firmware_wq);
class_remove_file(&firmware_class, &class_attr_timeout);
class_unregister(&firmware_class);
}
Manuel Estrada Sainz <[email protected]> wrote:
>
> Please apply the following patches.
Please, send just one patch per email, each one having its own changelog
info. There's just no way anyone's patch import tools can handle a single
changelog and multiple attachments. It is painful.
> - request_firmware_own-workqueue.diff:
> - In it's current form request_firmware_async() sleeps way too
> long on the system's shared workqueue, which makes it
> unresponsive until the firmware load finishes, gets canceled
> or times out.
Does this mean that we have another gaggle of kernel threads for all the
time the system is up?
It might be better to create a custom kernel thread on-demand, or kill off
the workqueue when its job has completed.
On Sun, Aug 10, 2003 at 02:29:28PM -0700, Andrew Morton wrote:
> Manuel Estrada Sainz <[email protected]> wrote:
> >
> > Please apply the following patches.
>
> Please, send just one patch per email, each one having its own changelog
> info. There's just no way anyone's patch import tools can handle a single
> changelog and multiple attachments. It is painful.
Sorry, I'll do it in a few minutes.
> > - request_firmware_own-workqueue.diff:
> > - In it's current form request_firmware_async() sleeps way too
> > long on the system's shared workqueue, which makes it
> > unresponsive until the firmware load finishes, gets canceled
> > or times out.
>
> Does this mean that we have another gaggle of kernel threads for all the
> time the system is up?
I guess so.
> It might be better to create a custom kernel thread on-demand, or kill off
> the workqueue when its job has completed.
OK, I'll wait on this one, and think about it, after all, it just fixes
annoying behavior, not real breakness.
Thanks for the quick reply
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
Andrew Morton wrote:
> Does this mean that we have another gaggle of kernel threads for all the
> time the system is up?
>
> It might be better to create a custom kernel thread on-demand, or kill off
> the workqueue when its job has completed.
Thanks for mentioning this!
There is indeed an explosion of kernel threads. I feel many of them
fall into two categories:
* needs a one-shot kernel thread, often after a timer fires or similar
interrupt-ish-context code runs (error handling threads also apply here)
* only needs per-cpu threads if IO(or net) load is significant
And personally, I don't think anyone would scream if
schedule_{task,work} just created a new thread -- if needed -- and then
did the work. That's what the driver authors _really_ want out of the
interface, IMO. They don't want to wait for another keventd task,
typically. In other words, a thread pool, that will create new threads
beyond the max pool size.
On the other side of the coin, a valid reason for creating kernel
threads is when your application is constantly doing stuff in process
context. You might as well have dedicated kernel threads for these
drivers, because you are assured you will need at least one.
So, in terms of concrete suggestions:
1) if schedule_work is called and no kevent thread is available, create
a new one
2) ponder perhaps an implementation that would use generic keventd until
a certain load is reached; then, create per-cpu kernel threads just like
private workqueue creation occurs now. i.e. switch from shared
(keventd) to private at runtime.
As a tangent, I would love a simplified interface that was used in
driver code like this:
run_in_process_context(callback_fn, callback_data)
that eliminates the tq/workqueue struct altogether. Combine this
simplified interface with suggestion #1 above, and you've got something
quite useful, and tough to screw up.
Jeff
Jeff Garzik wrote:
> So, in terms of concrete suggestions:
>
> 1) if schedule_work is called and no kevent thread is available, create
> a new one
> 2) ponder perhaps an implementation that would use generic keventd until
> a certain load is reached; then, create per-cpu kernel threads just like
> private workqueue creation occurs now. i.e. switch from shared
> (keventd) to private at runtime.
3) offer private workqueue interface like we have now -- but one thread
only, not NN threads
On Sun, Aug 10, 2003 at 07:00:29PM -0400, Jeff Garzik wrote:
> Jeff Garzik wrote:
> >So, in terms of concrete suggestions:
> >
> >1) if schedule_work is called and no kevent thread is available, create
> >a new one
> >2) ponder perhaps an implementation that would use generic keventd until
> >a certain load is reached; then, create per-cpu kernel threads just like
> >private workqueue creation occurs now. i.e. switch from shared
> >(keventd) to private at runtime.
>
>
> 3) offer private workqueue interface like we have now -- but one thread
> only, not NN threads
>
How about, launching the private workqueue threads on demand?
a) Create thread/s then the first work gets scheduled.
b) When the last pending work is done arm a timer or schedule a delayed
special work that kills the thread/s after a while of idleness.
c) When a new work gets added to an otherwise idle workqueue either:
- If we have thread/s we cancel the kill and go on.
- Else we are back at a) we create it/them.
And if instead of creating all threads at once, we create them one by
one as work comes in and also kill them one by one as they stay idle,
it could be quite smooth.
How does this sound? Am I over-designing the issue?
IMHO this would remove the threads of unused private workqueues without
affecting busy workqueues.
About using a single thread instead of NN, wouldn't that impact
workqueue users expecting speedy handling of their queues?
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Llu, 2003-08-11 at 00:00, Jeff Garzik wrote:
> > 2) ponder perhaps an implementation that would use generic keventd until
> > a certain load is reached; then, create per-cpu kernel threads just like
> > private workqueue creation occurs now. i.e. switch from shared
> > (keventd) to private at runtime.
>
>
> 3) offer private workqueue interface like we have now -- but one thread
> only, not NN threads
4) Have one permanent thread running which kicks off another thread
whenever something has been on the queue for more than 5 seconds and
reaps threads when the queue is empty for 30 ?