On Thu, Feb 20, 2020 at 11:23 PM Jens Axboe <axboe@xxxxxxxxx> wrote: > On 2/20/20 3:14 PM, Jens Axboe wrote: > >>> @@ -3646,46 +3596,11 @@ static int io_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, > >>> > >>> list_del_init(&poll->wait.entry); > >>> > >> [...] > >>> + tsk = req->task; > >>> + req->result = mask; > >>> + init_task_work(&req->sched_work, io_poll_task_func); > >>> + sched_work_add(tsk, &req->sched_work); > >> > >> Doesn't this have to check the return value? > > > > Trying to think if we can get here with TASK_EXITING, but probably safer > > to just handle it in any case. I'll add that. > > Double checked this one, and I think it's good as-is, but needs a > comment. If the sched_work_add() fails, then the work item is still in > the poll hash on the ctx. That work is canceled on exit. You mean via io_poll_remove_all()? That doesn't happen when a thread dies, right? As far as I can tell, the following might happen: 1. process with threads A and B set up uring 2. thread B submits chained requests poll->read 3. thread A waits for request completion 4. thread B dies 5. poll waitqueue is notified, data is ready Even if there isn't a memory leak, you'd still want the read request to execute at some point so that thread A can see the result, right? And actually, in this scenario, wouldn't the req->task be a dangling pointer, since you're not holding a reference? Or is there some magic callback from do_exit() to io_uring that I missed? There is a comment "/* task will wait for requests on exit, don't need a ref */", but I don't see how that works...
