Re: [PATCH net-next v12 01/14] mm: page_frag: add a test module for page_frag

[Alexander Duyck]

On Fri, Aug 2, 2024 at 3:02 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2024/8/1 22:50, Alexander Duyck wrote:
>
> >>
> >> The above was my initial thinking too, I went to the ptrpool thing using
> >> at least two CPUs as the below reason:
> >> 1. Test the concurrent calling between allocing and freeing more throughly,
> >>    for example, page->_refcount concurrent handling, cache draining and
> >>    cache reusing code path will be tested more throughly.
> >> 2. Test the performance impact of cache bouncing between different CPUs.
> >>
> >> I am not sure if there is a more lightweight implementation than ptrpool
> >> to do the above testing more throughly.
> >
> > You can still do that with a single producer single consumer ring
> > buffer/array and not have to introduce a ton of extra overhead for
> > some push/pop approach. There are a number of different
> > implementations for such things throughout the kernel.
>
> if we limit that to single producer single consumer, it seems we can
> use ptr_ring to replace ptrpool.

Right. That is more or less what I was thinking.

> >
> >>
> >>>
> >>> Lastly something that is a module only tester that always fails to
> >>> probe doesn't sound like it really makes sense as a standard kernel
> >>
> >> I had the same feeling as you, but when doing testing, it seems
> >> convenient enough to do a 'insmod xxx.ko' for testing without a
> >> 'rmmod xxx.ko'
> >
> > It means this isn't a viable module though. If it supports insmod to
> > trigger your tests you should let it succeed, and then do a rmmod to
> > remove it afterwards. Otherwise it is a test module and belongs in the
> > selftest block.
> >
> >>> module. I still think it would make more sense to move it to the
> >>> selftests tree and just have it build there as a module instead of
> >>
> >> I failed to find one example of test kernel module that is in the
> >> selftests tree yet. If it does make sense, please provide an example
> >> here, and I am willing to follow the pattern if there is one.
> >
> > You must not have been looking very hard. A quick grep for
> > "module_init" in the selftest folder comes up with
> > "tools/testing/selftests/bpf/bpf_testmod/" containing an example of a
> > module built in the selftests folder.
>
> After close look, it seems it will be treated as third party module when
> adding a kernel module in tools/testing/selftests as there seems to be no
> config for it in Kconfig file and can only be compiled as a module not as
> built-in.

Right now you can't compile it as built-in anyway and you were
returning EAGAIN. If you are going that route then it makes more sense
to compile it outside of the mm tree since it isn't a valid module in
the first place.

> >
> >>> trying to force it into the mm tree. The example of dmapool_test makes
> >>> sense as it could be run at early boot to run the test and then it
> >>
> >> I suppose you meant dmapool is built-in to the kernel and run at early
> >> boot? I am not sure what is the point of built-in for dmapool, as it
> >> only do one-time testing, and built-in for dmapool only waste some
> >> memory when testing is done.
> >
> > There are cases where one might want to test on a system w/o console
> > access such as an embedded system, or in the case of an environment
> > where people run without an initrd at all.
>
> I think moving it to tools/testing/selftests may defeat the above purpose.

That is why I am suggesting either fix the module so that it can be
compiled in, or move it to selftest. The current module is not a valid
one and doesn't belong here in its current form.

> >
> >>> just goes quiet. This module won't load and will always just return
> >>> -EAGAIN which doesn't sound like a valid kernel module to me.
> >>
> >> As above, it seems convenient enough to do a 'insmod xxx.ko' for testing
> >> without a 'rmmod xxx.ko'.
> >
> > It is, but it isn't. The problem is it creates a bunch of ugliness in
>
> Yes, it seems a bit ugly, but it supports the below perf cmd, I really
> would like to support the below case as it is very convenient.
>
> perf stat -r 200 -- insmod ./page_frag_test.ko test_push_cpu=16 test_pop_cpu=17

That is fine. If that is the case then it should be in the selftest folder.

> > the build as you are a tristate that isn't a tristate as you are only
> > building it if it is set to "m". There isn't anything like that
> > currently in the mm tree.
>
> After moving page_frag_test to selftest, it is only bulit as module, I guess
> it is ok to return -EAGAIN?

Yes, I would be fine with it in that case.