Re: [PATCH net-next] page_pool: Clamp ring size to 32K

[Alexander H Duyck]

On Mon, 2023-08-07 at 13:42 +0200, Jesper Dangaard Brouer wrote:
> 
> On 07/08/2023 05.49, Ratheesh Kannoth wrote:
> > https://lore.kernel.org/netdev/20230804133512.4dbbbc16@xxxxxxxxxx/T/
> > Capping the recycle ring to 32k instead of returning the error.
> > 
> 
> Page pool (PP) is just a cache of pages.  The driver octeontx2 (in link)
> is creating an excessive large cache of pages.  The drivers RX
> descriptor ring size should be independent of the PP ptr_ring size, as
> it is just a cache that grows as a functions of the in-flight packet
> workload, it functions as a "shock absorber".
> 
> 32768 pages (4KiB) is approx 128 MiB, and this will be per RX-queue.
> 
> The RX-desc ring (obviously) pins down these pages (immediately), but PP
> ring starts empty.  As the workload varies the "shock absorber" effect
> will let more pages into the system, that will travel the PP ptr_ring.
> As all pages originating from the same PP instance will get recycled,
> the in-flight pages in the "system" (PP ptr_ring) will grow over time.
> 
> The PP design have the problem that it never releases or reduces pages
> in this shock absorber "closed" system. (Cc. PP people/devel) we should
> consider implementing a MM shrinker callback (include/linux/shrinker.h).
> 
> Are the systems using driver octeontx2 ready to handle 128MiB memory per
> RX-queue getting pinned down overtime? (this could lead to some strange
> do debug situation if the memory is not sufficient)
> 
> --Jesper

I'm with Jesper on this. It doesn't make sense to be tying the
page_pool size strictly to the ring size. The amount of recycling you
get will depend on how long the packets are on the stack, not in the
driver.

For example, in the case of something like a software router or bridge
that is just taking the Rx packets and routing them to Tx you could
theoretically get away with a multiple of NAPI_POLL_WEIGHT since you
would likely never need much more than that as the Tx would likely be
cleaned about as fast as the Rx can consume the pages.

Rather than overriding the size here wouldn't it make more sense to do
it in the octeontx2 driver? With that at least you would know that you
were the one that limited the size instead of having the value modified
out from underneath you.

That said, one change that might help to enable this kind of change
would be look at adding a #define so that this value wouldn't be so
much a magic number and would be visible to the drivers should it ever
be changed in the future.