Re: [PATCH bpf-next v3 1/6] mm, bpf: Introduce try_alloc_pages() for opportunistic page allocation

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Dec 18, 2024 at 3:32 AM Michal Hocko <mhocko@suse.com> wrote:
>
> I like this proposal better. I am still not convinced that we really
> need internal __GFP_TRYLOCK though.
>
> If we reduce try_alloc_pages to the gfp usage we are at the following
>
> On Tue 17-12-24 19:07:14, alexei.starovoitov@gmail.com wrote:
> [...]
> > +struct page *try_alloc_pages_noprof(int nid, unsigned int order)
> > +{
> > +     gfp_t alloc_gfp = __GFP_NOWARN | __GFP_ZERO |
> > +                       __GFP_NOMEMALLOC | __GFP_TRYLOCK;
> > +     unsigned int alloc_flags = ALLOC_TRYLOCK;
> [...]
> > +     prepare_alloc_pages(alloc_gfp, order, nid, NULL, &ac,
> > +                         &alloc_gfp, &alloc_flags);
> [...]
> > +     page = get_page_from_freelist(alloc_gfp, order, alloc_flags, &ac);
> > +
> > +     /* Unlike regular alloc_pages() there is no __alloc_pages_slowpath(). */
> > +
> > +     trace_mm_page_alloc(page, order, alloc_gfp & ~__GFP_TRYLOCK, ac.migratetype);
> > +     kmsan_alloc_page(page, order, alloc_gfp);
> [...]
>
> From those that care about __GFP_TRYLOCK only kmsan_alloc_page doesn't
> have alloc_flags. Those could make the locking decision based on
> ALLOC_TRYLOCK.

__GFP_TRYLOCK here sets a baseline and is used in patch 4 by inner
bits of memcg's consume_stock() logic while called from
try_alloc_pages() in patch 5.
We cannot pass alloc_flags into it.
Just too much overhead.

__memcg_kmem_charge_page()
  -> obj_cgroup_charge_pages()
      -> try_charge_memcg()
         -> consume_stock()

all of them would need an extra 'u32 alloc_flags'.
This is too high cost to avoid ___GFP_TRYLOCK_BIT in gfp_types.h

> I am not familiar with kmsan internals and my main question is whether
> this specific usecase really needs a dedicated reentrant
> kmsan_alloc_page rather than rely on gfp flag to be sufficient.
> Currently kmsan_in_runtime bails out early in some contexts. The
> associated comment about hooks is not completely clear to me though.
> Memory allocation down the road is one of those but it is not really
> clear to me whether this is the only one.

As I mentioned in giant v2 thread I'm not touching kasan/kmsan
in this patch set, since it needs its own eyes
from experts in those bits,
but when it happens gfp & __GFP_TRYLOCK would be the way
to adjust whatever is necessary in kasan/kmsan internals.

As Shakeel mentioned, currently kmsan_alloc_page() is gutted,
since I'm using __GFP_ZERO unconditionally here.
We don't even get to kmsan_in_runtime() check.
For bpf use cases __GFP_ZERO and __GFP_ACCOUNT are pretty much
mandatory. When there will be a 2nd user of this try_alloc_pages()
api we can consider making flags for these two
and at that time full analysis kmsan reentrance would be necessary.
It works in this patch because of GFP_ZERO.

So __GFP_TRYLOCK is needed in many cases:
- to make decisions in consume_stock()
- in the future in kasan/kmsan
- and in slab kmalloc. There I'm going to introduce try_kmalloc()
(or kmalloc_nolock(), naming is hard) that will use this
internal __GFP_TRYLOCK flag to avoid locks and when it gets
to new_slab()->allocate_slab()->alloc_slab_page()
the latter will use try_alloc_pages() instead of alloc_pages().