[PATCH v1 net] page_pool: Cap queue size to 32k.

[PATCH v1 net] page_pool: Cap queue size to 32k.

[Ratheesh Kannoth]

Clamp to 32k instead of returning error.

Please find discussion at
https://lore.kernel.org/lkml/
CY4PR1801MB1911E15D518A77535F6E51E2D308A@CY4PR1801MB1911.
namprd18.prod.outlook.com/T/

Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>

---
ChangeLog:
v0 -> v1: Rebase && commit message changes
---
 net/core/page_pool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a3e12a61d456..e9dc8d8966ad 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -171,9 +171,10 @@ 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 */
+	/* Cap queue size 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 v1 net] page_pool: Cap queue size to 32k.

[Johannes Berg]

On Mon, 2023-08-14 at 11:34 +0530, Ratheesh Kannoth wrote:
> Clamp to 32k instead of returning error.
> 
> Please find discussion at
> https://lore.kernel.org/lkml/
> CY4PR1801MB1911E15D518A77535F6E51E2D308A@CY4PR1801MB1911.
> namprd18.prod.outlook.com/T/
> 

I'm not the one who's going to apply this, but honestly, I don't think
that will work as a commit message for such a change ...

Sure, link to it by all means, but also summarize it and make sense of
it for the commit message?

johannes

RE: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.

[Ratheesh Kannoth]

> From: Johannes Berg <johannes@sipsolutions.net>
> Subject: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.
> > Please find discussion at
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_l
> >
> I'm not the one who's going to apply this, but honestly, I don't think that will
> work as a commit message for such a change ...
> Sure, link to it by all means, but also summarize it and make sense of it for
> the commit message?

Why do you think it will not work ?. There is a long discussion about pros and cons of different approaches by 
Page pool maintainers in the discussion link.  However I  summarize it here, as commit message, it will 
Lead to some more questions by reviewers. 

-Ratheesh

 
 

Re: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.

[Jesper Dangaard Brouer]

On 14/08/2023 10.05, Ratheesh Kannoth wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Subject: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.
>>> Please find discussion at
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_l
>>>
>> I'm not the one who's going to apply this, but honestly, I don't think that will
>> work as a commit message for such a change ...
>> Sure, link to it by all means, but also summarize it and make sense of it for
>> the commit message?
> 
> Why do you think it will not work ?. There is a long discussion about pros and cons of different approaches by
> Page pool maintainers in the discussion link.  However I  summarize it here, as commit message, it will
> Lead to some more questions by reviewers.
> 

I agree with Johannes, this commit message is too thin.

It makes sense to give a summary of the discussion, because it show us
(page_pool maintainers) what you concluded for the discussion.

Further more, you also send another patch:
  - "[PATCH net-next] page_pool: Set page pool size"
  - 
https://lore.kernel.org/all/20230809021920.913324-1-rkannoth@marvell.com/

That patch solves the issue for your driver marvell/octeontx2 and I like
than change.

Why did you conclude that PP core should also change?

--Jesper
(p.s. Cc/To list have gotten excessive with 89 recipients)

RE: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.

[Ratheesh Kannoth]

> From: Jesper Dangaard Brouer <hawk@kernel.org>
> Subject: Re: [EXT] Re: [PATCH v1 net] page_pool: Cap queue size to 32k.


> I agree with Johannes, this commit message is too thin.
ACK.

 
> It makes sense to give a summary of the discussion, because it show us
> (page_pool maintainers) what you concluded for the discussion.
Got it. Thanks. 

> Further more, you also send another patch:
>   - "[PATCH net-next] page_pool: Set page pool size"
Okay. 

>   -
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_all_20230809021920.913324-2D1-2Drkannoth-
> 40marvell.com_&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=aekcsyBCH00
> _LewrEDcQBzsRw8KCpUR0vZb_auTHk4M&m=uvV_vt_cNyQItTD90jF1LdKovP
> 7j7FYtnr7I38__nYY6wHtFHSozYoRSSvCI14nh&s=vGgt2ccGdiRTEhj3MoGVx-
> EXHmB03v6I3UIIY1fEb24&e=
> 
> That patch solves the issue for your driver marvell/octeontx2 and I like than
> change.
Okay. 

> Why did you conclude that PP core should also change?
I could not  answer Jacub's question at https://lore.kernel.org/netdev/20230810024422.1781312-1-rkannoth@marvell.com/T/ 

> (p.s. Cc/To list have gotten excessive with 89 recipients)
I added maintainters of all files which used page_pool_init(). 

Re: [PATCH v1 net] page_pool: Cap queue size to 32k.

[Dan Carpenter]

On Mon, Aug 14, 2023 at 11:34:11AM +0530, Ratheesh Kannoth wrote:
> Clamp to 32k instead of returning error.

What is the motivation here?  What is the real world impact for the
users?

> 
> Please find discussion at
> https://lore.kernel.org/lkml/
> CY4PR1801MB1911E15D518A77535F6E51E2D308A@CY4PR1801MB1911.
> namprd18.prod.outlook.com/T/

Please don't break the URL up like this.  I think normally we would just
write up a normal commit message and use the Link: tag.

Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
Link: https://lore.kernel.org/lkml/CY4PR1801MB1911E15D518A77535F6E51E2D308A@CY4PR1801MB1911.namprd18.prod.outlook.com/
Signed-off-by:

> @@ -171,9 +171,10 @@ 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 */
> +	/* Cap queue size to 32k */
>  	if (ring_qsize > 32768)
> -		return -E2BIG;
> +		ring_qsize = 32768;
> +
>  
>  	/* DMA direction is either DMA_FROM_DEVICE or DMA_BIDIRECTIONAL.

Don't introduce a blank line here.  Checkpatch will complain if you
have to blank lines in a row.  It won't complain about the patch but it
will complain if you apply the patch and then re-run checkpatch -f on
the file.  (I didn't test this but it's wrong either way. :P).

regards,
dan carpenter