Re: [PATCH RFC net-next v2] page_pool: import Jesper's page_pool benchmark

[Ilias Apalodimas]

Thanks Jesper,

On Wed, 4 Jun 2025 at 11:39, Jesper Dangaard Brouer <hawk@xxxxxxxxxx> wrote:
>
>
>
> On 28/05/2025 21.46, Mina Almasry wrote:
> > On Wed, May 28, 2025 at 2:28 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >>
> >> Mina Almasry <almasrymina@xxxxxxxxxx> writes:
> >>
> >>> On Mon, May 26, 2025 at 5:51 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >>>>> Fast path results:
> >>>>> no-softirq-page_pool01 Per elem: 11 cycles(tsc) 4.368 ns
> >>>>>
> >>>>> ptr_ring results:
> >>>>> no-softirq-page_pool02 Per elem: 527 cycles(tsc) 195.187 ns
> >>>>>
> >>>>> slow path results:
> >>>>> no-softirq-page_pool03 Per elem: 549 cycles(tsc) 203.466 ns
> >>>>> ```
> >>>>>
> >>>>> Cc: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> >>>>> Cc: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
> >>>>> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> >>>>> Cc: Toke Høiland-Jørgensen <toke@xxxxxxx>
> >>>>>
> >>>>> Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> >>>>
> >>>> Back when you posted the first RFC, Jesper and I chatted about ways to
> >>>> avoid the ugly "load module and read the output from dmesg" interface to
> >>>> the test.
> >>>>
> >>>
> >>> I agree the existing interface is ugly.
> >>>
> >>>> One idea we came up with was to make the module include only the "inner"
> >>>> functions for the benchmark, and expose those to BPF as kfuncs. Then the
> >>>> test runner can be a BPF program that runs the tests, collects the data
> >>>> and passes it to userspace via maps or a ringbuffer or something. That's
> >>>> a nicer and more customisable interface than the printk output. And if
> >>>> they're small enough, maybe we could even include the functions into the
> >>>> page_pool code itself, instead of in a separate benchmark module?
> >>>>
> >>>> WDYT of that idea? :)
> >>>
> >>> ...but this sounds like an enormous amount of effort, for something
> >>> that is a bit ugly but isn't THAT bad. Especially for me, I'm not that
> >>> much of an expert that I know how to implement what you're referring
> >>> to off the top of my head. I normally am open to spending time but
> >>> this is not that high on my todolist and I have limited bandwidth to
> >>> resolve this :(
> >>>
> >>> I also feel that this is something that could be improved post merge.
> >>> I think it's very beneficial to have this merged in some form that can
> >>> be improved later. Byungchul is making a lot of changes to these mm
> >>> things and it would be nice to have an easy way to run the benchmark
> >>> in tree and maybe even get automated results from nipa. If we could
> >>> agree on mvp that is appropriate to merge without too much scope creep
> >>> that would be ideal from my side at least.
> >>
> >> Right, fair. I guess we can merge it as-is, and then investigate whether
> >> we can move it to BPF-based (or maybe 'perf bench' - Cc acme) later :)
> >
> > Thanks for the pliability. Reviewed-bys and comments welcome.
> >
> > Additionally Signed-off-by from Jesper is needed I think. Since most
> > of this code is his, I retained his authorship. Jesper, whenever this
> > looks good to me, a signed-off-by would be good and I would carry it
> > to future versions. Changing authorship to me is also fine by me but I
> > would think you want to retain the credit.
>
> Okay, I think Ilias'es comment[1] and ACK convinced me, let us merge
> this as-is.  We have been asking people to run it over several years
> before accepting patches. We shouldn't be pointing people to use
> out-of-tree tests for accepting patches.
>
> It is not perfect, but it have served us well for benchmarking in the
> last approx 10 years (5 years for page_pool test).  It is isolated as a
> selftest under (tools/testing/selftests/net/bench/page_pool/).
>
> Realistically we are all too busy inventing a new "perfect" benchmark
> for page_pool. That said, I do encourage others with free cycles to
> integrated a better benchmark test into `perf bench`.  Then we can just
> remove this module again.

I'll spend some time looking at acme comments. They seem to be moving
towards the right direction

Thanks
/Ilias
>
> Signed-off-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
>
>   [1]
> https://lore.kernel.org/all/CAC_iWjLmO4XZ_+PBaCNxpVCTmGKNBsLGyeeKS2ptRrepn1u0SQ@xxxxxxxxxxxxxx/
>
> Thanks Mina for pushing this forward,
> --Jesper