public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: "Eric W. Biederman" <[email protected]>
Cc: Olivier Langlois <[email protected]>,
	Pavel Begunkov <[email protected]>,
	[email protected], [email protected],
	[email protected],
	Alexander Viro <[email protected]>,
	Oleg Nesterov <[email protected]>,
	Linus Torvalds <[email protected]>
Subject: Re: [PATCH 2/2] coredump: Allow coredumps to pipes to work with io_uring
Date: Wed, 24 Aug 2022 09:51:24 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 8/24/22 9:11 AM, Eric W. Biederman wrote:
> Jens Axboe <[email protected]> writes:
> 
>> On 8/23/22 12:22 PM, Eric W. Biederman wrote:
>>> Olivier Langlois <[email protected]> writes:
>>>
>>>> On Mon, 2022-08-22 at 17:16 -0400, Olivier Langlois wrote:
>>>>>
>>>>> What is stopping the task calling do_coredump() to be interrupted and
>>>>> call task_work_add() from the interrupt context?
>>>>>
>>>>> This is precisely what I was experiencing last summer when I did work
>>>>> on this issue.
>>>>>
>>>>> My understanding of how async I/O works with io_uring is that the
>>>>> task
>>>>> is added to a wait queue without being put to sleep and when the
>>>>> io_uring callback is called from the interrupt context,
>>>>> task_work_add()
>>>>> is called so that the next time io_uring syscall is invoked, pending
>>>>> work is processed to complete the I/O.
>>>>>
>>>>> So if:
>>>>>
>>>>> 1. io_uring request is initiated AND the task is in a wait queue
>>>>> 2. do_coredump() is called before the I/O is completed
>>>>>
>>>>> IMHO, this is how you end up having task_work_add() called while the
>>>>> coredump is generated.
>>>>>
>>>> I forgot to add that I have experienced the issue with TCP/IP I/O.
>>>>
>>>> I suspect that with a TCP socket, the race condition window is much
>>>> larger than if it was disk I/O and this might make it easier to
>>>> reproduce the issue this way...
>>>
>>> I was under the apparently mistaken impression that the io_uring
>>> task_work_add only comes from the io_uring userspace helper threads.
>>> Those are definitely suppressed by my change.
>>>
>>> Do you have any idea in the code where io_uring code is being called in
>>> an interrupt context?  I would really like to trace that code path so I
>>> have a better grasp on what is happening.
>>>
>>> If task_work_add is being called from interrupt context then something
>>> additional from what I have proposed certainly needs to be done.
>>
>> task_work may come from the helper threads, but generally it does not.
>> One example would be doing a read from a socket. There's no data there,
>> poll is armed to trigger a retry. When we get the poll notification that
>> there's now data to be read, then we kick that off with task_work. Since
>> it's from the poll handler, it can trigger from interrupt context. See
>> the path from io_uring/poll.c:io_poll_wake() -> __io_poll_execute() ->
>> io_req_task_work_add() -> task_work_add().
> 
> But that is a task_work to the helper thread correct?

No, it goes to the task that originally allocated/issued the request.
Which would be the original task, unless the request was originally
marked as going straight to a helper (IOSQE_ASYNC). For most cases,
it'll be the original task, not a helper thread.

>> It can also happen for regular IRQ based reads from regular files, where
>> the completion is actually done via task_work added from the potentially
>> IRQ based completion path.
> 
> I can see that.
> 
> Which leaves me with the question do these task_work's directly wake up
> the thread that submitted the I/O request?   Or is there likely to be
> something for an I/O thread to do before an ordinary program thread is
> notified.
> 
> I am asking because it is only the case of notifying ordinary program
> threads that is interesting in the case of a coredump.  As I understand
> it a data to read notification would typically be handled by the I/O
> uring worker thread to trigger reading the data before letting userspace
> know everything it asked to be done is complete.

By default, it'll go back to the original task. If something is
pollable, then there's no helper thread doing the request. An issue
attempt is performed by the original task, there's no data/space there,
poll is armed to trigger a retry. Retry notification will then queue
task_work with the original task to retry.

Generally for io_uring, helper threads are a slow path and aren't used
unless we have no other options. For example, if someone has a network
IO backend and there's helper thread activity, then that's generally a
sign that something is wrong. This isn't super relevant to this
particular topic, just highlighting that normally you would not expect
to see much (if any) io-wq activity at all.

-- 
Jens Axboe

  reply	other threads:[~2022-08-24 15:51 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <[email protected]>
     [not found] ` <[email protected]>
     [not found]   ` <87h7i694ij.fsf_-_@disp2133>
2021-06-09 20:33     ` [RFC] coredump: Do not interrupt dump for TIF_NOTIFY_SIGNAL Linus Torvalds
2021-06-09 20:48       ` Eric W. Biederman
2021-06-09 20:52         ` Linus Torvalds
2021-06-09 21:02       ` Olivier Langlois
2021-06-09 21:05         ` Eric W. Biederman
2021-06-09 21:26           ` Olivier Langlois
2021-06-09 21:56             ` Olivier Langlois
2021-06-10 14:26             ` Eric W. Biederman
2021-06-10 15:17               ` Olivier Langlois
2021-06-10 18:58               ` [CFT}[PATCH] coredump: Limit what can interrupt coredumps Eric W. Biederman
2021-06-10 19:10                 ` Linus Torvalds
2021-06-10 19:18                   ` Eric W. Biederman
2021-06-10 19:50                     ` Linus Torvalds
2021-06-10 20:11                       ` [PATCH] " Eric W. Biederman
2021-06-10 21:04                         ` Linus Torvalds
2021-06-12 14:36                         ` Olivier Langlois
2021-06-12 16:26                           ` Jens Axboe
2021-06-14 14:10                         ` Oleg Nesterov
2021-06-14 16:37                           ` Eric W. Biederman
2021-06-14 16:59                             ` Oleg Nesterov
2021-06-15 22:08                           ` Eric W. Biederman
2021-06-16 19:23                             ` Olivier Langlois
2021-06-16 20:00                               ` Eric W. Biederman
2021-06-18 20:05                                 ` Olivier Langlois
2021-08-05 13:06                             ` Olivier Langlois
2021-08-10 21:48                               ` Tony Battersby
2021-08-11 20:47                                 ` Olivier Langlois
2021-08-12  1:55                                 ` Jens Axboe
2021-08-12 13:53                                   ` Tony Battersby
2021-08-15 20:42                                   ` Olivier Langlois
2021-08-16 13:02                                     ` Pavel Begunkov
2021-08-16 13:06                                       ` Pavel Begunkov
2021-08-17 18:15                                     ` Jens Axboe
2021-08-17 18:24                                       ` Jens Axboe
2021-08-17 19:29                                         ` Tony Battersby
2021-08-17 19:59                                           ` Jens Axboe
2021-08-17 21:28                                             ` Jens Axboe
2021-08-17 21:39                                               ` Tony Battersby
2021-08-17 22:05                                                 ` Jens Axboe
2021-08-18 14:37                                                   ` Tony Battersby
2021-08-18 14:46                                                     ` Jens Axboe
2021-08-18  2:57                                               ` Jens Axboe
2021-08-18  2:58                                                 ` Jens Axboe
2021-08-21 10:08                                                 ` Olivier Langlois
2021-08-21 16:47                                                   ` Olivier Langlois
2021-08-21 16:51                                                     ` Jens Axboe
2021-08-21 17:21                                                       ` Olivier Langlois
2021-08-21  9:52                                         ` Olivier Langlois
2021-08-21  9:48                                       ` Olivier Langlois
2021-10-22 14:13     ` [RFC] coredump: Do not interrupt dump for TIF_NOTIFY_SIGNAL Pavel Begunkov
2021-12-24  1:34       ` Olivier Langlois
2021-12-24 10:37         ` Pavel Begunkov
2021-12-24 19:52           ` Eric W. Biederman
2021-12-28 11:24             ` Pavel Begunkov
2022-03-14 23:58               ` Eric W. Biederman
     [not found]                 ` <[email protected]>
2022-06-01  3:15                   ` Jens Axboe
2022-07-20 16:49                     ` [PATCH 0/2] coredump: Allow io_uring using apps to dump to pipes Eric W. Biederman
2022-07-20 16:50                       ` [PATCH 1/2] signal: Move stopping for the coredump from do_exit into get_signal Eric W. Biederman
2022-07-20 16:51                       ` [PATCH 2/2] coredump: Allow coredumps to pipes to work with io_uring Eric W. Biederman
2022-08-22 21:16                         ` Olivier Langlois
2022-08-23  3:35                           ` Olivier Langlois
2022-08-23 18:22                             ` Eric W. Biederman
2022-08-23 18:27                               ` Jens Axboe
2022-08-24 15:11                                 ` Eric W. Biederman
2022-08-24 15:51                                   ` Jens Axboe [this message]
2022-01-05 19:39           ` [RFC] coredump: Do not interrupt dump for TIF_NOTIFY_SIGNAL Olivier Langlois

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox