[PATCHSET 0/6] Cancelation cleanups

[PATCHSET 0/6] Cancelation cleanups

[Jens Axboe]

Hi,

While adding another cancelable request type, I noticed that there's a
bit of boilerplate code for supporting that. This patch series gets rid
of the generic hlist_head based cancel implementations and adds a
generic one instead that both futex and waitid can use. The pending
epoll wait implementation will be able to use it too.

 io_uring/cancel.c | 41 +++++++++++++++++++++++++++++++++++++++++
 io_uring/cancel.h |  8 ++++++++
 io_uring/futex.c  | 42 +++---------------------------------------
 io_uring/waitid.c | 42 +++---------------------------------------
 4 files changed, 55 insertions(+), 78 deletions(-)

-- 
Jens Axboe

[PATCH 1/6] io_uring/cancel: add generic remove_all helper

[Jens Axboe]

Any opcode that is cancelable ends up defining its own remove all
helper, which iterates the pending list and cancels matches. Add a
generic helper for it, which can be used by them.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/cancel.c | 20 ++++++++++++++++++++
 io_uring/cancel.h |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/io_uring/cancel.c b/io_uring/cancel.c
index 484193567839..0565dc0d7611 100644
--- a/io_uring/cancel.c
+++ b/io_uring/cancel.c
@@ -341,3 +341,23 @@ int io_sync_cancel(struct io_ring_ctx *ctx, void __user *arg)
 		fput(file);
 	return ret;
 }
+
+bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
+			  struct hlist_head *list, bool cancel_all,
+			  bool (*cancel)(struct io_kiocb *))
+{
+	struct hlist_node *tmp;
+	struct io_kiocb *req;
+	bool found = false;
+
+	lockdep_assert_held(&ctx->uring_lock);
+
+	hlist_for_each_entry_safe(req, tmp, list, hash_node) {
+		if (!io_match_task_safe(req, tctx, cancel_all))
+			continue;
+		if (cancel(req))
+			found = true;
+	}
+
+	return found;
+}
diff --git a/io_uring/cancel.h b/io_uring/cancel.h
index bbfea2cd00ea..80734a0a2b26 100644
--- a/io_uring/cancel.h
+++ b/io_uring/cancel.h
@@ -24,6 +24,10 @@ int io_try_cancel(struct io_uring_task *tctx, struct io_cancel_data *cd,
 int io_sync_cancel(struct io_ring_ctx *ctx, void __user *arg);
 bool io_cancel_req_match(struct io_kiocb *req, struct io_cancel_data *cd);
 
+bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
+			  struct hlist_head *list, bool cancel_all,
+			  bool (*cancel)(struct io_kiocb *));
+
 static inline bool io_cancel_match_sequence(struct io_kiocb *req, int sequence)
 {
 	if (req->cancel_seq_set && sequence == req->work.cancel_seq)
-- 
2.47.2

RE: [PATCH 1/6] io_uring/cancel: add generic remove_all helper

[lizetao]

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Thursday, February 6, 2025 4:26 AM
> To: io-uring@vger.kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>
> Subject: [PATCH 1/6] io_uring/cancel: add generic remove_all helper
> 
> Any opcode that is cancelable ends up defining its own remove all helper, which
> iterates the pending list and cancels matches. Add a generic helper for it, which
> can be used by them.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  io_uring/cancel.c | 20 ++++++++++++++++++++  io_uring/cancel.h |  4 ++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/io_uring/cancel.c b/io_uring/cancel.c index
> 484193567839..0565dc0d7611 100644
> --- a/io_uring/cancel.c
> +++ b/io_uring/cancel.c
> @@ -341,3 +341,23 @@ int io_sync_cancel(struct io_ring_ctx *ctx, void __user
> *arg)
>  		fput(file);
>  	return ret;
>  }
> +
> +bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
> +			  struct hlist_head *list, bool cancel_all,
> +			  bool (*cancel)(struct io_kiocb *)) {
> +	struct hlist_node *tmp;
> +	struct io_kiocb *req;
> +	bool found = false;
> +
> +	lockdep_assert_held(&ctx->uring_lock);
> +
> +	hlist_for_each_entry_safe(req, tmp, list, hash_node) {
> +		if (!io_match_task_safe(req, tctx, cancel_all))
> +			continue;

Should call hlist_del_init(&req->hash_node) here, just like the original code logic.
> +		if (cancel(req))
> +			found = true;
> +	}
> +
> +	return found;
> +}
> diff --git a/io_uring/cancel.h b/io_uring/cancel.h index
> bbfea2cd00ea..80734a0a2b26 100644
> --- a/io_uring/cancel.h
> +++ b/io_uring/cancel.h
> @@ -24,6 +24,10 @@ int io_try_cancel(struct io_uring_task *tctx, struct
> io_cancel_data *cd,  int io_sync_cancel(struct io_ring_ctx *ctx, void __user
> *arg);  bool io_cancel_req_match(struct io_kiocb *req, struct io_cancel_data
> *cd);
> 
> +bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
> +			  struct hlist_head *list, bool cancel_all,
> +			  bool (*cancel)(struct io_kiocb *));
> +
>  static inline bool io_cancel_match_sequence(struct io_kiocb *req, int sequence)
> {
>  	if (req->cancel_seq_set && sequence == req->work.cancel_seq)
> --
> 2.47.2
> 

---
Li Zetao

Re: [PATCH 1/6] io_uring/cancel: add generic remove_all helper

[Jens Axboe]

On 2/6/25 5:46 AM, lizetao wrote:
> 
> 
>> -----Original Message-----
>> From: Jens Axboe <axboe@kernel.dk>
>> Sent: Thursday, February 6, 2025 4:26 AM
>> To: io-uring@vger.kernel.org
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Subject: [PATCH 1/6] io_uring/cancel: add generic remove_all helper
>>
>> Any opcode that is cancelable ends up defining its own remove all helper, which
>> iterates the pending list and cancels matches. Add a generic helper for it, which
>> can be used by them.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  io_uring/cancel.c | 20 ++++++++++++++++++++  io_uring/cancel.h |  4 ++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/io_uring/cancel.c b/io_uring/cancel.c index
>> 484193567839..0565dc0d7611 100644
>> --- a/io_uring/cancel.c
>> +++ b/io_uring/cancel.c
>> @@ -341,3 +341,23 @@ int io_sync_cancel(struct io_ring_ctx *ctx, void __user
>> *arg)
>>  		fput(file);
>>  	return ret;
>>  }
>> +
>> +bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
>> +			  struct hlist_head *list, bool cancel_all,
>> +			  bool (*cancel)(struct io_kiocb *)) {
>> +	struct hlist_node *tmp;
>> +	struct io_kiocb *req;
>> +	bool found = false;
>> +
>> +	lockdep_assert_held(&ctx->uring_lock);
>> +
>> +	hlist_for_each_entry_safe(req, tmp, list, hash_node) {
>> +		if (!io_match_task_safe(req, tctx, cancel_all))
>> +			continue;
> 
> Should call hlist_del_init(&req->hash_node) here, just like the
> original code logic.

Indeed, good catch! I'll make the change.

-- 
Jens Axboe

[PATCH 2/6] io_uring/futex: convert to io_cancel_remove_all()

[Jens Axboe]

Use the generic helper for cancelations.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/futex.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/io_uring/futex.c b/io_uring/futex.c
index 3159a2b7eeca..808eb57f1210 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -90,7 +90,7 @@ static bool io_futexv_claim(struct io_futex *iof)
 	return true;
 }
 
-static bool __io_futex_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req)
+static bool __io_futex_cancel(struct io_kiocb *req)
 {
 	/* futex wake already done or in progress */
 	if (req->opcode == IORING_OP_FUTEX_WAIT) {
@@ -128,7 +128,7 @@ int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 		if (req->cqe.user_data != cd->data &&
 		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
 			continue;
-		if (__io_futex_cancel(ctx, req))
+		if (__io_futex_cancel(req))
 			nr++;
 		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
 			break;
@@ -144,21 +144,7 @@ int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 bool io_futex_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
 			 bool cancel_all)
 {
-	struct hlist_node *tmp;
-	struct io_kiocb *req;
-	bool found = false;
-
-	lockdep_assert_held(&ctx->uring_lock);
-
-	hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) {
-		if (!io_match_task_safe(req, tctx, cancel_all))
-			continue;
-		hlist_del_init(&req->hash_node);
-		__io_futex_cancel(ctx, req);
-		found = true;
-	}
-
-	return found;
+	return io_cancel_remove_all(ctx, tctx, &ctx->futex_list, cancel_all, __io_futex_cancel);
 }
 
 int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
-- 
2.47.2

[PATCH 3/6] io_uring/waitid: convert to io_cancel_remove_all()

[Jens Axboe]

Use the generic helper for cancelations.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/waitid.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/io_uring/waitid.c b/io_uring/waitid.c
index 853e97a7b0ec..ed7c76426358 100644
--- a/io_uring/waitid.c
+++ b/io_uring/waitid.c
@@ -134,7 +134,7 @@ static void io_waitid_complete(struct io_kiocb *req, int ret)
 	io_req_task_complete(req, &ts);
 }
 
-static bool __io_waitid_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req)
+static bool __io_waitid_cancel(struct io_kiocb *req)
 {
 	struct io_waitid *iw = io_kiocb_to_cmd(req, struct io_waitid);
 	struct io_waitid_async *iwa = req->async_data;
@@ -171,7 +171,7 @@ int io_waitid_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 		if (req->cqe.user_data != cd->data &&
 		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
 			continue;
-		if (__io_waitid_cancel(ctx, req))
+		if (__io_waitid_cancel(req))
 			nr++;
 		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
 			break;
@@ -187,21 +187,7 @@ int io_waitid_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 bool io_waitid_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
 			  bool cancel_all)
 {
-	struct hlist_node *tmp;
-	struct io_kiocb *req;
-	bool found = false;
-
-	lockdep_assert_held(&ctx->uring_lock);
-
-	hlist_for_each_entry_safe(req, tmp, &ctx->waitid_list, hash_node) {
-		if (!io_match_task_safe(req, tctx, cancel_all))
-			continue;
-		hlist_del_init(&req->hash_node);
-		__io_waitid_cancel(ctx, req);
-		found = true;
-	}
-
-	return found;
+	return io_cancel_remove_all(ctx, tctx, &ctx->waitid_list, cancel_all, __io_waitid_cancel);
 }
 
 static inline bool io_waitid_drop_issue_ref(struct io_kiocb *req)
-- 
2.47.2

[PATCH 4/6] io_uring/cancel: add generic cancel helper

[Jens Axboe]

Any opcode that is cancelable ends up defining its own cancel helper
for finding and canceling a specific request. Add a generic helper that
can be used for this purpose.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/cancel.c | 21 +++++++++++++++++++++
 io_uring/cancel.h |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/io_uring/cancel.c b/io_uring/cancel.c
index 0565dc0d7611..4ff3771131c2 100644
--- a/io_uring/cancel.c
+++ b/io_uring/cancel.c
@@ -361,3 +361,24 @@ bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
 
 	return found;
 }
+
+int io_cancel_remove(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
+		     unsigned int issue_flags, struct hlist_head *list,
+		     bool (*cancel)(struct io_kiocb *))
+{
+	struct hlist_node *tmp;
+	struct io_kiocb *req;
+	int nr = 0;
+
+	io_ring_submit_lock(ctx, issue_flags);
+	hlist_for_each_entry_safe(req, tmp, list, hash_node) {
+		if (!io_cancel_req_match(req, cd))
+			continue;
+		if (cancel(req))
+			nr++;
+		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
+			break;
+	}
+	io_ring_submit_unlock(ctx, issue_flags);
+	return nr ?: -ENOENT;
+}
diff --git a/io_uring/cancel.h b/io_uring/cancel.h
index 80734a0a2b26..43e9bb74e9d1 100644
--- a/io_uring/cancel.h
+++ b/io_uring/cancel.h
@@ -28,6 +28,10 @@ bool io_cancel_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
 			  struct hlist_head *list, bool cancel_all,
 			  bool (*cancel)(struct io_kiocb *));
 
+int io_cancel_remove(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
+		     unsigned int issue_flags, struct hlist_head *list,
+		     bool (*cancel)(struct io_kiocb *));
+
 static inline bool io_cancel_match_sequence(struct io_kiocb *req, int sequence)
 {
 	if (req->cancel_seq_set && sequence == req->work.cancel_seq)
-- 
2.47.2

[PATCH 5/6] io_uring/futex: use generic io_cancel_remove() helper

[Jens Axboe]

Don't implement our own loop rolling and checking, just use the generic
helper to find and cancel requests.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/futex.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/io_uring/futex.c b/io_uring/futex.c
index 808eb57f1210..54b9760f2aa6 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -116,29 +116,7 @@ static bool __io_futex_cancel(struct io_kiocb *req)
 int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 		    unsigned int issue_flags)
 {
-	struct hlist_node *tmp;
-	struct io_kiocb *req;
-	int nr = 0;
-
-	if (cd->flags & (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED))
-		return -ENOENT;
-
-	io_ring_submit_lock(ctx, issue_flags);
-	hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) {
-		if (req->cqe.user_data != cd->data &&
-		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
-			continue;
-		if (__io_futex_cancel(req))
-			nr++;
-		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
-			break;
-	}
-	io_ring_submit_unlock(ctx, issue_flags);
-
-	if (nr)
-		return nr;
-
-	return -ENOENT;
+	return io_cancel_remove(ctx, cd, issue_flags, &ctx->futex_list, __io_futex_cancel);
 }
 
 bool io_futex_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
-- 
2.47.2

RE: [PATCH 5/6] io_uring/futex: use generic io_cancel_remove() helper

[lizetao]

Hi,

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Thursday, February 6, 2025 4:26 AM
> To: io-uring@vger.kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>
> Subject: [PATCH 5/6] io_uring/futex: use generic io_cancel_remove() helper
> 
> Don't implement our own loop rolling and checking, just use the generic helper to
> find and cancel requests.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  io_uring/futex.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/io_uring/futex.c b/io_uring/futex.c index
> 808eb57f1210..54b9760f2aa6 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c
> @@ -116,29 +116,7 @@ static bool __io_futex_cancel(struct io_kiocb *req)  int
> io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
>  		    unsigned int issue_flags)
>  {
> -	struct hlist_node *tmp;
> -	struct io_kiocb *req;
> -	int nr = 0;
> -
> -	if (cd->flags &
> (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED))
> -		return -ENOENT;

Why remove this check?
> -
> -	io_ring_submit_lock(ctx, issue_flags);
> -	hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) {
> -		if (req->cqe.user_data != cd->data &&
> -		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
> -			continue;
> -		if (__io_futex_cancel(req))
> -			nr++;
> -		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
> -			break;
> -	}
> -	io_ring_submit_unlock(ctx, issue_flags);
> -
> -	if (nr)
> -		return nr;
> -
> -	return -ENOENT;
> +	return io_cancel_remove(ctx, cd, issue_flags, &ctx->futex_list,
> +__io_futex_cancel);
>  }
> 
>  bool io_futex_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
> --
> 2.47.2
> 
> 

---
Li Zetao

Re: [PATCH 5/6] io_uring/futex: use generic io_cancel_remove() helper

[Jens Axboe]

On 2/6/25 5:56 AM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Jens Axboe <axboe@kernel.dk>
>> Sent: Thursday, February 6, 2025 4:26 AM
>> To: io-uring@vger.kernel.org
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Subject: [PATCH 5/6] io_uring/futex: use generic io_cancel_remove() helper
>>
>> Don't implement our own loop rolling and checking, just use the generic helper to
>> find and cancel requests.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  io_uring/futex.c | 24 +-----------------------
>>  1 file changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/io_uring/futex.c b/io_uring/futex.c index
>> 808eb57f1210..54b9760f2aa6 100644
>> --- a/io_uring/futex.c
>> +++ b/io_uring/futex.c
>> @@ -116,29 +116,7 @@ static bool __io_futex_cancel(struct io_kiocb *req)  int
>> io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
>>  		    unsigned int issue_flags)
>>  {
>> -	struct hlist_node *tmp;
>> -	struct io_kiocb *req;
>> -	int nr = 0;
>> -
>> -	if (cd->flags &
>> (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED))
>> -		return -ENOENT;
> 
> Why remove this check?

It isn't really necessary. Yes we could loop pointlessly if they are set
and not find anything, but it's really just a bad use case from the
application. End result should be the same, that -ENOENT is returned on
trying to lookup a futex operation based on the fd.

-- 
Jens Axboe

[PATCH 6/6] io_uring/waitid: use generic io_cancel_remove() helper

[Jens Axboe]

Don't implement our own loop rolling and checking, just use the generic
helper to find and cancel requests.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/waitid.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/io_uring/waitid.c b/io_uring/waitid.c
index ed7c76426358..4fb465b48560 100644
--- a/io_uring/waitid.c
+++ b/io_uring/waitid.c
@@ -159,29 +159,7 @@ static bool __io_waitid_cancel(struct io_kiocb *req)
 int io_waitid_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,
 		     unsigned int issue_flags)
 {
-	struct hlist_node *tmp;
-	struct io_kiocb *req;
-	int nr = 0;
-
-	if (cd->flags & (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED))
-		return -ENOENT;
-
-	io_ring_submit_lock(ctx, issue_flags);
-	hlist_for_each_entry_safe(req, tmp, &ctx->waitid_list, hash_node) {
-		if (req->cqe.user_data != cd->data &&
-		    !(cd->flags & IORING_ASYNC_CANCEL_ANY))
-			continue;
-		if (__io_waitid_cancel(req))
-			nr++;
-		if (!(cd->flags & IORING_ASYNC_CANCEL_ALL))
-			break;
-	}
-	io_ring_submit_unlock(ctx, issue_flags);
-
-	if (nr)
-		return nr;
-
-	return -ENOENT;
+	return io_cancel_remove(ctx, cd, issue_flags, &ctx->waitid_list, __io_waitid_cancel);
 }
 
 bool io_waitid_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
-- 
2.47.2