On 2/20/20 3:38 PM, Jann Horn wrote:
> 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?
Off of io_uring_flush, we do:
if (fatal_signal_pending(current) || (current->flags & PF_EXITING)) {
io_uring_cancel_task_poll(current);
io_uring_cancel_task_async(current);
io_wq_cancel_pid(ctx->io_wq, task_pid_vnr(current));
}
to cancel _anything_ that the task has pending.
> 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
Unless I'm mistaken, when B dies, the requests from #2 will be canceled.
> 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?
It just needs to complete, if the task is going away, then a cancelation
is fine too.
> 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...
That'd only be the case if we didn't cancel requests when it dies. I'll
double check if that's 100% the case.
--
Jens Axboe
