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

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

[Ratheesh Kannoth]

https://lore.kernel.org/netdev/20230804133512.4dbbbc16@kernel.org/T/
Capping the recycle ring to 32k instead of returning the error.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
---
 net/core/page_pool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5d615a169718..404f835a94be 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -182,9 +182,9 @@ static int page_pool_init(struct page_pool *pool,
 	if (pool->p.pool_size)
 		ring_qsize = pool->p.pool_size;
 
-	/* Sanity limit mem that can be pinned down */
+	/* Clamp to 32K */
 	if (ring_qsize > 32768)
-		return -E2BIG;
+		ring_qsize = 32768;
 
 	/* DMA direction is either DMA_FROM_DEVICE or DMA_BIDIRECTIONAL.
 	 * DMA_BIDIRECTIONAL is for allowing page used for DMA sending,
-- 
2.25.1

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

[Jesper Dangaard Brouer]

On 07/08/2023 05.49, Ratheesh Kannoth wrote:
> https://lore.kernel.org/netdev/20230804133512.4dbbbc16@kernel.org/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

> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
> ---
>   net/core/page_pool.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 5d615a169718..404f835a94be 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -182,9 +182,9 @@ static int page_pool_init(struct page_pool *pool,
>   	if (pool->p.pool_size)
>   		ring_qsize = pool->p.pool_size;
>   
> -	/* Sanity limit mem that can be pinned down */
> +	/* Clamp to 32K */
>   	if (ring_qsize > 32768)
> -		return -E2BIG;
> +		ring_qsize = 32768;
>   
>   	/* DMA direction is either DMA_FROM_DEVICE or DMA_BIDIRECTIONAL.
>   	 * DMA_BIDIRECTIONAL is for allowing page used for DMA sending,

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@kernel.org/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.

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.

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

[Jesper Dangaard Brouer]

On 07/08/2023 19.20, Jakub Kicinski wrote:
> 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.
>>

Thanks for agreeing with me, and I agree with you :-)

>> 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.
>>

I agree.

>> 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.
>>

I'm not fully agreeing here.  I don't think we can expect driver
developer to be experts on page_pool cache dynamics.  I'm more on
Jakub's side here, as perhaps we/net-core can come up with some control
system, even if this means we change this underneath drivers.


>> 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.

I disagree here, as driver developers should not treat "pool_size" as
the ring size.  It seems to be a copy-paste-programming scheme without
understanding PP dynamics.

>    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)...
> 

I agree here, that tuning ring size is a hard problem, and this is
better handled in the core.  Happy to hear, that/if Jakub is working on
this.

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

As a temporary solution, I'm actually fine with capping at 32k.
Driver developer loose some feedback control, but perhaps that is okay,
if we can agree that the net-core should control tuning this anyhow.

--Jesper

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

[Ratheesh Kannoth]

> From: Jesper Dangaard Brouer <hawk@kernel.org>
> Sent: Tuesday, August 8, 2023 1:42 AM
> As a temporary solution, I'm actually fine with capping at 32k.
> Driver developer loose some feedback control, but perhaps that is okay, if
> we can agree that the net-core should control tuning this anyhow.


Capping will never let user know that memory is unnecessarily wasted as there is not much
Correlation between ring size and page pool size.  I would prefer not setting pool->p.pool_size in
Octeontx2  (pool->p.pool_size =  0 ) driver  and let page pool infra decide on that. Is this change acceptable ?
100G link burst traffic would be able to handle with 1024 page pool size, do we have any data with any other driver ?

-Ratheesh

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

[Alexander Lobakin]

From: Jesper Dangaard Brouer <hawk@kernel.org>
Date: Mon, 7 Aug 2023 22:11:35 +0200

> 
> 
> On 07/08/2023 19.20, Jakub Kicinski wrote:
>> 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.
>>>
> 
> Thanks for agreeing with me, and I agree with you :-)
> 
>>> 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.
>>>
> 
> I agree.
> 
>>> 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.
>>>
> 
> I'm not fully agreeing here.  I don't think we can expect driver
> developer to be experts on page_pool cache dynamics.  I'm more on
> Jakub's side here, as perhaps we/net-core can come up with some control
> system, even if this means we change this underneath drivers.
> 
> 
>>> 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.
> 
> I disagree here, as driver developers should not treat "pool_size" as
> the ring size.  It seems to be a copy-paste-programming scheme without
> understanding PP dynamics.

+1. That's why I wrote in the previous thread that pool_size must be the
minimum value which gives optimal performance. I don't believe Otx2 HW
needs 32k entries in PP's ptr_ring to have optimal performance.
That's why I wrote that developers must check whether there's any
benefit in using bigger pool_size values. Values bigger than 2k don't
seem reasonable to me, especially now that we use direct recycling way
more aggressively -- often times ptr_ring is left unused at all.
Jakub thought that my "pls test whether bigger sizes make sense" meant
"please tune Page Pool to your servers", not exactly what I wanted to
say =\ I said only "please keep pool_size reasonable, it's your right to
have 2^32 descriptors on the ring, but don't do that with Page Pool".

> 
>>    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)...
>>
> 
> I agree here, that tuning ring size is a hard problem, and this is
> better handled in the core.  Happy to hear, that/if Jakub is working on
> this.
> 
>>    So capping the recycle ring to 32k instead of returning the error
>> seems
>>    like an okay solution for now.
> 
> As a temporary solution, I'm actually fine with capping at 32k.
> Driver developer loose some feedback control, but perhaps that is okay,
> if we can agree that the net-core should control tuning this anyhow.
> 
> --Jesper

Thanks,
Olek