On Wed, Dec 11, 2024 at 2:11 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> On 12/10/24 03:39, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> <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.
