task_put batching

PATCH Applied Pavel Begunkov 2 participants 12 messages Jul 18, 2020 – Jul 20, 2020

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Pavel Begunkov

author

2020-07-18 08:34 UTC #0 lore raw

cover task_put batching

For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)

before:

io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%

after:

io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%

Pavel Begunkov (2):
tasks: add put_task_struct_many()
io_uring: batch put_task_struct()

 fs/io_uring.c              | 28 ++++++++++++++++++++++++++--
 include/linux/sched/task.h |  6 ++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-07-18 08:34 UTC #1 lore raw

PATCH 1/2 tasks: add put_task_struct_many()

put_task_struct_many() is as put_task_struct() but puts several references at once. Useful to batching it.

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/linux/sched/task.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/sched/task.h +6 -0

index 38359071236a..1301077f9c24 100644
@@ -126,6 +126,12 @@ static inline void put_task_struct(struct task_struct *t)
 		__put_task_struct(t);
 }
 
+static inline void put_task_struct_many(struct task_struct *t, int nr)
+{
+	if (refcount_sub_and_test(nr, &t->usage))
+		__put_task_struct(t);
+}
+
 void put_task_struct_rcu_user(struct task_struct *task);
 
 #ifdef CONFIG_ARCH_WANTS_DYNAMIC_TASK_STRUCT

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-07-18 08:34 UTC #2 lore raw

PATCH 2/2 io_uring: batch put_task_struct()

In reply to Pavel Begunkov

As every iopoll request have a task ref, it becomes expensive to put them one by one, instead we can put several at once integrating that into io_req_free_batch().

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

fs/io_uring.c +26 -2

index 57e1f26b6a6b..b52aa0d8b09d 100644
@@ -1543,7 +1543,6 @@ static void io_dismantle_req(struct io_kiocb *req)
 		kfree(req->io);
 	if (req->file)
 		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
-	__io_put_req_task(req);
 	io_req_clean_work(req);
 
 	if (req->flags & REQ_F_INFLIGHT) {

@@ -1563,6 +1562,7 @@ static void __io_free_req(struct io_kiocb *req)
 	struct io_ring_ctx *ctx;
 
 	io_dismantle_req(req);
+	__io_put_req_task(req);
 	ctx = req->ctx;
 	if (likely(!io_is_fallback_req(req)))
 		kmem_cache_free(req_cachep, req);

@@ -1806,8 +1806,18 @@ static void io_free_req(struct io_kiocb *req)
 struct req_batch {
 	void *reqs[IO_IOPOLL_BATCH];
 	int to_free;
+
+	struct task_struct	*task;
+	int			task_refs;
 };
 
+static inline void io_init_req_batch(struct req_batch *rb)
+{
+	rb->to_free = 0;
+	rb->task_refs = 0;
+	rb->task = NULL;
+}
+
 static void __io_req_free_batch_flush(struct io_ring_ctx *ctx,
 				      struct req_batch *rb)
 {

@@ -1821,6 +1831,10 @@ static void io_req_free_batch_finish(struct io_ring_ctx *ctx,
 {
 	if (rb->to_free)
 		__io_req_free_batch_flush(ctx, rb);
+	if (rb->task) {
+		put_task_struct_many(rb->task, rb->task_refs);
+		rb->task = NULL;
+	}
 }
 
 static void io_req_free_batch(struct req_batch *rb, struct io_kiocb *req)

@@ -1832,6 +1846,16 @@ static void io_req_free_batch(struct req_batch *rb, struct io_kiocb *req)
 	if (req->flags & REQ_F_LINK_HEAD)
 		io_queue_next(req);
 
+	if (req->flags & REQ_F_TASK_PINNED) {
+		if (req->task != rb->task && rb->task) {
+			put_task_struct_many(rb->task, rb->task_refs);
+			rb->task = req->task;
+			rb->task_refs = 0;
+		}
+		rb->task_refs++;
+		req->flags &= ~REQ_F_TASK_PINNED;
+	}
+
 	io_dismantle_req(req);
 	rb->reqs[rb->to_free++] = req;
 	if (unlikely(rb->to_free == ARRAY_SIZE(rb->reqs)))

@@ -1977,7 +2001,7 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 	/* order with ->result store in io_complete_rw_iopoll() */
 	smp_rmb();
 
-	rb.to_free = 0;
+	io_init_req_batch(&rb);
 	while (!list_empty(done)) {
 		int cflags = 0;

Signature · 1 line

2.24.0

Jens Axboe

maintainer Applied

2020-07-18 14:37 UTC #3 lore raw

In reply to Pavel Begunkov

Quoted 8 lines · Pavel Begunkov

For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)

Quoted 7 lines

before:

io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%

after:

io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%

Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.

I have applied it, thanks.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-07-19 11:17 UTC #4 lore raw

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%

Quoted 5 lines

Quoted 3 lines
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Still not seeing a win here, but it's clean and it _should_ work. For

Well, if this thing is useful, it'd be hard to quantify, because active polling would hide it. I think, it'd need to apply a lot of isolated pressure on cache synchronisation (e.g. spam with barriers), or try to create and measure an atomic heavy task pinned to another core. Don't worth the effort IMHO.
`
Just out of curiosity, let me ask how do you test it?
- is it a VM?
- how many cores and threads do you use?
- how many io_uring instances you have? Per thread?
- Is it all goes to a single NVMe SSD?

Quoted 3 lines

some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.

I'll take a look whether I can reproduce.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-07-19 18:50 UTC #5 lore raw

Quoted 14 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe
Quoted 12 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Still not seeing a win here, but it's clean and it _should_ work. For
Well, if this thing is useful, it'd be hard to quantify, because active polling would hide it. I think, it'd need to apply a lot of isolated

It should be very visible in my setup, as we're CPU limited, not device limited. Hence it makes it very easy to show CPU gains, as they directly translate into improved performance.

Quoted 9 lines

pressure on cache synchronisation (e.g. spam with barriers), or try to create and measure an atomic heavy task pinned to another core. Don't worth the effort IMHO.
`
Just out of curiosity, let me ask how do you test it?
- is it a VM?
- how many cores and threads do you use?
- how many io_uring instances you have? Per thread?
- Is it all goes to a single NVMe SSD?

It's not a VM, it's a normal box. I'm using just one CPU, one thread, and just one NVMe device. That's my goto test for seeing if we reclaimed some CPU cycles.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-07-20 14:20 UTC #6 lore raw

Quoted 19 lines · Jens Axboe

Quoted 18 lines · Pavel Begunkov
Quoted 17 lines · Jens Axboe
Quoted 15 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Still not seeing a win here, but it's clean and it _should_ work. For
Well, if this thing is useful, it'd be hard to quantify, because active polling would hide it. I think, it'd need to apply a lot of isolated
It should be very visible in my setup, as we're CPU limited, not device limited. Hence it makes it very easy to show CPU gains, as they directly translate into improved performance.

IIRC, atomics for x64 in a single thread don't hurt too much. Disregarding this patch, it would be good to have a many-threaded benchmark to look after scalability.

Quoted 10 lines

Quoted 9 lines
pressure on cache synchronisation (e.g. spam with barriers), or try to create and measure an atomic heavy task pinned to another core. Don't worth the effort IMHO.
`
Just out of curiosity, let me ask how do you test it?
- is it a VM?
- how many cores and threads do you use?
- how many io_uring instances you have? Per thread?
- Is it all goes to a single NVMe SSD?

Quoted 3 lines

It's not a VM, it's a normal box. I'm using just one CPU, one thread, and just one NVMe device. That's my goto test for seeing if we reclaimed some CPU cycles.

Got it, thanks

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2020-07-20 15:24 UTC #7 lore raw

In reply to Jens Axboe

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%

Quoted 8 lines

Quoted 3 lines
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.

It grows because the patch is garbage, the second condition is always false. See the diff. Could you please drop both patches?

fs/io_uring.c +3 -2

index 87a772eee0c4..2f02f85269eb 100644
@@ -1847,8 +1847,9 @@ static void io_req_free_batch(struct req_batch *rb, struct io_kiocb *req)
                io_queue_next(req);
 
        if (req->flags & REQ_F_TASK_PINNED) {
-               if (req->task != rb->task && rb->task) {
-                       put_task_struct_many(rb->task, rb->task_refs);
+               if (req->task != rb->task) {
+                       if (rb->task)
+                               put_task_struct_many(rb->task, rb->task_refs);
                        rb->task = req->task;
                        rb->task_refs = 0;
                }

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-07-20 16:39 UTC #8 lore raw

Quoted 18 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe
Quoted 15 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%

Quoted 7 lines

Quoted 4 lines
Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.
It grows because the patch is garbage, the second condition is always false. See the diff. Could you please drop both patches?

Hah, indeed. With this on top, it looks like it should in terms of performance and profiles.

I can just fold this into the existing one, if you'd like.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-07-20 16:08 UTC #9 lore raw

Quoted 24 lines · Jens Axboe

Quoted 23 lines · Pavel Begunkov
Quoted 21 lines · Jens Axboe
Quoted 8 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
Quoted 7 lines
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.

Quoted 5 lines

Quoted 2 lines
It grows because the patch is garbage, the second condition is always false. See the diff. Could you please drop both patches?
Hah, indeed. With this on top, it looks like it should in terms of performance and profiles.

It just shows, that it doesn't really matters for a single-threaded app, as expected. Worth to throw some contention though. I'll think about finding some time to get/borrow a multi-threaded one.

Quoted 2 lines

I can just fold this into the existing one, if you'd like.

Would be nice. I'm going to double-check the counter and re-measure anyway. BTW, how did you find it? A tool or a proc file would be awesome.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-07-20 16:19 UTC #10 lore raw

Quoted 28 lines · Pavel Begunkov

Quoted 27 lines · Jens Axboe
Quoted 25 lines · Pavel Begunkov
Quoted 13 lines · Jens Axboe
Quoted 12 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%
Quoted 8 lines
Quoted 3 lines
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.
It grows because the patch is garbage, the second condition is always false. See the diff. Could you please drop both patches?

Quoted 6 lines

Quoted 2 lines
Hah, indeed. With this on top, it looks like it should in terms of performance and profiles.
It just shows, that it doesn't really matters for a single-threaded app, as expected. Worth to throw some contention though. I'll think about finding some time to get/borrow a multi-threaded one.

But it kind of did here, ended up being mostly a wash in terms of perf here as my testing reported. With the incremental applied, it's up a bit over before the task put batching.

Quoted 4 lines

Quoted 1 line
I can just fold this into the existing one, if you'd like.
Would be nice. I'm going to double-check the counter and re-measure anyway. BTW, how did you find it? A tool or a proc file would be awesome.

For this kind of testing, I just use t/io_uring out of fio. It's probably the lowest overhead kind of tool:

# sudo taskset -c 0 t/io_uring -b512 -p1 /dev/nvme2n1

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-07-20 16:44 UTC #11 lore raw

Quoted 32 lines · Jens Axboe

Quoted 31 lines · Pavel Begunkov
Quoted 29 lines · Jens Axboe
Quoted 18 lines · Pavel Begunkov
Quoted 17 lines · Jens Axboe
Quoted 15 lines · Pavel Begunkov
For my a bit exaggerated test case perf continues to show high CPU cosumption by io_dismantle(), and so calling it io_iopoll_complete(). Even though the patch doesn't yield throughput increase for my setup, probably because the effect is hidden behind polling, but it definitely improves relative percentage. And the difference should only grow with increasing number of CPUs. Another reason to have this is that atomics may affect other parallel tasks (e.g. which doesn't use io_uring)
before:
io_iopoll_complete: 5.29%
io_dismantle_req:   2.16%
after:
io_iopoll_complete: 3.39%
io_dismantle_req:   0.465%
Quoted 7 lines
Quoted 4 lines
Still not seeing a win here, but it's clean and it _should_ work. For some reason I end up getting the offset in task ref put growing the fput_many(). Which doesn't (on the surface) make a lot of sense, but may just mean that we have some weird side effects.
It grows because the patch is garbage, the second condition is always false. See the diff. Could you please drop both patches?
Hah, indeed. With this on top, it looks like it should in terms of performance and profiles.

Quoted 7 lines

Quoted 3 lines
It just shows, that it doesn't really matters for a single-threaded app, as expected. Worth to throw some contention though. I'll think about finding some time to get/borrow a multi-threaded one.
But it kind of did here, ended up being mostly a wash in terms of perf here as my testing reported. With the incremental applied, it's up a bit over before the task put batching.

Hmm, I need to get used to sensitivity of your box, that's a good one!

Do you mean, that the buggy version without atomics was on par comparing to not having it at all, but the fixed/updated one is a bit faster? Sounds like micro binary differences, like a bit altered jumps.

It'd also interesting to know, what degree of coalescing in io_iopoll_complete() you manage to get with that.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
I can just fold this into the existing one, if you'd like.
Would be nice. I'm going to double-check the counter and re-measure anyway. BTW, how did you find it? A tool or a proc file would be awesome.
For this kind of testing, I just use t/io_uring out of fio. It's probably the lowest overhead kind of tool:
# sudo taskset -c 0 t/io_uring -b512 -p1 /dev/nvme2n1

I use io_uring-bench.c from time to time, but didn't know it continued living under fio/t/. Thanks! I also put it under cshield for more consistency, but it looks like io-wq ignores that.

Signature · 1 line

Pavel Begunkov