public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Jann Horn <[email protected]>, Minchan Kim <[email protected]>,
	io-uring <[email protected]>
Cc: Andrew Morton <[email protected]>,
	LKML <[email protected]>,
	linux-mm <[email protected]>,
	Linux API <[email protected]>,
	Oleksandr Natalenko <[email protected]>,
	Suren Baghdasaryan <[email protected]>,
	Tim Murray <[email protected]>,
	Daniel Colascione <[email protected]>,
	Sandeep Patil <[email protected]>,
	Sonny Rao <[email protected]>,
	Brian Geffon <[email protected]>, Michal Hocko <[email protected]>,
	Johannes Weiner <[email protected]>,
	Shakeel Butt <[email protected]>,
	John Dias <[email protected]>,
	Joel Fernandes <[email protected]>,
	[email protected],
	Alexander Duyck <[email protected]>
Subject: Re: [PATCH v5 1/7] mm: pass task and mm to do_madvise
Date: Fri, 14 Feb 2020 11:22:08 -0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAG48ez3S5+EasZ1ZWcMQYZQQ5zJOBtY-_C7oz6DMfG4Gcyig1g@mail.gmail.com>

On 2/14/20 10:25 AM, Jann Horn wrote:
> +Jens and io-uring list
> 
> On Fri, Feb 14, 2020 at 6:06 PM Minchan Kim <[email protected]> wrote:
>> In upcoming patches, do_madvise will be called from external process
>> context so we shouldn't asssume "current" is always hinted process's
>> task_struct.
> [...]
>> [1] http://lore.kernel.org/r/CAG48ez27=pwm5m_N_988xT1huO7g7h6arTQL44zev6TD-h-7Tg@mail.gmail.com
> [...]
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
> [...]
>> @@ -2736,7 +2736,7 @@ static int io_madvise(struct io_kiocb *req, struct io_kiocb **nxt,
>>         if (force_nonblock)
>>                 return -EAGAIN;
>>
>> -       ret = do_madvise(ma->addr, ma->len, ma->advice);
>> +       ret = do_madvise(current, current->mm, ma->addr, ma->len, ma->advice);
>>         if (ret < 0)
>>                 req_set_fail_links(req);
>>         io_cqring_add_event(req, ret);
> 
> Jens, can you have a look at this change and the following patch
> <https://lore.kernel.org/linux-mm/[email protected]/>
> ("[PATCH v5 3/7] mm: check fatal signal pending of target process")?
> Basically Minchan's patch tries to plumb through the identity of the
> target task so that if that task gets killed in the middle of the
> operation, the (potentially long-running and costly) madvise operation
> can be cancelled. Just passing in "current" instead (which in this
> case is the uring worker thread AFAIK) doesn't really break anything,
> other than making the optimization not work, but I wonder whether this
> couldn't be done more cleanly - maybe by passing in NULL to mean "we
> don't know who the target task is", since I think we don't know that
> here?

Thanks for bringing this to my attention, patches that touch io_uring
(or anything else) really should be CC'ed to the maintainer(s) of those
areas...

Yeah, the change above won't do the right thing for io_uring, in fact
it'll always be the wrong task. So I'd second Jann's question, and ask
if we really need the actual task, or if NULL could be used? For
cancelation purposes, I'm guessing you want the task that's actually
doing the operation, even if it's on behalf of someone else. That makes
the interface a bit weird, as you'd assume the task/mm passed in would
be related to the madvise itself, not just for cancelation.

Would be nice with some clarification, so we can figure out an approach
that would actually work.

-- 
Jens Axboe


  reply	other threads:[~2020-02-14 18:22 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <[email protected]>
     [not found] ` <[email protected]>
2020-02-14 17:25   ` [PATCH v5 1/7] mm: pass task and mm to do_madvise Jann Horn
2020-02-14 18:22     ` Jens Axboe [this message]
2020-02-14 18:45       ` Minchan Kim
2020-02-14 19:09         ` Jens Axboe
2020-02-14 19:31           ` Minchan Kim

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] \
    [email protected] \
    [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