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

[Jakub Kicinski]

On Mon, 07 Aug 2023 07:18:21 -0700 Alexander H Duyck wrote:
> > 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)
>
> 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.

All the points y'all making are valid, sizing the cache is a hard
problem. But the proposed solution goes in the wrong direction, IMO.
The driver doesn't know. I started hacking together page pool control
over netlink. I think that the pool size selection logic should be in
the core, with inputs taken from user space / workload (via netlink).

If it wasn't for the fact that I'm working on that API I'd probably
side with you. And 64k descriptors is impractically large.

Copy / pasting from the discussion on previous version:

  Tuning this in the driver relies on the assumption that the HW /
  driver is the thing that matters. I'd think that the workload,
  platform (CPU) and config (e.g. is IOMMU enabled?) will matter at
  least as much. While driver developers will end up tuning to whatever
  servers they have, random single config and most likely.. iperf.

  IMO it's much better to re-purpose "pool_size" and treat it as the ring
  size, because that's what most drivers end up putting there. 
  Defer tuning of the effective ring size to the core and user input 
  (via the "it will be added any minute now" netlink API for configuring
  page pools)...

  So capping the recycle ring to 32k instead of returning the error seems
  like an okay solution for now.