Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5e481c98-bf82-283f-e826-82802a2bd7d6@intel.com>
Date:   Thu, 10 Aug 2023 19:09:21 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH net] octeontx2-pf: Set page pool size
Content-Language: en-US
To:     Ratheesh Kannoth <rkannoth@marvell.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <sgoutham@marvell.com>, <lcherian@marvell.com>,
        <gakula@marvell.com>, <jerinj@marvell.com>, <hkelam@marvell.com>,
        <sbhatta@marvell.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
References: <20230810024422.1781312-1-rkannoth@marvell.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230810024422.1781312-1-rkannoth@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VkE4SENlZFJsWDAvR0xYYXpZV2FaUFJWZHV3Zzd3d1FKOStDMXZBYVBPdTRE?=
 =?utf-8?B?ZlkxUzRlZW1wazFKUHorZ21DSC9IcDR6QVZtbS9qUkduaGhKU1NUWVRYUCts?=
 =?utf-8?B?TXBnL2VxWWJlbS9LbUF0cjNPeEplZHZ0REZoaERMdG1zMVZZNER3d3NBZXVT?=
 =?utf-8?B?UTZWRVRaSTh2a3hlSEFGNjloYldVUTk5Z3d5cjM2YmlHV3REbytLZGUwYXFE?=
 =?utf-8?B?ZlhhYkptTGFJTHdaVWlsZlhrYlBsaXI3aVJpSXhKbENJMzVqU082V2swakVN?=
 =?utf-8?B?djFTeWc5Vm5uVmdMeSsydEtjamF6NHRuTWwyUFdEOFczWGJxUlovandldEhn?=
 =?utf-8?B?RnRuWm01UWhwdXBaeCt0a2F6RXFEd2NJdlN2aGQxeExQRmF6c2htLy85WlZq?=
 =?utf-8?B?eDNhaWRwWTFGWkU3OEhGaUxQeS85eUkzcnhwSDhWVlFDQTlXYm1CMm54RlFE?=
 =?utf-8?B?d0l6WXRSM3N6U1hVQjNqZW9Qck8waFo2TjlTU3Q0LzROTWgxS2tlQWo5VWRp?=
 =?utf-8?B?dlpEdjZScGVrRWFiQ3hocW5LYS9Uak1aZVQxL0VuZTVXaGh0REZ1eXJNVG5p?=
 =?utf-8?B?MUVpbGpUNExRZjRkZ3FFRzhMdDdacjJlUWxkakFzTG90MVRIdkhNOHhEOGpW?=
 =?utf-8?B?U3FiWmduMHF6VW5lQTBWdXhSNkt6UE13U3liTzBVTVI2NE1aMW5BODJRRExU?=
 =?utf-8?B?WUY1YmJwMWlxbU14VWZZbldjQytsU3JHL1lTV1ZoMndXRTR6UXRQRlFpSTR6?=
 =?utf-8?B?R3c4SDBGRDV4Q1ZsYlhMLzVjSU9Ob3FEanpJMUIydXBFajd3Q05BTmErcVFI?=
 =?utf-8?B?cGRUbkJ6bW84eFNRbTc2S3lEb3EzeDdkVEJ1ZFp2dFo3bEJNVFQ5d25uNDBq?=
 =?utf-8?B?VCt6TVpOZ2lWMmo0aHVQWitObSszVFFHRzJSMTNtQWxzUEduc3EwMjZ2R3Vu?=
 =?utf-8?B?ZEV5aGFKVmJLQndvbjJNUXB2cFBPaWVYelA4cGVFRU1zeUlzTUkxZ3lyQVJY?=
 =?utf-8?B?OW93ODB3ZER3RUg3OGVHVkZBZVJUa0M5SmJvUlVvczdXZStBZGU5OHd3WVNJ?=
 =?utf-8?B?RW55VW9aY0hYSlh6L3Z6Q0ptcXkwd0RESWIyTkV4dkRZbHgvT0hUVUlTY2JS?=
 =?utf-8?B?YlYwTytlRWVmU1daVm5uaWNhOUxOQjJ2UHNLVEhEVS9LdktabDUvbzlwbmdj?=
 =?utf-8?B?eWVpTGdja1lDc2N1RVRaSnZlNk1UNHBoanF2azNUVVFiem1VNWxUS3BQS21N?=
 =?utf-8?B?ZlY4eUthZDdRSjlROE5OWXZYa0lSeGExTE1qR3FWOGI4bXpQWlE3ZjJHYjNV?=
 =?utf-8?B?UC92OE83UXIxcTl6M1h4VjFnTmVRVEJsOEpWZWt5RWEzZWlkNmI0bWxGazdR?=
 =?utf-8?B?WTJmWWFwZVZKdVpuWkZ1TDRSdXMvOUJza2UxaW9tNzRQRmxLdUdrejhrU3Mv?=
 =?utf-8?B?QkU4Y1k3eDlNcjkrVnh4VmZvbmpUdm16TXFkNTJPcVNiM3FqSEhhSy9LUEN1?=
 =?utf-8?B?V2NmL0lBVzBBaFVrUytNT0FYdFhEMzRRdkZPUlpKaEpCNTY4Z1hIaE44SlZo?=
 =?utf-8?B?cXdPZEN6Z2NCZCtzeGFxZWp0V2ZxbitPZStJdS9Gdmt2OE9JM0oySnpyNVNY?=
 =?utf-8?B?RnNObFdlbEh1M0hDV2c3eDdlS1plVUZDUWRwRkZXenZWVXNOU0k0dkJQWXdx?=
 =?utf-8?B?TjZ4bTFYQ2xmb25kVzlIdkFHaTVQNkpNSXNKaGxvNURwWTNST3AyMXE0OTNS?=
 =?utf-8?B?K3l2amcxOXA0dEh1aTBQR2NJR0hlQ1BWTk5jb1NlcXZTYzgzeFJzV1RQTjVn?=
 =?utf-8?B?dUlXbHBOc3FVSVNoUUMreU1pazZ4aml5M1pTWXJlS3czOG5QYis0TW9GZmZI?=
 =?utf-8?B?SkRxZnA3SWp4eXZ4K0dHWnJwR0o5Qi9RcnIveWFGUkU1VjhhWkoxOWk5ZUl0?=
 =?utf-8?B?ZzRLTENZMGdhejFFb3czS3JWM2pOY1BBajlUZWlwZDlPWGExc2t6RFF6d0pM?=
 =?utf-8?B?NVpBNEZPV1pPRFlGc0c5ZXo1TitFS1BwdXVobFBYREFMQmkvTE1pR1hZT0o0?=
 =?utf-8?B?NjNyaStKam1nL1JETENiR1dpci9OSTZpalRjSmw5ZE1tZlhrU2paYkZEdFhB?=
 =?utf-8?B?MDh4cGw0aUJ0V1djS1Vmak5nZ1RBU1FWUkV1aERVSTE1SFhGcW0xaDZpeTZ2?=
 =?utf-8?B?QVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35e62e76-e86d-4eb8-2bd9-08db99c4abbc
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Aug 2023 17:10:17.8211
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: tbG4Vvd57qInkn8XHB5EcDGcFkv7Q2TRWUveihdZOU7148IhY8vgHmlJYfy17AxPHTdnqXcGKMuuFaxVUlyMG6+cx6NbHkbGLSOFNWs058Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5679
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Ratheesh Kannoth <rkannoth@marvell.com>
Date: Thu, 10 Aug 2023 08:14:22 +0530

> page pool infra does direct recycling aggressively.
> This would often keep ptr_ring left unused. Save
> memory by configuring ptr_ring to a constant value(2K).
> 
> Please find discussion at
> https://lore.kernel.org/netdev/
> 	15d32b22-22b0-64e3-a49e-88d780c24616@kernel.org/T/
> 
> Fixes: b2e3406a38f0 ("octeontx2-pf: Add support for page pool")

Now the commit message doesn't explain why this is a fix.
The subject is also ambigous.
In the subject, you need to say clearly what you're fixing. E.g. "fix
page_pool creation fail for rings > 32k".
In the commitmsg, provide the actual kernel warning/error and explain
some implementation details, like: "instead of clamping page_pool size
to 32k at most, limit it even more to 2k to avoid wasting memory on much
less used now ptr_ring".

> Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 77c8f650f7ac..123348a9e19e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -1432,7 +1432,8 @@ int otx2_pool_init(struct otx2_nic *pfvf, u16 pool_id,
>  	}
>  
>  	pp_params.flags = PP_FLAG_PAGE_FRAG | PP_FLAG_DMA_MAP;
> -	pp_params.pool_size = numptrs;
> +#define OTX2_PAGE_POOL_SZ 2048
> +	pp_params.pool_size = OTX2_PAGE_POOL_SZ;

And if the ring size is e.g. 256 or 512 or even 1024, why have Page Pool
with 2048 elements? Should be something like

min(numptrs, OTX2_PAGE_POOL_MAX_SZ)

And please place the definition somewhere next to other definitions at
the top of the file or in some header, dunno. Placing it inside the
function almost guarantees you won't be able to find it one day.

>  	pp_params.nid = NUMA_NO_NODE;
>  	pp_params.dev = pfvf->dev;
>  	pp_params.dma_dir = DMA_FROM_DEVICE;

Thanks,
Olek