Re: [PATCH RFC 4/4] mm/page_alloc_test: Add smoke-test for page allocation

[Brendan Jackman <jackmanb@google.com>]

On Mon, 24 Feb 2025 at 19:26, Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
> > +static void action_nodemask_free(void *ctx)
> > +{
> > +     NODEMASK_FREE(ctx);
> > +}
> > +
> > +/*
> > + * Call __alloc_pages_noprof with a nodemask containing only the nid.
> > + *
> > + * Never returns NULL.
> > + */
> > +static inline struct page *alloc_pages_force_nid(struct kunit *test,
> > +                                              gfp_t gfp, int order, int nid)
> > +{
> > +     NODEMASK_ALLOC(nodemask_t, nodemask, GFP_KERNEL);
>
> For the sake of the test can't we just put the nodemask on the stack?

Hm, I think whether or not it's test code is irrelevant to whether we
can put it on the stack. Presumably the nodemask code is written as it
is because nodemasks can be massive, so we can overflow the stack here
just as easily and confusingly as anywhere else.

(I think we're not in a very deep stack here right now but KUnit could
easily change that).

FWIW I think when using the mm/.kunitconfig provided in this series it
does actually go on the stack.

> > +     struct page *page;
> > +
> > +     KUNIT_ASSERT_NOT_NULL(test, nodemask);
> > +     kunit_add_action(test, action_nodemask_free, &nodemask);
>
> Why aren't we just freeing the nodemask after using it, before we make
> any assertions?

I guess that's just a philosophical question, I usually default to
writing KUnit code such that you can throw an assertion in ~anywhere
and things just work.

But, I'm not passionate about it, I would also be fine with freeing it
directly (it would certainly save quite a few lines of code).

> > +/* Generate test cases as the cross product of orders and alloc_fresh_gfps.  */
> > +static const void *alloc_fresh_gen_params(const void *prev, char *desc)
> > +{
> > +     /* Buffer to avoid allocations. */
> > +     static struct alloc_fresh_test_case tc;
> > +
> > +     if (!prev) {
> > +             /* First call */
> > +             tc.order = 0;
> > +             tc.gfp_idx = 0;
> > +             return &tc;
> > +     }
>
> We need to set 'tc' here to whatever 'prev' is pointing at, right?

prev always points at tc (or is NULL). Sounds like it needs a comment
to that effect!

(Note tc is static).

Ack to everything else, thanks for the review!