[PATCHSET 0/3] io-wq locking improvements

[PATCHSET 0/3] io-wq locking improvements

[Jens Axboe]

Hi,

In chatting with someone that was trying to use io_uring to read
mailddirs, they found that running a test case that does:

open file, statx file, read file, close file

The culprit here is statx, and argumentation aside on whether it makes
sense to statx in the first place, it does highlight that io-wq is
pretty locking intensive.

This (very lightly tested [1]) patchset attempts to improve this
situation, but reducing the frequency of grabbing wq->lock and
acct->lock.

The first patch gets rid of wq->lock on work insertion. io-wq grabs it
to iterate the free worker list, but that is not necessary.

Second patch reduces the frequency of acct->lock grabs, when we need to
run the queue and process new work. We currently grab the lock and check
for work, then drop it, then grab it again to process the work. That is
unneccessary.

Final patch just optimizes how we activate new workers. It's not related
to the locking itself, just reducing the overhead of activating a new
worker.

Running the above test case on a directory with 50K files, each being
between 10 and 4096 bytes, before these patches we get spend 160-170ms
running the workload. With this patchset, we spend 90-100ms doing the
same work. A bit of profile information is included in the patch commit
messages.

Can also be found here:

https://git.kernel.dk/cgit/linux/log/?h=io_uring-wq-lock

[1] Runs the test suite just fine, with PROVE_LOCKING enabled and raw
    lockdep as well.

-- 
Jens Axboe

[PATCH 1/3] io_uring/io-wq: don't grab wq->lock for worker activation

[Jens Axboe]

The worker free list is RCU protected, and checks for workers going away
when iterating it. There's no need to hold the wq->lock around the
lookup.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/io-wq.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 399e9a15c38d..3e7025b9e0dd 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -909,13 +909,10 @@ void io_wq_enqueue(struct io_wq *wq, struct io_wq_work *work)
 	clear_bit(IO_ACCT_STALLED_BIT, &acct->flags);
 	raw_spin_unlock(&acct->lock);
 
-	raw_spin_lock(&wq->lock);
 	rcu_read_lock();
 	do_create = !io_wq_activate_free_worker(wq, acct);
 	rcu_read_unlock();
 
-	raw_spin_unlock(&wq->lock);
-
 	if (do_create && ((work_flags & IO_WQ_WORK_CONCURRENT) ||
 	    !atomic_read(&acct->nr_running))) {
 		bool did_create;
-- 
2.40.1

[PATCH 2/3] io_uring/io-wq: reduce frequency of acct->lock acquisitions

[Jens Axboe]

When we check if we have work to run, we grab the acct lock, check,
drop it, and then return the result. If we do have work to run, then
running the work will again grab acct->lock and get the work item.

This causes us to grab acct->lock more frequently than we need to.
If we have work to do, have io_acct_run_queue() return with the acct
lock still acquired. io_worker_handle_work() is then always invoked
with the acct lock already held.

In a simple test cases that stats files (IORING_OP_STATX always hits
io-wq), we see a nice reduction in locking overhead with this change:

19.32%   -12.55%  [kernel.kallsyms]      [k] __cmpwait_case_32
20.90%   -12.07%  [kernel.kallsyms]      [k] queued_spin_lock_slowpath

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/io-wq.c | 47 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 3e7025b9e0dd..18a049fc53ef 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -232,17 +232,25 @@ static void io_worker_exit(struct io_worker *worker)
 	do_exit(0);
 }
 
-static inline bool io_acct_run_queue(struct io_wq_acct *acct)
+static inline bool __io_acct_run_queue(struct io_wq_acct *acct)
 {
-	bool ret = false;
+	return !test_bit(IO_ACCT_STALLED_BIT, &acct->flags) &&
+		!wq_list_empty(&acct->work_list);
+}
 
+/*
+ * If there's work to do, returns true with acct->lock acquired. If not,
+ * returns false with no lock held.
+ */
+static inline bool io_acct_run_queue(struct io_wq_acct *acct)
+	__acquires(&acct->lock)
+{
 	raw_spin_lock(&acct->lock);
-	if (!wq_list_empty(&acct->work_list) &&
-	    !test_bit(IO_ACCT_STALLED_BIT, &acct->flags))
-		ret = true;
-	raw_spin_unlock(&acct->lock);
+	if (__io_acct_run_queue(acct))
+		return true;
 
-	return ret;
+	raw_spin_unlock(&acct->lock);
+	return false;
 }
 
 /*
@@ -397,6 +405,7 @@ static void io_wq_dec_running(struct io_worker *worker)
 	if (!io_acct_run_queue(acct))
 		return;
 
+	raw_spin_unlock(&acct->lock);
 	atomic_inc(&acct->nr_running);
 	atomic_inc(&wq->worker_refs);
 	io_queue_worker_create(worker, acct, create_worker_cb);
@@ -521,9 +530,13 @@ static void io_assign_current_work(struct io_worker *worker,
 	raw_spin_unlock(&worker->lock);
 }
 
-static void io_worker_handle_work(struct io_worker *worker)
+/*
+ * Called with acct->lock held, drops it before returning
+ */
+static void io_worker_handle_work(struct io_wq_acct *acct,
+				  struct io_worker *worker)
+	__releases(&acct->lock)
 {
-	struct io_wq_acct *acct = io_wq_get_acct(worker);
 	struct io_wq *wq = worker->wq;
 	bool do_kill = test_bit(IO_WQ_BIT_EXIT, &wq->state);
 
@@ -537,7 +550,6 @@ static void io_worker_handle_work(struct io_worker *worker)
 		 * can't make progress, any work completion or insertion will
 		 * clear the stalled flag.
 		 */
-		raw_spin_lock(&acct->lock);
 		work = io_get_next_work(acct, worker);
 		raw_spin_unlock(&acct->lock);
 		if (work) {
@@ -591,6 +603,10 @@ static void io_worker_handle_work(struct io_worker *worker)
 					wake_up(&wq->hash->wait);
 			}
 		} while (work);
+
+		if (!__io_acct_run_queue(acct))
+			break;
+		raw_spin_lock(&acct->lock);
 	} while (1);
 }
 
@@ -611,8 +627,13 @@ static int io_wq_worker(void *data)
 		long ret;
 
 		set_current_state(TASK_INTERRUPTIBLE);
+
+		/*
+		 * If we have work to do, io_acct_run_queue() returns with
+		 * the acct->lock held. If not, it will drop it.
+		 */
 		while (io_acct_run_queue(acct))
-			io_worker_handle_work(worker);
+			io_worker_handle_work(acct, worker);
 
 		raw_spin_lock(&wq->lock);
 		/*
@@ -645,8 +666,8 @@ static int io_wq_worker(void *data)
 		}
 	}
 
-	if (test_bit(IO_WQ_BIT_EXIT, &wq->state))
-		io_worker_handle_work(worker);
+	if (test_bit(IO_WQ_BIT_EXIT, &wq->state) && io_acct_run_queue(acct))
+		io_worker_handle_work(acct, worker);
 
 	io_worker_exit(worker);
 	return 0;
-- 
2.40.1

[PATCH 3/3] io_uring/io-wq: don't gate worker wake up success on wake_up_process()

[Jens Axboe]

All we really care about is finding a free worker. If said worker is
already running, it's either starting new work already or it's just
finishing up existing work. For the latter, we'll be finding this work
item next anyway, and for the former, if the worker does go to sleep,
it'll create a new worker anyway as we have pending items.

This reduces try_to_wake_up() overhead considerably:

23.16%    -10.46%  [kernel.kallsyms]      [k] try_to_wake_up

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/io-wq.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 18a049fc53ef..2da0b1ba6a56 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -276,11 +276,14 @@ static bool io_wq_activate_free_worker(struct io_wq *wq,
 			io_worker_release(worker);
 			continue;
 		}
-		if (wake_up_process(worker->task)) {
-			io_worker_release(worker);
-			return true;
-		}
+		/*
+		 * If the worker is already running, it's either already
+		 * starting work or finishing work. In either case, if it does
+		 * to go sleep, we'll kick off a new task for this work anyway.
+		 */
+		wake_up_process(worker->task);
 		io_worker_release(worker);
+		return true;
 	}
 
 	return false;
-- 
2.40.1

Re: [PATCHSET 0/3] io-wq locking improvements

[Hao Xu]

On 8/10/23 03:43, Jens Axboe wrote:
> Hi,
> 
> In chatting with someone that was trying to use io_uring to read
> mailddirs, they found that running a test case that does:
> 
> open file, statx file, read file, close file
> 
> The culprit here is statx, and argumentation aside on whether it makes
> sense to statx in the first place, it does highlight that io-wq is
> pretty locking intensive.
> 
> This (very lightly tested [1]) patchset attempts to improve this
> situation, but reducing the frequency of grabbing wq->lock and
> acct->lock.
> 
> The first patch gets rid of wq->lock on work insertion. io-wq grabs it
> to iterate the free worker list, but that is not necessary.
> 
> Second patch reduces the frequency of acct->lock grabs, when we need to
> run the queue and process new work. We currently grab the lock and check
> for work, then drop it, then grab it again to process the work. That is
> unneccessary.
> 
> Final patch just optimizes how we activate new workers. It's not related
> to the locking itself, just reducing the overhead of activating a new
> worker.
> 
> Running the above test case on a directory with 50K files, each being
> between 10 and 4096 bytes, before these patches we get spend 160-170ms
> running the workload. With this patchset, we spend 90-100ms doing the
> same work. A bit of profile information is included in the patch commit
> messages.
> 
> Can also be found here:
> 
> https://git.kernel.dk/cgit/linux/log/?h=io_uring-wq-lock
> 
> [1] Runs the test suite just fine, with PROVE_LOCKING enabled and raw
>      lockdep as well.
> 

Haven't got time to test it, but looks good from the code itself.

Reviewed-by: Hao Xu <howeyxu@tencent.com>

Re: [PATCHSET 0/3] io-wq locking improvements

[Jens Axboe]

On 8/10/23 10:00 PM, Hao Xu wrote:
> On 8/10/23 03:43, Jens Axboe wrote:
>> Hi,
>>
>> In chatting with someone that was trying to use io_uring to read
>> mailddirs, they found that running a test case that does:
>>
>> open file, statx file, read file, close file
>>
>> The culprit here is statx, and argumentation aside on whether it makes
>> sense to statx in the first place, it does highlight that io-wq is
>> pretty locking intensive.
>>
>> This (very lightly tested [1]) patchset attempts to improve this
>> situation, but reducing the frequency of grabbing wq->lock and
>> acct->lock.
>>
>> The first patch gets rid of wq->lock on work insertion. io-wq grabs it
>> to iterate the free worker list, but that is not necessary.
>>
>> Second patch reduces the frequency of acct->lock grabs, when we need to
>> run the queue and process new work. We currently grab the lock and check
>> for work, then drop it, then grab it again to process the work. That is
>> unneccessary.
>>
>> Final patch just optimizes how we activate new workers. It's not related
>> to the locking itself, just reducing the overhead of activating a new
>> worker.
>>
>> Running the above test case on a directory with 50K files, each being
>> between 10 and 4096 bytes, before these patches we get spend 160-170ms
>> running the workload. With this patchset, we spend 90-100ms doing the
>> same work. A bit of profile information is included in the patch commit
>> messages.
>>
>> Can also be found here:
>>
>> https://git.kernel.dk/cgit/linux/log/?h=io_uring-wq-lock
>>
>> [1] Runs the test suite just fine, with PROVE_LOCKING enabled and raw
>>      lockdep as well.
>>
> 
> Haven't got time to test it, but looks good from the code itself.
> 
> Reviewed-by: Hao Xu <howeyxu@tencent.com>

Thanks, added.

-- 
Jens Axboe