libata: use single threaded work queue

Hi,

On boxes with lots of CPUs, we have so many kernel threads it's not funny. The basic problem is that create_workqueue() creates a per-cpu thread, where we could easily get by with a single thread for a lot of cases.

One such case appears to be ata_wq. You want at most one per pio drive, not one per CPU. I'd suggest just dropping it to a single threaded wq.

Signed-off-by Jens Axboe<[email protected]>

drivers/ata/libata-core.c +0 -0

index 072ba5e..0d78628 100644

Quoted 17 lines · Jens Axboe

Hi,

On boxes with lots of CPUs, we have so many kernel threads it's not funny. The basic problem is that create_workqueue() creates a per-cpu thread, where we could easily get by with a single thread for a lot of cases.

One such case appears to be ata_wq. You want at most one per pio drive, not one per CPU. I'd suggest just dropping it to a single threaded wq.

Signed-off-by Jens Axboe<[email protected]>

drivers/ata/libata-core.c +0 -0

index 072ba5e..0d78628 100644

drivers/ata/libata-core.c static int __init ata_init(void) · line 6585

Quoted 7 lines

drivers/ata/libata-core.c +1 -1

@@ -6580,7 +6580,7 @@ static int __init ata_init(void)
  {
  	ata_parse_force_param();
 
-	ata_wq = create_workqueue("ata");
+	ata_wq = create_singlethread_workqueue("ata");
  	if (!ata_wq)
  		goto free_force_tbl;

I agree with one-thread-per-cpu is too much, in these modern multi-core times, but one thread is too little. You have essentially re-created simplex DMA -- blocking and waiting such that one drive out of ~4 can be used at any one time.

ata_pio_task() is in a workqueue so that it can sleep and/or spend a long time polling ATA registers. That means an active task can definitely starve all other tasks in the workqueue, if only one thread is available. If starvation occurs, it will potentially pause the unrelated task for several seconds.

The proposed patch actually expands an existing problem -- uniprocessor case, where there is only one workqueue thread. For the reasons outlined above, we actually want multiple threads even in the UP case. If you have more than one PIO device, latency is bloody awful, with occasional multi-second "hiccups" as one PIO devices waits for another. It's an ugly wart that users DO occasionally complain about; luckily most users have at most one PIO polled device.

It would be nice if we could replace this workqueue with a thread pool, where thread count inside the pool ranges from zero to $N depending on level of thread pool activity. Our common case in libata would be _zero_ threads, if so...

Quoted 30 lines · Jeff Garzik

Quoted 17 lines · Jens Axboe

Hi,

On boxes with lots of CPUs, we have so many kernel threads it's not funny. The basic problem is that create_workqueue() creates a per-cpu thread, where we could easily get by with a single thread for a lot of cases.

One such case appears to be ata_wq. You want at most one per pio drive, not one per CPU. I'd suggest just dropping it to a single threaded wq.

Signed-off-by Jens Axboe<[email protected]>

drivers/ata/libata-core.c +0 -0

index 072ba5e..0d78628 100644

drivers/ata/libata-core.c static int __init ata_init(void) · line 6585

Quoted 7 lines

drivers/ata/libata-core.c +1 -1

@@ -6580,7 +6580,7 @@ static int __init ata_init(void)
  {
  	ata_parse_force_param();
 
-	ata_wq = create_workqueue("ata");
+	ata_wq = create_singlethread_workqueue("ata");
  	if (!ata_wq)
  		goto free_force_tbl;

I agree with one-thread-per-cpu is too much, in these modern multi-core times, but one thread is too little. You have essentially re-created simplex DMA -- blocking and waiting such that one drive out of ~4 can be used at any one time.

ata_pio_task() is in a workqueue so that it can sleep and/or spend a long time polling ATA registers. That means an active task can definitely starve all other tasks in the workqueue, if only one thread is available. If starvation occurs, it will potentially pause the unrelated task for several seconds.

Quoted 3 lines

The proposed patch actually expands an existing problem -- uniprocessor case, where there is only one workqueue thread. For the reasons outlined above, we actually want multiple threads even in the UP case. If you have more than one PIO device, latency is bloody awful, with occasional multi-second "hiccups" as one PIO devices waits for another. It's an ugly wart that users DO occasionally complain about; luckily most users have at most one PIO polled device.

It would be nice if we could replace this workqueue with a thread pool, where thread count inside the pool ranges from zero to $N depending on level of thread pool activity. Our common case in libata would be _zero_ threads, if so...

That would be ideal, N essentially be:

        N = min(nr_cpus, nr_drives_that_need_pio);

Quoted 52 lines · Jens Axboe

Quoted 50 lines · Jeff Garzik

Quoted 17 lines · Jens Axboe

Hi,

On boxes with lots of CPUs, we have so many kernel threads it's not funny. The basic problem is that create_workqueue() creates a per-cpu thread, where we could easily get by with a single thread for a lot of cases.

One such case appears to be ata_wq. You want at most one per pio drive, not one per CPU. I'd suggest just dropping it to a single threaded wq.

Signed-off-by Jens Axboe<[email protected]>

drivers/ata/libata-core.c +0 -0

index 072ba5e..0d78628 100644

drivers/ata/libata-core.c static int __init ata_init(void) · line 6585

Quoted 7 lines

drivers/ata/libata-core.c +1 -1

@@ -6580,7 +6580,7 @@ static int __init ata_init(void)
  {
  	ata_parse_force_param();
 
-	ata_wq = create_workqueue("ata");
+	ata_wq = create_singlethread_workqueue("ata");
  	if (!ata_wq)
  		goto free_force_tbl;

I agree with one-thread-per-cpu is too much, in these modern multi-core times, but one thread is too little. You have essentially re-created simplex DMA -- blocking and waiting such that one drive out of ~4 can be used at any one time.

ata_pio_task() is in a workqueue so that it can sleep and/or spend a long time polling ATA registers. That means an active task can definitely starve all other tasks in the workqueue, if only one thread is available. If starvation occurs, it will potentially pause the unrelated task for several seconds.

The proposed patch actually expands an existing problem -- uniprocessor case, where there is only one workqueue thread. For the reasons outlined above, we actually want multiple threads even in the UP case. If you have more than one PIO device, latency is bloody awful, with occasional multi-second "hiccups" as one PIO devices waits for another. It's an ugly wart that users DO occasionally complain about; luckily most users have at most one PIO polled device.

It would be nice if we could replace this workqueue with a thread pool, where thread count inside the pool ranges from zero to $N depending on level of thread pool activity. Our common case in libata would be _zero_ threads, if so...

Quoted 3 lines

That would be ideal, N essentially be:

        N = min(nr_cpus, nr_drives_that_need_pio);

..

No, that would leave just a single thread again for UP.

It would be nice to just create these threads on-demand, and destroy them again after periods of dis-use. Kind of like how Apache does worker threads.

Quoted 53 lines · Mark Lord

Quoted 52 lines · Jens Axboe

Quoted 50 lines · Jeff Garzik

Quoted 17 lines · Jens Axboe

Hi,

On boxes with lots of CPUs, we have so many kernel threads it's not funny. The basic problem is that create_workqueue() creates a per-cpu thread, where we could easily get by with a single thread for a lot of cases.

One such case appears to be ata_wq. You want at most one per pio drive, not one per CPU. I'd suggest just dropping it to a single threaded wq.

Signed-off-by Jens Axboe<[email protected]>

drivers/ata/libata-core.c +0 -0

index 072ba5e..0d78628 100644

drivers/ata/libata-core.c static int __init ata_init(void) · line 6585

Quoted 7 lines

drivers/ata/libata-core.c +1 -1

@@ -6580,7 +6580,7 @@ static int __init ata_init(void)
   {
   	ata_parse_force_param();
 
-	ata_wq = create_workqueue("ata");
+	ata_wq = create_singlethread_workqueue("ata");
   	if (!ata_wq)
   		goto free_force_tbl;

I agree with one-thread-per-cpu is too much, in these modern multi-core times, but one thread is too little. You have essentially re-created simplex DMA -- blocking and waiting such that one drive out of ~4 can be used at any one time.

ata_pio_task() is in a workqueue so that it can sleep and/or spend a long time polling ATA registers. That means an active task can definitely starve all other tasks in the workqueue, if only one thread is available. If starvation occurs, it will potentially pause the unrelated task for several seconds.

The proposed patch actually expands an existing problem -- uniprocessor case, where there is only one workqueue thread. For the reasons outlined above, we actually want multiple threads even in the UP case. If you have more than one PIO device, latency is bloody awful, with occasional multi-second "hiccups" as one PIO devices waits for another. It's an ugly wart that users DO occasionally complain about; luckily most users have at most one PIO polled device.

It would be nice if we could replace this workqueue with a thread pool, where thread count inside the pool ranges from zero to $N depending on level of thread pool activity. Our common case in libata would be _zero_ threads, if so...

Quoted 6 lines

Quoted 3 lines

That would be ideal, N essentially be:

         N = min(nr_cpus, nr_drives_that_need_pio);

..

No, that would leave just a single thread again for UP.

So one thread per ap would be better, then.

Quoted 3 lines

It would be nice to just create these threads on-demand, and destroy them again after periods of dis-use. Kind of like how Apache does worker threads.

Well, that's the same thread pool suggestion that Jeff came up with. And I agree, that's a nicer long term solution (it's also how the per-bdi flushing replacement works). The problem with that appears to be that any suggested patchset for thread pools spiral into certain "but what color should it be?!" death.

I'll try and work up a simple create_threadpool() implementation, we can literally cut away hundreds of threads with that.

Quoted 9 lines

Quoted 3 lines

It would be nice to just create these threads on-demand, and destroy them again after periods of dis-use. Kind of like how Apache does worker threads.

Well, that's the same thread pool suggestion that Jeff came up with. And I agree, that's a nicer long term solution (it's also how the per-bdi flushing replacement works). The problem with that appears to be that any suggested patchset for thread pools spiral into certain "but what color should it be?!" death.

Let people complain with code :) libata has two basic needs in this area:
(1) specifying a thread count other than "1" or "nr-cpus"
(2) don't start unneeded threads / idle out unused threads

Quoted 3 lines

Let people complain with code :) libata has two basic needs in this area:
(1) specifying a thread count other than "1" or "nr-cpus"
(2) don't start unneeded threads / idle out unused threads

To be even more general,

libata needs a workqueue or thread pool that can

(a) scale up to nr-drives-that-use-pio threads, on demand
(b) scale down to zero threads, with lack of demand

That handles the worst case of each PIO-polling drive needing to sleep (thus massively impacting latency, if any other PIO-polling drive must wait for a free thread).

That also handles the best case of not needing any threads at all.

Somewhat simpler for the general case would be to implement

create_workqueue(min_threads,max_threads, max_thread_idle_time); queue_work_within();

at which point you can queue work to the workqueue but with a per workqueue timing running so you know when you might need to create a new thread if the current work hasn't finished. Idle threads would then sleep and either expire or pick up new work - so that under load we don't keep destructing threads.

That might need a single thread (for the system) that does nothing but create workqueue threads to order. It could manage the "next workqueue deadline" timer and thread creation.

The threads would then pick up their work queue work. There is an intrinsic race where if we are just on the limit we might create a thread just as there is no work to be done - but it would be rare and would just then go away.

I see no point tring to fix ata when applying sanity to the workqueue logic would sort a lot of other cases out nicely.

Are work threads per workqueue? Combined with per-cpu binding, dynamic thread pool per workqueue can get quite messy. All three factors end up getting multiplied - ie. #cpus * pool_size which can be enlarged by the same work hopping around * #workqueues.

Another problem is that if we apply this to the existing default workqueue which is used by many different supposed-to-be-short works in essentially batch mode, we might end up enlarging cache footprint by scheduling unnecessarily many threads, which, in tight situations, might show up as small but noticeable performance regression.

Quoted 4 lines

Are work threads per workqueue? Combined with per-cpu binding, dynamic thread pool per workqueue can get quite messy. All three factors end up getting multiplied - ie. #cpus * pool_size which can be enlarged by the same work hopping around * #workqueues.

Stop a moment. Exactly how many work queue users need per cpu binding wizardry ?

Quoted 5 lines

Another problem is that if we apply this to the existing default workqueue which is used by many different supposed-to-be-short works in essentially batch mode, we might end up enlarging cache footprint by scheduling unnecessarily many threads, which, in tight situations, might show up as small but noticeable performance regression.

Only if you make the default assumed max wait time for the work too low - its a tunable behaviour in fact.

It's not about needing per-cpu binding but if works can be executed on the same cpu they were issued, it's almost always beneficial. The only reason why we have single threaded workqueue now is to limit the number of threads.

Quoted 2 lines

Only if you make the default assumed max wait time for the work too low - its a tunable behaviour in fact.

If the default workqueue is made to manage concurrency well, most works should be able to just use it, so the queue will contain both long running ones and short running ones which can disturb the current batch like processing of the default workqueue which is assumed to have only short ones.

kthreads). It would be great if a single work API is exported and concurrency is managed automatically so that no one else has to worry about concurrency but achieving that requires much more intelligence on the workqueue implementation as the basic concurrency policies which used to be imposed by those segregations need to be handled automatically. Maybe it's better trade-off to leave those segregations as-are and just add another workqueue type with dynamic thread pool.

Quoted 4 lines

It's not about needing per-cpu binding but if works can be executed on the same cpu they were issued, it's almost always beneficial. The only reason why we have single threaded workqueue now is to limit the number of threads.

That would argue very strongly for putting all the logic in one place so everything shares queues.

Quoted 7 lines

Quoted 2 lines

Only if you make the default assumed max wait time for the work too low - its a tunable behaviour in fact.

If the default workqueue is made to manage concurrency well, most works should be able to just use it, so the queue will contain both long running ones and short running ones which can disturb the current batch like processing of the default workqueue which is assumed to have only short ones.

Not sure why it matters - the short ones will instead end up being processed serially in parallel to the hog.

Quoted 8 lines

kthreads). It would be great if a single work API is exported and concurrency is managed automatically so that no one else has to worry about concurrency but achieving that requires much more intelligence on the workqueue implementation as the basic concurrency policies which used to be imposed by those segregations need to be handled automatically. Maybe it's better trade-off to leave those segregations as-are and just add another workqueue type with dynamic thread pool.

The more intelligence in the workqueue logic, the less in the drivers and the more it can be adjusted and adapt itself.

Consider things like power management which might argue for breaking the cpu affinity to avoid waking up a sleeping CPU in preference to jumping work between processors

Somewhat simpler for the general case would be to implement

create_workqueue(min_threads,max_threads, max_thread_idle_time); queue_work_within();

at which point you can queue work to the workqueue but with a per workqueue timing running so you know when you might need to create a new thread if the current work hasn't finished. Idle threads would then sleep and either expire or pick up new work

Sounds a lot like kernel/slow-work.c / include/linux/slow-work.h to me (as Ben already pointed out).

Well, that's the same thread pool suggestion that Jeff came up with. And
I agree, that's a nicer long term solution (it's also how the per-bdi flushing replacement works). The problem with that appears to be that any suggested patchset for thread pools spiral into certain "but what color should it be?!" death.

I'll try and work up a simple create_threadpool() implementation, we can
literally cut away hundreds of threads with that.

Quoted 7 lines · Jens Axboe

Well, that's the same thread pool suggestion that Jeff came up with. And
I agree, that's a nicer long term solution (it's also how the per-bdi flushing replacement works). The problem with that appears to be that any suggested patchset for thread pools spiral into certain "but what color should it be?!" death.

Quoted 3 lines

I'll try and work up a simple create_threadpool() implementation, we can
literally cut away hundreds of threads with that.

Don't we have one already ? Dave Howells slow_work or such ?

Yes, it addresses different aspect of the concurrency problem. Might be more suitable for ATA workqueues but definitely more costly to convert to. Argh...

Quoted 3 lines · Tejun Heo

Yes, it addresses different aspect of the concurrency problem. Might be more suitable for ATA workqueues but definitely more costly to convert to. Argh...

^ compared to Jens's lazy workqueue.