[PATCH 1/1] io_uring: add lockdep checks for io_handle_tw_list

[PATCH 1/1] io_uring: add lockdep checks for io_handle_tw_list

[Pavel Begunkov]

Add a lockdep check to io_handle_tw_list() verifying that the context is
locked and no task work drops it by accident.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 6df996d01ccf..13e0b48d1aac 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1054,6 +1054,10 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,
 			mutex_lock(&ctx->uring_lock);
 			percpu_ref_get(&ctx->refs);
 		}
+
+		lockdep_assert(req->ctx == ctx);
+		lockdep_assert_held(&ctx->uring_lock);
+
 		INDIRECT_CALL_2(req->io_task_work.func,
 				io_poll_task_func, io_req_rw_complete,
 				req, ts);
-- 
2.48.1

Re: [PATCH 1/1] io_uring: add lockdep checks for io_handle_tw_list

[Jens Axboe]

On 4/1/25 9:46 AM, Pavel Begunkov wrote:
> Add a lockdep check to io_handle_tw_list() verifying that the context is
> locked and no task work drops it by accident.

I think we'd want a bit more of a "why" explanation here, but I can add
that while committing.

> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 6df996d01ccf..13e0b48d1aac 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -1054,6 +1054,10 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,
>  			mutex_lock(&ctx->uring_lock);
>  			percpu_ref_get(&ctx->refs);
>  		}
> +
> +		lockdep_assert(req->ctx == ctx);
> +		lockdep_assert_held(&ctx->uring_lock);
> +
>  		INDIRECT_CALL_2(req->io_task_work.func,
>  				io_poll_task_func, io_req_rw_complete,
>  				req, ts);

If the assumption is that some previous tw messed things up, might not
be a bad idea to include dumping of that if one of the above lockdep
asserts fail? Preferably in such a way that code generation is the same
when lockdep isn't set...

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: add lockdep checks for io_handle_tw_list

[Pavel Begunkov]

On 4/1/25 17:13, Jens Axboe wrote:
> On 4/1/25 9:46 AM, Pavel Begunkov wrote:
>> Add a lockdep check to io_handle_tw_list() verifying that the context is
>> locked and no task work drops it by accident.
> 
> I think we'd want a bit more of a "why" explanation here, but I can add
> that while committing.
> 
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index 6df996d01ccf..13e0b48d1aac 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -1054,6 +1054,10 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,
>>   			mutex_lock(&ctx->uring_lock);
>>   			percpu_ref_get(&ctx->refs);
>>   		}
>> +
>> +		lockdep_assert(req->ctx == ctx);
>> +		lockdep_assert_held(&ctx->uring_lock);
>> +
>>   		INDIRECT_CALL_2(req->io_task_work.func,
>>   				io_poll_task_func, io_req_rw_complete,
>>   				req, ts);
> 
> If the assumption is that some previous tw messed things up, might not
> be a bad idea to include dumping of that if one of the above lockdep
> asserts fail? Preferably in such a way that code generation is the same
> when lockdep isn't set...

We can move it after the tw run where it still has the request
(but doesn't own it).

-- 
Pavel Begunkov

Re: [PATCH 1/1] io_uring: add lockdep checks for io_handle_tw_list

[Jens Axboe]

On 4/1/25 11:19 AM, Pavel Begunkov wrote:
> On 4/1/25 17:13, Jens Axboe wrote:
>> On 4/1/25 9:46 AM, Pavel Begunkov wrote:
>>> Add a lockdep check to io_handle_tw_list() verifying that the context is
>>> locked and no task work drops it by accident.
>>
>> I think we'd want a bit more of a "why" explanation here, but I can add
>> that while committing.
>>
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index 6df996d01ccf..13e0b48d1aac 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -1054,6 +1054,10 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,
>>>               mutex_lock(&ctx->uring_lock);
>>>               percpu_ref_get(&ctx->refs);
>>>           }
>>> +
>>> +        lockdep_assert(req->ctx == ctx);
>>> +        lockdep_assert_held(&ctx->uring_lock);
>>> +
>>>           INDIRECT_CALL_2(req->io_task_work.func,
>>>                   io_poll_task_func, io_req_rw_complete,
>>>                   req, ts);
>>
>> If the assumption is that some previous tw messed things up, might not
>> be a bad idea to include dumping of that if one of the above lockdep
>> asserts fail? Preferably in such a way that code generation is the same
>> when lockdep isn't set...
> 
> We can move it after the tw run where it still has the request
> (but doesn't own it).

Yep let's do that, but it'd still be nice to dump what func is if it
ends up triggering.

-- 
Jens Axboe