On Wed 18-12-24 17:18:51, Alexei Starovoitov wrote:
> On Wed, Dec 18, 2024 at 3:32 AM Michal Hocko <mhocko@xxxxxxxx> 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@xxxxxxxxx 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.
Yes, I have addressed that part in a reply. In short I believe we can
achieve reentrancy for NOWAIT/ATOMIC charges without a dedicated gfp
flag.
[...]
> > 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.
I have missed that part! That means that you can drop kmsan_alloc_page
altogether no?
[...]
> - 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().
I cannot really comment on the slab side of things. All I am saying is
that we should _try_ to avoid __GFP_TRYLOCK if possible/feasible. It
seems that the page allocator can do without that. Maybe slab side can
as well.
--
Michal Hocko
SUSE Labs
