All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jann Horn <jannh@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Linux-MM <linux-mm@kvack.org>,
	kernel list <linux-kernel@vger.kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	John Hubbard <jhubbard@nvidia.com>, Jan Kara <jack@suse.cz>,
	stable <stable@vger.kernel.org>, Michal Hocko <mhocko@suse.com>
Subject: Re: [PATCH v2] mm/gup: fix try_grab_compound_head() race with split_huge_page()
Date: Tue, 15 Jun 2021 04:36:34 +0200	[thread overview]
Message-ID: <CAG48ez1nZcrJPO-hOLyE08g8HKSGEambCp6mNv6FNR2c9+6sJg@mail.gmail.com> (raw)
In-Reply-To: <20210614190032.09d8b7ac530c8b14ace44b82@linux-foundation.org>

On Tue, Jun 15, 2021 at 4:00 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 15 Jun 2021 03:20:14 +0200 Jann Horn <jannh@google.com> wrote:
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -43,8 +43,25 @@ static void hpage_pincount_sub(struct page *page, int refs)
> >
> >       atomic_sub(refs, compound_pincount_ptr(page));
> >  }
> >
> > +/* Equivalent to calling put_page() @refs times. */
> > +static void put_page_refs(struct page *page, int refs)
> > +{
> > +#ifdef CONFIG_DEBUG_VM
> > +     if (VM_WARN_ON_ONCE_PAGE(page_ref_count(page) < refs, page))
> > +             return;
> > +#endif
>
> Well dang those ifdefs.
>
> With CONFIG_DEBUG_VM=n, this expands to
>
>         if (((void)(sizeof((__force long)(page_ref_count(page) < refs))))
>                 return;
>
> which will fail with "void value not ignored as it ought to be".
> Because VM_WARN_ON_ONCE_PAGE() is an rval with CONFIG_DEBUG_VM=y and is
> not an rval with CONFIG_DEBUG_VM=n.    So the ifdefs are needed.
>
> I know we've been around this loop before, but it still sucks!  Someone
> please remind me of the reasoning?
>
> Can we do
>
> #define VM_WARN_ON_ONCE_PAGE(cond, page) {
>         BUILD_BUG_ON_INVALID(cond);
>         cond;
> }
>
> ?

See also <https://lore.kernel.org/linux-mm/CAG48ez3Vb1BxaZ0EHhR9ctkjCCygMWOQqFMGqt-=Ea2yXrvKiw@mail.gmail.com/>.

I see basically two issues with your proposal:

1. Even if you use it without "if (...)", the compiler has to generate
code to evaluate the condition unless it can prove that the condition
has no side effects. (It can't prove that for stuff like atomic_read()
or READ_ONCE(), because those are volatile loads and C says you can't
eliminate those. There are compiler builtins that are more
fine-grained, but the kernel doesn't use those.)

2. The current version generates no code at all for !DEBUG_VM builds.
Your proposal would still have the conditional bailout even in
!DEBUG_VM builds - and if the compiler already has to evaluate the
condition and generate a conditional branch, I don't know whether
there is much of a point in omitting the code that prints a warning
under !DEBUG_VM (although I guess it could impact register spilling
and assignment).


If you don't like the ifdeffery in this patch, can you please merge
the v1 patch? It's not like I was adding a new BUG_ON(), I was just
refactoring an existing BUG_ON() into a helper function, so I wasn't
making things worse; and I don't want to think about how to best
design WARN/BUG macros for the VM subsystem in order to land this
bugfix.

(Also, this patch is intended for stable-backporting, so mixing in
more changes unrelated to the issue being fixed might make backporting
more annoying. This v2 patch will already require more fixup than the
v1 one, because VM_WARN_ON_ONCE_PAGE() was only added recently.)

  reply	other threads:[~2021-06-15  2:37 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15  1:20 [PATCH v2] mm/gup: fix try_grab_compound_head() race with split_huge_page() Jann Horn
2021-06-15  2:00 ` Andrew Morton
2021-06-15  2:36   ` Jann Horn [this message]
2021-06-15  2:36     ` Jann Horn
2021-06-15  2:38     ` Jann Horn
2021-06-15  2:38       ` Jann Horn
2021-06-15  6:37 ` John Hubbard
2021-06-15 12:09   ` Jann Horn
2021-06-15 12:09     ` Jann Horn
2021-06-15 23:10     ` Yang Shi
2021-06-15 23:10       ` Yang Shi
2021-06-16 17:27       ` Vlastimil Babka
2021-06-16 18:40         ` Yang Shi
2021-06-16 18:40           ` Yang Shi
2021-06-17 16:09           ` Vlastimil Babka
2021-06-18 13:25     ` Jason Gunthorpe
2021-06-18 13:50       ` Matthew Wilcox
2021-06-18 14:58         ` Jason Gunthorpe

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 \
    --in-reply-to=CAG48ez1nZcrJPO-hOLyE08g8HKSGEambCp6mNv6FNR2c9+6sJg@mail.gmail.com \
    --to=jannh@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=jack@suse.cz \
    --cc=jhubbard@nvidia.com \
    --cc=kirill@shutemov.name \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=stable@vger.kernel.org \
    --cc=willy@infradead.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.