| Message ID | ffd30102aee729e48911f595d1c05804e59b0403.1743522348.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] io_uring: add lockdep checks for io_handle_tw_list | expand |
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...
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).
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.
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);
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(+)