public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io-wq: check max_worker limits if a worker transitions bound state
@ 2021-08-29 22:19 Jens Axboe
  2021-08-30  3:06 ` Hao Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2021-08-29 22:19 UTC (permalink / raw)
  To: io-uring

For the two places where new workers are created, we diligently check if
we are allowed to create a new worker. If we're currently at the limit
of how many workers of a given type we can have, then we don't create
any new ones.

If you have a mixed workload with various types of bound and unbounded
work, then it can happen that a worker finishes one type of work and
is then transitioned to the other type. For this case, we don't check
if we are actually allowed to do so. This can cause io-wq to temporarily
exceed the allowed number of workers for a given type.

When retrieving work, check that the types match. If they don't, check
if we are allowed to transition to the other type. If not, then don't
handle the new work.

Cc: [email protected]
Reported-by: Johannes Lundberg <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>

---

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 4b5fc621ab39..dced22288983 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -424,7 +424,31 @@ static void io_wait_on_hash(struct io_wqe *wqe, unsigned int hash)
 	spin_unlock(&wq->hash->wait.lock);
 }
 
-static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
+/*
+ * We can always run the work if the worker is currently the same type as
+ * the work (eg both are bound, or both are unbound). If they are not the
+ * same, only allow it if incrementing the worker count would be allowed.
+ */
+static bool io_worker_can_run_work(struct io_worker *worker,
+				   struct io_wq_work *work)
+{
+	struct io_wqe_acct *acct;
+
+	if ((worker->flags & IO_WORKER_F_BOUND) &&
+	    !(work->flags & IO_WQ_WORK_UNBOUND))
+		return true;
+	else if (!(worker->flags & IO_WORKER_F_BOUND) &&
+		 (work->flags & IO_WQ_WORK_UNBOUND))
+		return true;
+
+	/* not the same type, check if we'd go over the limit */
+	acct = io_work_get_acct(worker->wqe, work);
+	return acct->nr_workers < acct->max_workers;
+}
+
+static struct io_wq_work *io_get_next_work(struct io_wqe *wqe,
+					   struct io_worker *worker,
+					   bool *stalled)
 	__must_hold(wqe->lock)
 {
 	struct io_wq_work_node *node, *prev;
@@ -436,6 +460,9 @@ static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
 
 		work = container_of(node, struct io_wq_work, list);
 
+		if (!io_worker_can_run_work(worker, work))
+			break;
+
 		/* not hashed, can run anytime */
 		if (!io_wq_is_hashed(work)) {
 			wq_list_del(&wqe->work_list, node, prev);
@@ -462,6 +489,7 @@ static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
 		raw_spin_unlock(&wqe->lock);
 		io_wait_on_hash(wqe, stall_hash);
 		raw_spin_lock(&wqe->lock);
+		*stalled = true;
 	}
 
 	return NULL;
@@ -501,6 +529,7 @@ static void io_worker_handle_work(struct io_worker *worker)
 
 	do {
 		struct io_wq_work *work;
+		bool stalled;
 get_next:
 		/*
 		 * If we got some work, mark us as busy. If we didn't, but
@@ -509,10 +538,11 @@ static void io_worker_handle_work(struct io_worker *worker)
 		 * can't make progress, any work completion or insertion will
 		 * clear the stalled flag.
 		 */
-		work = io_get_next_work(wqe);
+		stalled = false;
+		work = io_get_next_work(wqe, worker, &stalled);
 		if (work)
 			__io_worker_busy(wqe, worker, work);
-		else if (!wq_list_empty(&wqe->work_list))
+		else if (stalled)
 			wqe->flags |= IO_WQE_FLAG_STALLED;
 
 		raw_spin_unlock_irq(&wqe->lock);

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] io-wq: check max_worker limits if a worker transitions bound state
  2021-08-29 22:19 [PATCH] io-wq: check max_worker limits if a worker transitions bound state Jens Axboe
@ 2021-08-30  3:06 ` Hao Xu
  2021-08-30 12:20   ` Jens Axboe
  0 siblings, 1 reply; 3+ messages in thread
From: Hao Xu @ 2021-08-30  3:06 UTC (permalink / raw)
  To: Jens Axboe, io-uring

在 2021/8/30 上午6:19, Jens Axboe 写道:
> For the two places where new workers are created, we diligently check if
> we are allowed to create a new worker. If we're currently at the limit
> of how many workers of a given type we can have, then we don't create
> any new ones.
> 
> If you have a mixed workload with various types of bound and unbounded
> work, then it can happen that a worker finishes one type of work and
> is then transitioned to the other type. For this case, we don't check
> if we are actually allowed to do so. This can cause io-wq to temporarily
> exceed the allowed number of workers for a given type.
> 
> When retrieving work, check that the types match. If they don't, check
> if we are allowed to transition to the other type. If not, then don't
> handle the new work.
> 
> Cc: [email protected]
> Reported-by: Johannes Lundberg <[email protected]>
> Signed-off-by: Jens Axboe <[email protected]>
> 
> ---
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 4b5fc621ab39..dced22288983 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -424,7 +424,31 @@ static void io_wait_on_hash(struct io_wqe *wqe, unsigned int hash)
>   	spin_unlock(&wq->hash->wait.lock);
>   }
>   
> -static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
> +/*
> + * We can always run the work if the worker is currently the same type as
> + * the work (eg both are bound, or both are unbound). If they are not the
> + * same, only allow it if incrementing the worker count would be allowed.
> + */
> +static bool io_worker_can_run_work(struct io_worker *worker,
> +				   struct io_wq_work *work)
> +{
> +	struct io_wqe_acct *acct;
> +
> +	if ((worker->flags & IO_WORKER_F_BOUND) &&
> +	    !(work->flags & IO_WQ_WORK_UNBOUND))
> +		return true;
> +	else if (!(worker->flags & IO_WORKER_F_BOUND) &&
> +		 (work->flags & IO_WQ_WORK_UNBOUND))
> +		return true;

How about:
bool a = !(worker->flags & IO_WORKER_F_BOUND);
bool b = !(work->flags & IO_WQ_WORK_UNBOUND);

if (a != b)
     return true;
> +
> +	/* not the same type, check if we'd go over the limit */
> +	acct = io_work_get_acct(worker->wqe, work);
> +	return acct->nr_workers < acct->max_workers;
> +}
> +



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] io-wq: check max_worker limits if a worker transitions bound state
  2021-08-30  3:06 ` Hao Xu
@ 2021-08-30 12:20   ` Jens Axboe
  0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2021-08-30 12:20 UTC (permalink / raw)
  To: Hao Xu, io-uring

On 8/29/21 9:06 PM, Hao Xu wrote:
> 在 2021/8/30 上午6:19, Jens Axboe 写道:
>> For the two places where new workers are created, we diligently check if
>> we are allowed to create a new worker. If we're currently at the limit
>> of how many workers of a given type we can have, then we don't create
>> any new ones.
>>
>> If you have a mixed workload with various types of bound and unbounded
>> work, then it can happen that a worker finishes one type of work and
>> is then transitioned to the other type. For this case, we don't check
>> if we are actually allowed to do so. This can cause io-wq to temporarily
>> exceed the allowed number of workers for a given type.
>>
>> When retrieving work, check that the types match. If they don't, check
>> if we are allowed to transition to the other type. If not, then don't
>> handle the new work.
>>
>> Cc: [email protected]
>> Reported-by: Johannes Lundberg <[email protected]>
>> Signed-off-by: Jens Axboe <[email protected]>
>>
>> ---
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index 4b5fc621ab39..dced22288983 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -424,7 +424,31 @@ static void io_wait_on_hash(struct io_wqe *wqe, unsigned int hash)
>>   	spin_unlock(&wq->hash->wait.lock);
>>   }
>>   
>> -static struct io_wq_work *io_get_next_work(struct io_wqe *wqe)
>> +/*
>> + * We can always run the work if the worker is currently the same type as
>> + * the work (eg both are bound, or both are unbound). If they are not the
>> + * same, only allow it if incrementing the worker count would be allowed.
>> + */
>> +static bool io_worker_can_run_work(struct io_worker *worker,
>> +				   struct io_wq_work *work)
>> +{
>> +	struct io_wqe_acct *acct;
>> +
>> +	if ((worker->flags & IO_WORKER_F_BOUND) &&
>> +	    !(work->flags & IO_WQ_WORK_UNBOUND))
>> +		return true;
>> +	else if (!(worker->flags & IO_WORKER_F_BOUND) &&
>> +		 (work->flags & IO_WQ_WORK_UNBOUND))
>> +		return true;
> 
> How about:
> bool a = !(worker->flags & IO_WORKER_F_BOUND);
> bool b = !(work->flags & IO_WQ_WORK_UNBOUND);
> 
> if (a != b)
>      return true;

Yeah good point, I'll change it to be:

if (!(worker->flags & IO_WORKER_F_BOUND) !=                             
    !(work->flags & IO_WQ_WORK_UNBOUND))                                
         return 1;                                    

return acct->nr_workers < acct->max_workers;

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-30 12:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-29 22:19 [PATCH] io-wq: check max_worker limits if a worker transitions bound state Jens Axboe
2021-08-30  3:06 ` Hao Xu
2021-08-30 12:20   ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox