fix hangs with shared sqpoll

PATCH Superseded Pavel Begunkov 5 participants 17 messages Apr 16, 2021 – Apr 19, 2021 1 message not in the archive

Subsystems (4)

VFS Library code io_uring Per-CPU memory allocator

lore

Share & tools

lore

Pavel Begunkov

author

2021-04-16 00:27 UTC #0 lore raw

cover fix hangs with shared sqpoll

Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.

Pavel Begunkov (2):
percpu_ref: add percpu_ref_atomic_count()
io_uring: fix shared sqpoll cancellation hangs

 fs/io_uring.c                   |  5 +++--
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2021-04-16 00:27 UTC #1 lore raw

PATCH 1/2 percpu_ref: add percpu_ref_atomic_count()

Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.

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

---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

include/linux/percpu-refcount.h +1 -0

index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref

lib/percpu-refcount.c +26 -0

index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;
+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;
+	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
+
+	return count;
+}
+
 /**
  * percpu_ref_reinit - re-initialize a percpu refcount
  * @ref: perpcu_ref to re-initialize

Signature · 1 line

2.24.0

Dennis Zhou

maintainer

2021-04-16 04:45 UTC #2 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Hello,

Quoted 41 lines · Pavel Begunkov

Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.

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

---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

include/linux/percpu-refcount.h +1 -0

index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref

lib/percpu-refcount.c +11 -0

index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;

lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 448

Quoted 10 lines

lib/percpu-refcount.c +10 -0

+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;

Sorry I missed Jens' patch before and also the update to percpu_ref. However, I feel like I'm missing something. This isn't entirely related to your patch, but I'm not following why percpu_count_ptr stores the excess count of an exited percpu_ref and doesn't warn when it's not zero. It seems like this should be an error if it's not 0?

Granted we have made some contract with the user to do the right thing, but say someone does mess up, we don't indicate to them hey this ref is actually dead and if they're waiting for it to go to 0, it never will.

Quoted 11 lines

lib/percpu-refcount.c +5 -0

+	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
+
+	return count;
+}
+
 /**
  * percpu_ref_reinit - re-initialize a percpu refcount
  * @ref: perpcu_ref to re-initialize

Signature · 1 line

2.24.0

Thanks,
Dennis

Pavel Begunkov

author

2021-04-16 13:20 UTC #3 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Quoted 61 lines · Dennis Zhou

Hello,
Quoted 41 lines · Pavel Begunkov
Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.
Signed-off-by Pavel Begunkov <[email protected]>
---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/percpu-refcount.h +1 -0
index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref
lib/percpu-refcount.c +11 -0
index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;
lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 448
Quoted 10 lines
lib/percpu-refcount.c +10 -0
+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;
Sorry I missed Jens' patch before and also the update to percpu_ref. However, I feel like I'm missing something. This isn't entirely related to your patch, but I'm not following why percpu_count_ptr stores the excess count of an exited percpu_ref and doesn't warn when it's not zero. It seems like this should be an error if it's not 0?

Quoted 3 lines

Granted we have made some contract with the user to do the right thing, but say someone does mess up, we don't indicate to them hey this ref is actually dead and if they're waiting for it to go to 0, it never will.

fwiw, I copied is_zero, but skimming through the code don't immediately see myself why it is so...

Cc Ming, he split out some parts of it to dynamic allocation not too long ago, maybe he knows the trick.

Quoted 12 lines

lib/percpu-refcount.c +5 -0

+	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
+
+	return count;
+}
+
 /**
  * percpu_ref_reinit - re-initialize a percpu refcount
  * @ref: perpcu_ref to re-initialize

Signature · 1 line

2.24.0

Signature · 1 line

Pavel Begunkov

Ming Lei

2021-04-16 14:10 UTC #4 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Quoted 66 lines · Pavel Begunkov

Quoted 61 lines · Dennis Zhou
Hello,
Quoted 41 lines · Pavel Begunkov
Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.
Signed-off-by Pavel Begunkov <[email protected]>
---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/percpu-refcount.h +1 -0
index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref
lib/percpu-refcount.c +11 -0
index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;
lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 448
Quoted 10 lines
lib/percpu-refcount.c +10 -0
+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;
Sorry I missed Jens' patch before and also the update to percpu_ref. However, I feel like I'm missing something. This isn't entirely related to your patch, but I'm not following why percpu_count_ptr stores the excess count of an exited percpu_ref and doesn't warn when it's not zero. It seems like this should be an error if it's not 0?
Quoted 3 lines
Granted we have made some contract with the user to do the right thing, but say someone does mess up, we don't indicate to them hey this ref is actually dead and if they're waiting for it to go to 0, it never will.

Quoted 5 lines

fwiw, I copied is_zero, but skimming through the code don't immediately see myself why it is so...
Cc Ming, he split out some parts of it to dynamic allocation not too long ago, maybe he knows the trick.

I remembered that percpu_ref_is_zero() can be called even after percpu_ref_exit() returns, and looks percpu_ref_is_zero() isn't classified into 'active use'.

Thanks,
Ming

Dennis Zhou

maintainer

2021-04-16 14:37 UTC #5 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Quoted 67 lines · Ming Lei

Quoted 66 lines · Pavel Begunkov
Quoted 64 lines · Dennis Zhou
Hello,
Quoted 41 lines · Pavel Begunkov
Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.
Signed-off-by Pavel Begunkov <[email protected]>
---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/percpu-refcount.h +1 -0
index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref
lib/percpu-refcount.c +11 -0
index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;
lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 448
Quoted 10 lines
lib/percpu-refcount.c +10 -0
+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;
Sorry I missed Jens' patch before and also the update to percpu_ref. However, I feel like I'm missing something. This isn't entirely related to your patch, but I'm not following why percpu_count_ptr stores the excess count of an exited percpu_ref and doesn't warn when it's not zero. It seems like this should be an error if it's not 0?
Granted we have made some contract with the user to do the right thing, but say someone does mess up, we don't indicate to them hey this ref is actually dead and if they're waiting for it to go to 0, it never will.

Quoted 9 lines

Quoted 5 lines
fwiw, I copied is_zero, but skimming through the code don't immediately see myself why it is so...
Cc Ming, he split out some parts of it to dynamic allocation not too long ago, maybe he knows the trick.
I remembered that percpu_ref_is_zero() can be called even after percpu_ref_exit() returns, and looks percpu_ref_is_zero() isn't classified into 'active use'.

Looking at the commit prior, it seems like percpu_ref_is_zero() was subject to the usual init and exit lifetime. I guess I'm just not convinced it should ever be > 0. I'll think about it a little longer and might fix it.

Thanks,
Dennis

Ming Lei

2021-04-19 02:03 UTC #6 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Quoted 74 lines · Dennis Zhou

Quoted 73 lines · Ming Lei
Quoted 71 lines · Pavel Begunkov
Quoted 61 lines · Dennis Zhou
Hello,
Quoted 41 lines · Pavel Begunkov
Add percpu_ref_atomic_count(), which returns number of references of a percpu_ref switched prior into atomic mode, so the caller is responsible to make sure it's in the right mode.
Signed-off-by Pavel Begunkov <[email protected]>
---
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/percpu-refcount.h +1 -0
index 16c35a728b4c..0ff40e79efa2 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref
lib/percpu-refcount.c +11 -0
index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
+unsigned long percpu_ref_atomic_count(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	unsigned long count, flags;
lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 448
Quoted 10 lines
lib/percpu-refcount.c +10 -0
+
+	if (WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count)))
+		return -1UL;
+
+	/* protect us from being destroyed */
+	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
+	if (ref->data)
+		count = atomic_long_read(&ref->data->count);
+	else
+		count = ref->percpu_count_ptr >> __PERCPU_REF_FLAG_BITS;
Sorry I missed Jens' patch before and also the update to percpu_ref. However, I feel like I'm missing something. This isn't entirely related to your patch, but I'm not following why percpu_count_ptr stores the excess count of an exited percpu_ref and doesn't warn when it's not zero. It seems like this should be an error if it's not 0?
Quoted 3 lines
Granted we have made some contract with the user to do the right thing, but say someone does mess up, we don't indicate to them hey this ref is actually dead and if they're waiting for it to go to 0, it never will.
fwiw, I copied is_zero, but skimming through the code don't immediately see myself why it is so...
Cc Ming, he split out some parts of it to dynamic allocation not too long ago, maybe he knows the trick.

Quoted 8 lines

Quoted 3 lines
I remembered that percpu_ref_is_zero() can be called even after percpu_ref_exit() returns, and looks percpu_ref_is_zero() isn't classified into 'active use'.
Looking at the commit prior, it seems like percpu_ref_is_zero() was subject to the usual init and exit lifetime. I guess I'm just not convinced it should ever be > 0. I'll think about it a little longer and might fix it.

There may not be > 0 at that time, but it was allowed for percpu_ref_is_zero() to read un-initialized refcount, and there was such kernel oops report:

https://lore.kernel.org/lkml/[email protected]/#r

Thanks,
Ming

Bart Van Assche

2021-04-16 15:31 UTC #7 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

In reply to Pavel Begunkov

Quoted 5 lines · Pavel Begunkov

lib/percpu-refcount.c +0 -0
index a1071cdefb5a..56286995e2b8 100644

lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 434

Quoted 10 lines

lib/percpu-refcount.c +7 -0

@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);
 
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */

How about using the name percpu_ref_read() instead of percpu_ref_atomic_count()?

Thanks,

Bart.

Jens Axboe

maintainer

2021-04-16 15:34 UTC #8 lore raw

Re: [PATCH 1/2] percpu_ref: add percpu_ref_atomic_count()

Quoted 9 lines · Bart Van Assche

Quoted 8 lines · Pavel Begunkov

lib/percpu-refcount.c +0 -0

index a1071cdefb5a..56286995e2b8 100644
@@ -425,6 +425,32 @@ bool percpu_ref_is_zero(struct percpu_ref *ref)
 }
 EXPORT_SYMBOL_GPL(percpu_ref_is_zero);

Quoted 10 lines

lib/percpu-refcount.c bool percpu_ref_is_zero(struct percpu_ref *ref) · line 434
Quoted 7 lines
lib/percpu-refcount.c +7 -0
+/**
+ * percpu_ref_atomic_count - returns number of left references
+ * @ref: percpu_ref to test
+ *
+ * This function is safe to call as long as @ref is switch into atomic mode,
+ * and is between init and exit.
+ */
How about using the name percpu_ref_read() instead of percpu_ref_atomic_count()?

Not sure we're going that route, but in any case, I think it's important to have it visibly require the ref to be in atomic mode. Maybe percpu_ref_read_atomic() would be better, but I do think 'atomic' has to be in the name.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2021-04-16 00:27 UTC #9 lore raw

PATCH 2/2 io_uring: fix shared sqpoll cancellation hangs

In reply to Pavel Begunkov

[  736.982891] INFO: task iou-sqp-4294:4295 blocked for more than 122 seconds.
[  736.982897] Call Trace:
[  736.982901]  schedule+0x68/0xe0
[  736.982903]  io_uring_cancel_sqpoll+0xdb/0x110
[  736.982908]  io_sqpoll_cancel_cb+0x24/0x30
[  736.982911]  io_run_task_work_head+0x28/0x50
[  736.982913]  io_sq_thread+0x4e3/0x720

We call io_uring_cancel_sqpoll() one by one for each ctx either in sq_thread() itself or via task works, and it's intended to cancel all requests of a specified context. However the function uses per-task counters to track the number of inflight requests, so it counts more requests than available via currect io_uring ctx and goes to sleep for them to appear (e.g. from IRQ), that will never happen.

Reported-by Joakim Hassila <[email protected]>

Reported-by Jens Axboe <[email protected]>

Fixes 37d1e2e3642e2 ("io_uring: move SQPOLL thread io-wq forked worker")

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

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

fs/io_uring.c +3 -2

index dff34975d86b..c1c843b044c0 100644
@@ -9000,10 +9000,11 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 
 	WARN_ON_ONCE(!sqd || ctx->sq_data->thread != current);
 
+	percpu_ref_switch_to_atomic_sync(&ctx->refs);
 	atomic_inc(&tctx->in_idle);
 	do {
 		/* read completions before cancelations */
-		inflight = tctx_inflight(tctx);
+		inflight = percpu_ref_atomic_count(&ctx->refs);
 		if (!inflight)
 			break;
 		io_uring_try_cancel_requests(ctx, current, NULL);

@@ -9014,7 +9015,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 		 * avoids a race where a completion comes in before we did
 		 * prepare_to_wait().
 		 */
-		if (inflight == tctx_inflight(tctx))
+		if (inflight == percpu_ref_atomic_count(&ctx->refs))
 			schedule();
 		finish_wait(&tctx->wait, &wait);
 	} while (1);

Signature · 1 line

2.24.0

Pavel Begunkov

author

2021-04-16 00:30 UTC #10 lore raw

In reply to Pavel Begunkov

Quoted 1 line · Pavel Begunkov

Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.

1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead

Quoted 10 lines

Pavel Begunkov (2):
percpu_ref: add percpu_ref_atomic_count()
io_uring: fix shared sqpoll cancellation hangs
 fs/io_uring.c                   |  5 +++--
 include/linux/percpu-refcount.h |  1 +
 lib/percpu-refcount.c           | 26 ++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2021-04-16 13:04 UTC #11 lore raw

Quoted 6 lines · Pavel Begunkov

Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead

I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.

Totally untested...

fs/io_uring.c +2 -2

index 6c182a3a221b..9edbcf01ea49 100644
@@ -8928,7 +8928,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 	atomic_inc(&tctx->in_idle);
 	do {
 		/* read completions before cancelations */
-		inflight = tctx_inflight(tctx, false);
+		inflight = percpu_ref_sum(&ctx->refs);
 		if (!inflight)
 			break;
 		io_uring_try_cancel_requests(ctx, current, NULL);

@@ -8939,7 +8939,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 		 * avoids a race where a completion comes in before we did
 		 * prepare_to_wait().
 		 */
-		if (inflight == tctx_inflight(tctx, false))
+		if (inflight == percpu_ref_sum(&ctx->refs))
 			schedule();
 		finish_wait(&tctx->wait, &wait);
 	} while (1);

include/linux/percpu-refcount.h +1 -0

index 16c35a728b4c..2f29f34bc993 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+long percpu_ref_sum(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref

lib/percpu-refcount.c +28 -0

index a1071cdefb5a..b09ed9fdd32d 100644
@@ -475,3 +475,31 @@ void percpu_ref_resurrect(struct percpu_ref *ref)
 	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
 }
 EXPORT_SYMBOL_GPL(percpu_ref_resurrect);
+
+/**
+ * percpu_ref_sum - return approximate ref counts
+ * @ref: perpcu_ref to sum
+ *
+ * Note that this should only really be used to compare refs, as by the
+ * very nature of percpu references, the value may be stale even before it
+ * has been returned.
+ */
+long percpu_ref_sum(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	long ret;
+
+	rcu_read_lock();
+	if (__ref_is_percpu(ref, &percpu_count)) {
+		ret = atomic_long_read(&ref->data->count);
+	} else {
+		int cpu;
+
+		ret = 0;
+		for_each_possible_cpu(cpu)
+			ret += *per_cpu_ptr(percpu_count, cpu);
+	}
+	rcu_read_unlock();
+
+	return ret;
+}

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2021-04-16 13:16 UTC #12 lore raw

Quoted 11 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov
Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead
I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.

But we care about it being monotonous. There are nuances with it. I think, non sync'ed summing may put it to eternal sleep.

Are you looking to save on switching? It's almost always is already dying with prior ref_kill

Quoted 75 lines

Totally untested...

fs/io_uring.c +2 -2

index 6c182a3a221b..9edbcf01ea49 100644
@@ -8928,7 +8928,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 	atomic_inc(&tctx->in_idle);
 	do {
 		/* read completions before cancelations */
-		inflight = tctx_inflight(tctx, false);
+		inflight = percpu_ref_sum(&ctx->refs);
 		if (!inflight)
 			break;
 		io_uring_try_cancel_requests(ctx, current, NULL);

@@ -8939,7 +8939,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
 		 * avoids a race where a completion comes in before we did
 		 * prepare_to_wait().
 		 */
-		if (inflight == tctx_inflight(tctx, false))
+		if (inflight == percpu_ref_sum(&ctx->refs))
 			schedule();
 		finish_wait(&tctx->wait, &wait);
 	} while (1);

include/linux/percpu-refcount.h +1 -0

index 16c35a728b4c..2f29f34bc993 100644
@@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 void percpu_ref_resurrect(struct percpu_ref *ref);
 void percpu_ref_reinit(struct percpu_ref *ref);
 bool percpu_ref_is_zero(struct percpu_ref *ref);
+long percpu_ref_sum(struct percpu_ref *ref);
 
 /**
  * percpu_ref_kill - drop the initial ref

lib/percpu-refcount.c +28 -0

index a1071cdefb5a..b09ed9fdd32d 100644
@@ -475,3 +475,31 @@ void percpu_ref_resurrect(struct percpu_ref *ref)
 	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
 }
 EXPORT_SYMBOL_GPL(percpu_ref_resurrect);
+
+/**
+ * percpu_ref_sum - return approximate ref counts
+ * @ref: perpcu_ref to sum
+ *
+ * Note that this should only really be used to compare refs, as by the
+ * very nature of percpu references, the value may be stale even before it
+ * has been returned.
+ */
+long percpu_ref_sum(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	long ret;
+
+	rcu_read_lock();
+	if (__ref_is_percpu(ref, &percpu_count)) {
+		ret = atomic_long_read(&ref->data->count);
+	} else {
+		int cpu;
+
+		ret = 0;
+		for_each_possible_cpu(cpu)
+			ret += *per_cpu_ptr(percpu_count, cpu);
+	}
+	rcu_read_unlock();
+
+	return ret;
+}

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2021-04-16 13:58 UTC #13 lore raw

Quoted 9 lines · Pavel Begunkov

Quoted 8 lines · Jens Axboe
Quoted 6 lines · Pavel Begunkov
Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead

Quoted 5 lines

Quoted 3 lines
I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.
But we care about it being monotonous. There are nuances with it.

Do we, though? We care about it changing when something has happened, but not about it being monotonic.

Quoted 1 line

I think, non sync'ed summing may put it to eternal sleep.

That's what the two reads are about, that's the same as before. The numbers are racy in both cases, but that's why we compare after having added ourselves to the wait queue.

Quoted 2 lines

Are you looking to save on switching? It's almost always is already dying with prior ref_kill

Yep, always looking to avoid a sync switch if at all possible. For 99% of the cases it's fine, it's the last case in busy prod that wreaks havoc.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2021-04-16 14:13 UTC #14 lore raw

Quoted 10 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov
Quoted 8 lines · Jens Axboe
Quoted 6 lines · Pavel Begunkov
Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.
But we care about it being monotonous. There are nuances with it.
Do we, though? We care about it changing when something has happened, but not about it being monotonic.

We may find inflight == get_inflight(), when it's not really so, and so get to schedule() awhile there are pending requests that are not going to be cancelled by itself. And those pending requests may have been non-discoverable and so non-cancellable, e.g. because were a part of a ling/hardlink.

Quoted 6 lines

Quoted 1 line
I think, non sync'ed summing may put it to eternal sleep.
That's what the two reads are about, that's the same as before. The numbers are racy in both cases, but that's why we compare after having added ourselves to the wait queue.

Quoted 6 lines

Quoted 2 lines
Are you looking to save on switching? It's almost always is already dying with prior ref_kill
Yep, always looking to avoid a sync switch if at all possible. For 99% of the cases it's fine, it's the last case in busy prod that wreaks havoc.

Limited to sqpoll, so I wouldn't worry. Also considering that sqpoll doesn't have many file notes (as it was called before). We can completely avoid it and even make faster if happens from sq_thread() on it getting to exit, but do we want it for 5.12?

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2021-04-16 14:46 UTC #15 lore raw

Quoted 17 lines · Pavel Begunkov

Quoted 16 lines · Jens Axboe
Quoted 14 lines · Pavel Begunkov
Quoted 11 lines · Jens Axboe
Quoted 6 lines · Pavel Begunkov
Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead
I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.
But we care about it being monotonous. There are nuances with it.

Quoted 8 lines

Quoted 2 lines
Do we, though? We care about it changing when something has happened, but not about it being monotonic.
We may find inflight == get_inflight(), when it's not really so, and so get to schedule() awhile there are pending requests that are not going to be cancelled by itself. And those pending requests may have been non-discoverable and so non-cancellable, e.g. because were a part of a ling/hardlink.

Anyway, there might be other problems because of how wake_up()'s and ctx->refs putting is ordered. Needs to be remade, probably without ctx->refs in the first place.

Quoted 18 lines

Quoted 6 lines
Quoted 1 line
I think, non sync'ed summing may put it to eternal sleep.
That's what the two reads are about, that's the same as before. The numbers are racy in both cases, but that's why we compare after having added ourselves to the wait queue.
Quoted 6 lines
Quoted 2 lines
Are you looking to save on switching? It's almost always is already dying with prior ref_kill
Yep, always looking to avoid a sync switch if at all possible. For 99% of the cases it's fine, it's the last case in busy prod that wreaks havoc.
Limited to sqpoll, so I wouldn't worry. Also considering that sqpoll doesn't have many file notes (as it was called before). We can completely avoid it and even make faster if happens from sq_thread() on it getting to exit, but do we want it for 5.12?

Signature · 1 line

Pavel Begunkov

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Begunkov

author

2021-04-18 13:56 UTC #17 lore raw

Quoted 28 lines · Hillf Danton

Quoted 27 lines · On Fri, 16 Apr 2021 15:42:07 Pavel Begunkov wrote:
Quoted 25 lines · Pavel Begunkov
Quoted 10 lines · Jens Axboe
Quoted 9 lines · Pavel Begunkov
Quoted 8 lines · Jens Axboe
Quoted 6 lines · Pavel Begunkov
Quoted 1 line · Pavel Begunkov
Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
1/2 is basically a rip off of one of old Jens' patches, but can't find it anywhere. If you still have it, especially if it was reviewed/etc., may make sense to go with it instead
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
I wonder if we can do something like the below instead - we don't care about a particularly stable count in terms of wakeup reliance, and it'd save a nasty sync atomic switch.
But we care about it being monotonous. There are nuances with it.
Do we, though? We care about it changing when something has happened, but not about it being monotonic.
We may find inflight == get_inflight(), when it's not really so, and so get to schedule() awhile there are pending requests that are not going to be cancelled by itself. And those pending requests may have been non-discoverable and so non-cancellable, e.g. because were a part of a ling/hardlink.

Quoted 5 lines

Quoted 4 lines
Anyway, there might be other problems because of how wake_up()'s and ctx->refs putting is ordered. Needs to be remade, probably without ctx->refs in the first place.
Given the test rounds in the current tree, next tree and his tree the

Whose "his" tree?

Quoted 2 lines

percpu count had survived, one of the quick questions is how it fell apart last night?

What "percpu count had survived"? Do you mean the percpu-related patch from the series? What fell apart?

Signature · 1 line

Pavel Begunkov