Re: [PATCH bpf-next v2 2/6] mm, bpf: Introduce free_pages_nolock()

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

On Wed, Dec 11, 2024 at 2:11 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 12/10/24 03:39, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Introduce free_pages_nolock() that can free a page without taking locks.
> > It relies on trylock only and can be called from any context.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> <snip>
>
> > +/* Can be called while holding raw_spin_lock or from IRQ. RCU must be watching. */
> > +void free_pages_nolock(struct page *page, unsigned int order)
>
> What does "RCU must be watching" mean and why?

Meaning that rcu_is_watching() must be true.
Because pgalloc_tag_get() relies on RCU.
ree_unref_page() and things it calls doesn't use RCU directly,
but it calls tracepoints and they need RCU too.

Hence the comment to say that free_pages_nolock() cannot be
called in _any_ context. Some care needs to be taken.
bpf needs RCU, so we don't allow attach bpf to idle and notrace,
but, last I heard, notrace attr is not 100% reliable on some odd arch-es.

> > +{
> > +     int head = PageHead(page);
> > +     struct alloc_tag *tag = pgalloc_tag_get(page);
> > +
> > +     if (put_page_testzero(page)) {
> > +             __free_unref_page(page, order, FPI_TRYLOCK);
> > +     } else if (!head) {
> > +             pgalloc_tag_sub_pages(tag, (1 << order) - 1);
> > +             while (order-- > 0)
> > +                     __free_unref_page(page + (1 << order), order, FPI_TRYLOCK);
>
> Not your fault for trying to support everything __free_pages did,
> specifically order > 0 pages that are not compound and thus needing this
> extra !head branch. We'd love to get rid of that eventually in
> __free_pages(), but at least I think we don't need to support it in a new
> API and instead any users switching to it should know it's not supported.

Great. Good to know.

> I suppose BFP doesn't need that, right?

Nope.
Initially I wrote above bit as:

void free_pages_nolock(struct page *page, unsigned int order)
{
         if (put_page_testzero(page))
                __free_unref_page(page, order, FPI_TRYLOCK);
}

but then I studied Matthew's commit
e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")

and couldn't convince myself that the race he described
+               get_page(page);
+               free_pages(addr, 3);
+               put_page(page);

will never happen.
It won't happen if bpf is the only user of this api,
but who knows what happens in the future.
So copy-pasted this part just in case.
Will be happy to drop it.

> Maybe if the function was taking a folio instead of page, it would be the
> best as that has to be order-0 or compound by definition. It also wouldn't
> need the order parameter. What do you think, Matthew?

For bpf there is no use case for folios.
All we need is a page order 0 and separately
later kmalloc_nolock() will call it from new_slab().
And I think new_slab() might need order >= 1.
So that's the reason to have order parameter.