| Message ID | 20230814060411.2401817-1-rkannoth@marvell.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v1,net] page_pool: Cap queue size to 32k. | expand |
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
> 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
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)
> 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().
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
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,
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(-)