Re: [PATCH RFC v3 06/36] kmsan: gfp: introduce __GFP_NO_KMSAN_SHADOW

[Alexander Potapenko <glider@google.com>]

On Wed, Nov 27, 2019 at 3:48 PM Marco Elver <elver@google.com> wrote:
>
> On Fri, 22 Nov 2019 at 12:26, <glider@google.com> wrote:
> >
> > This flag is to be used by KMSAN runtime to mark that newly created
> > memory pages don't need KMSAN metadata backing them.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > To: Alexander Potapenko <glider@google.com>
> > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: linux-mm@kvack.org
> >
> > ---
> > We can't decide what to do here:
> >  - do we need to conditionally define ___GFP_NO_KMSAN_SHADOW depending on
> >    CONFIG_KMSAN like LOCKDEP does?
> >  - if KMSAN is defined, and LOCKDEP is not, do we want to "compactify" the GFP
> >    bits?
>
> A maintainer would know the real answer, but this is my guess: making
> the behaviour not change without KMSAN would probably be better. It
> would require some ifdef trickery to deal with LOCKDEP on/off case
> though.
I actually find it quite unfortunate that LOCKDEP has this on/off case.
It's already inconvenient to add a new GFP flag past ___GFP_NOLOCKDEP,
as its value will be depending on this blinking LOCKDEP bit.
Now imagine someone wants to add a second GFP flag that can be
disabled similarly to LOCKDEP - there'll be even more hassle counting
which bits are present.

Michal, do you think it's possible to make __GFP_BITS_SHIFT
independent from CONFIG_LOCKDEP?

> > Change-Id: If5d0352fd5711ad103328e2c185eb885e826423a
> > ---
> >  include/linux/gfp.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > index fb07b503dc45..b4e7963cd94b 100644
> > --- a/include/linux/gfp.h
> > +++ b/include/linux/gfp.h
> > @@ -44,6 +44,7 @@ struct vm_area_struct;
> >  #else
> >  #define ___GFP_NOLOCKDEP       0
> >  #endif
>
> Since this change unconditionally changes GFP_BITS_SHIFT to 25, the
> #ifdef for GFP_NOLOCKDEP could also go away -- but: see above.
>
> > +#define ___GFP_NO_KMSAN_SHADOW  0x1000000u
> >  /* If the above are modified, __GFP_BITS_SHIFT may need updating */
> >
> >  /*
> > @@ -212,12 +213,13 @@ struct vm_area_struct;
> >  #define __GFP_NOWARN   ((__force gfp_t)___GFP_NOWARN)
> >  #define __GFP_COMP     ((__force gfp_t)___GFP_COMP)
> >  #define __GFP_ZERO     ((__force gfp_t)___GFP_ZERO)
> > +#define __GFP_NO_KMSAN_SHADOW  ((__force gfp_t)___GFP_NO_KMSAN_SHADOW)
>
> Should this be ordered after __GFP_NOLOCKDEP with a brief comment what
> it does?  All of these up to __GFP_ZERO have a doc comment above.
>
> >  /* Disable lockdep for GFP context tracking */
> >  #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
> >
> >  /* Room for N __GFP_FOO bits */
> > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
> > +#define __GFP_BITS_SHIFT (25)
> >  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
> >
> >  /**
> > --
> > 2.24.0.432.g9d3f5f5b63-goog
> >



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg