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

Alexei Starovoitov
2024-12-18T07:25:32Z

On Tue, Dec 17, 2024 at 10:49 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Tue, Dec 17, 2024 at 10:37 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Dec 17, 2024 at 9:58 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > What I mean is, functions like __free_unref_page() and
> > > free_unref_page_commit() now accept fpi_flags, but any flags other
> > > than FPI_TRYLOCK are essentially ignored, also not very clear.
> >
> > They're not ignored. They are just not useful in this context.
>
> I think they are. For example, if you pass FPI_SKIP_REPORT_NOTIFY to
> __free_unref_page(), page_reporting_notify_free() will still be called
> when the page is eventually freed to the buddy allocator. Same goes
> for FPI_NO_TAIL.

free_pcppages_bulk()->page_reporting_notify_free() will _not_ be called
when FPI_TRYLOCK is specified.
They are internal flags. The callers cannot make try_alloc_pages()
pass these extra flags.
The flags are more or less exclusive.

> > The code rules over comment. If you have a concrete suggestion on
> > how to improve the comment please say so.
>
> What I had in mind is adding a WARN in the pcp freeing functions if
> any FPI flag but FPI_TRYLOCK is passed, and/or explicitly calling out
> that other flags should not be passed as they have no effect in this
> context (whether at the function definition, above the WARN, or at the
> flag definitions).

pcp freeing functions?
In particular?
tbh this sounds like defensive programming...
BUILD_BUG_ON is a good thing when api is misused,
but WARN will be wasting run-time cycles on something that should
have been caught during code review.
free_unref_page_commit() is a shallow function when FPI_TRYLOCK is used.
There is no need to propagate fpi_flags further into free_pcppages_bulk
just to issue a WARN.