diff mbox series

[v5,3/5] workqueue: Make too_many_workers() return the worker excess

Message ID 20221122192937.2386494-4-vschneid@redhat.com
State New
Headers
Series workqueue: destroy_worker() vs isolated CPUs |

Commit Message

Valentin Schneider Nov. 22, 2022, 7:29 p.m. UTC 
  Later patches will need the logic implemented within too_many_workers() to
get the amount of workers to delete. Rather than duplicate the logic,
rework too_many_workers() to return the count of workers to delete - its
return value can be used as a boolean value, so no change in behaviour
intended.

The function currently returns true when
  (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy
thus, the desired number of idle workers is expressed by
  (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO == nr_busy - 1
IOW
   nr_idle == ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO) + 2

MAX_IDLE_WORKERS_RATIO being a compile-time power of 2, we can leave that
as a division.

While at it, rename too_many_workers() to worker_cull_count().

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 kernel/workqueue.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Tejun Heo Nov. 22, 2022, 8:17 p.m. UTC | #1 
Hello,

On Tue, Nov 22, 2022 at 07:29:35PM +0000, Valentin Schneider wrote:
...
> The function currently returns true when
>   (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy
> thus, the desired number of idle workers is expressed by
>   (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO == nr_busy - 1
> IOW
>    nr_idle == ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO) + 2
> +/* How many idle workers should we get rid of, if any? */
> +static unsigned int worker_cull_count(struct worker_pool *pool)

Can we name it nr_workers_to_cull()?

>  {
>  	bool managing = pool->flags & POOL_MANAGER_ACTIVE;
>  	int nr_idle = pool->nr_idle + managing; /* manager is considered idle */
>  	int nr_busy = pool->nr_workers - nr_idle;
>  
> -	return nr_idle > 2 && (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy;
> +	lockdep_assert_held(&pool->lock);
> +
> +	/*
> +	 * We keep at least 2 spare idle workers, but overall aim to keep at
> +	 * most (1 / MAX_IDLE_WORKERS_RATIO) workers idle.
> +	 */
> +	return max(0, nr_idle - 2 - ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO));

I think we can do away with the subtraction on nr_busy. I don't think it'd
make any material difference, so maybe we can do:

        return max(0, nr_idle - 2 - nr_busy / MAX_IDLE_WORKERS_RATIO);

Thanks.
Valentin Schneider Nov. 28, 2022, 11:24 a.m. UTC | #2 
On 22/11/22 10:17, Tejun Heo wrote:
> Hello,
>
> On Tue, Nov 22, 2022 at 07:29:35PM +0000, Valentin Schneider wrote:
> ...
>> The function currently returns true when
>>   (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy
>> thus, the desired number of idle workers is expressed by
>>   (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO == nr_busy - 1
>> IOW
>>    nr_idle == ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO) + 2
>> +/* How many idle workers should we get rid of, if any? */
>> +static unsigned int worker_cull_count(struct worker_pool *pool)
>
> Can we name it nr_workers_to_cull()?
>

Ack

>>  {
>>  	bool managing = pool->flags & POOL_MANAGER_ACTIVE;
>>  	int nr_idle = pool->nr_idle + managing; /* manager is considered idle */
>>  	int nr_busy = pool->nr_workers - nr_idle;
>>  
>> -	return nr_idle > 2 && (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy;
>> +	lockdep_assert_held(&pool->lock);
>> +
>> +	/*
>> +	 * We keep at least 2 spare idle workers, but overall aim to keep at
>> +	 * most (1 / MAX_IDLE_WORKERS_RATIO) workers idle.
>> +	 */
>> +	return max(0, nr_idle - 2 - ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO));
>
> I think we can do away with the subtraction on nr_busy. I don't think it'd
> make any material difference, so maybe we can do:
>
>         return max(0, nr_idle - 2 - nr_busy / MAX_IDLE_WORKERS_RATIO);
>

I'll do that if this survives in the next revision :)

> Thanks.
>
> -- 
> tejun
diff mbox series

Patch

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8185a42848c50..4fc8085f3fe17 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -816,14 +816,20 @@  static bool need_to_create_worker(struct worker_pool *pool)
 	return need_more_worker(pool) && !may_start_working(pool);
 }
 
-/* Do we have too many workers and should some go away? */
-static bool too_many_workers(struct worker_pool *pool)
+/* How many idle workers should we get rid of, if any? */
+static unsigned int worker_cull_count(struct worker_pool *pool)
 {
 	bool managing = pool->flags & POOL_MANAGER_ACTIVE;
 	int nr_idle = pool->nr_idle + managing; /* manager is considered idle */
 	int nr_busy = pool->nr_workers - nr_idle;
 
-	return nr_idle > 2 && (nr_idle - 2) * MAX_IDLE_WORKERS_RATIO >= nr_busy;
+	lockdep_assert_held(&pool->lock);
+
+	/*
+	 * We keep at least 2 spare idle workers, but overall aim to keep at
+	 * most (1 / MAX_IDLE_WORKERS_RATIO) workers idle.
+	 */
+	return max(0, nr_idle - 2 - ((nr_busy - 1) / MAX_IDLE_WORKERS_RATIO));
 }
 
 /*
@@ -1806,7 +1812,7 @@  static void worker_enter_idle(struct worker *worker)
 	/* idle_list is LIFO */
 	list_add(&worker->entry, &pool->idle_list);
 
-	if (too_many_workers(pool) && !timer_pending(&pool->idle_timer))
+	if (worker_cull_count(pool) && !timer_pending(&pool->idle_timer))
 		mod_timer(&pool->idle_timer, jiffies + IDLE_WORKER_TIMEOUT);
 
 	/* Sanity check nr_running. */
@@ -2025,7 +2031,7 @@  static void idle_worker_timeout(struct timer_list *t)
 
 	raw_spin_lock_irq(&pool->lock);
 
-	while (too_many_workers(pool)) {
+	while (worker_cull_count(pool)) {
 		struct worker *worker;
 		unsigned long expires;