Re: [PATCH v2] mm/gup: fix try_grab_compound_head() race with split_huge_page()

[Jann Horn]

On Tue, Jun 15, 2021 at 4:00 AM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 15 Jun 2021 03:20:14 +0200 Jann Horn <jannh@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/>.

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.)