RE: Re: [PATH net-next v1] octeontx2-pf: Add support for page pool

[Ratheesh Kannoth]

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Thursday, May 18, 2023 9:17 AM
> To: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri
> Goutham <sgoutham@xxxxxxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; Subbaraya Sundeep Bhatta
> <sbhatta@xxxxxxxxxxx>; Geethasowjanya Akula <gakula@xxxxxxxxxxx>;
> Srujana Challa <schalla@xxxxxxxxxxx>; Hariprasad Kelam
> <hkelam@xxxxxxxxxxx>
> Subject: [EXT] Re: [PATH net-next v1] octeontx2-pf: Add support for page
> pool
> On Wed, 17 May 2023 09:45:11 +0530 Ratheesh Kannoth wrote:
> > Page pool for each rx queue enhance rx side performance by reclaiming
> > buffers back to each queue specific pool. DMA mapping is done only for
> > first allocation of buffers.
> > As subsequent buffers allocation avoid DMA mapping, it results in
> > performance improvement.
> >
> > Image        |  Performance with Linux kernel Packet Generator
> > ------------ | -----------------------------------------------
> > Vannila      |   3Mpps
> >              |
> > with this    |   42Mpps
> > change	     |
> > ----------------------------------------------------------------
> >
> > Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>
> >
> 
> Put an extra --- here, to place the change log outside the normal commit
> message.
Okay. 

> 
> > ChangeLog
> > v0 -> v1: Removed CONFIG_PAGE_POOL #ifdefs in code
> > 	  Used compound page APIs
> > 	  Replaced page_pool_put_page API with page_pool_put_full_page
> API
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > index f9286648e45c..49df1876eca3 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > @@ -518,11 +518,36 @@ void otx2_config_irq_coalescing(struct otx2_nic
> *pfvf, int qidx)
> >  		     (pfvf->hw.cq_ecount_wait - 1));  }
> >
> > +static int otx2_alloc_pool_buf(struct otx2_nic *pfvf, struct otx2_pool
> *pool,
> > +			       dma_addr_t *dma)
> > +{
> > +	unsigned int offset = 0;
> > +	struct page *page;
> > +	size_t sz;
> > +
> > +	sz = SKB_DATA_ALIGN(pool->rbsize);
> > +	sz = ALIGN(sz, OTX2_ALIGN);
> > +
> > +	page = page_pool_alloc_frag(pool->page_pool, &offset, sz,
> > +				    (in_interrupt() ? GFP_ATOMIC :
> GFP_KERNEL) |
> 
> in_interrupt() should not be used in drivers, AFAIR.
Okay. 

> Pass the correct flags from the caller (or don't -- it seems like the only caller
> assumes softirq context already).
I agree. Will Remove GFP_KERNEL.

> 
> > +				    GFP_DMA);
> 
> GFP_DMA? Why?
No need. Will Remove. 

> 
> > +	if (unlikely(!page)) {
> > +		netdev_err(pfvf->netdev, "Allocation of page pool
> failed\n");
> 
> No prints on allocation errors, please, it only adds stress to the system. You
> can add a statistic if you want.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	*dma = page_pool_get_dma_addr(page) + offset;
> > +	return 0;
> > +}
> 
> > +	pp_params.flags = PP_FLAG_PAGE_FRAG | PP_FLAG_DMA_MAP;
> > +	pp_params.pool_size = numptrs;
> > +	pp_params.nid = NUMA_NO_NODE;
> > +	pp_params.dev = pfvf->dev;
> > +	pp_params.dma_dir = DMA_FROM_DEVICE;
> > +	pool->page_pool = page_pool_create(&pp_params);
> > +	if (!pool->page_pool) {
> > +		netdev_err(pfvf->netdev, "Creation of page pool failed\n");
> > +		return -EFAULT;
> 
> EFAULT == "Bad address", doesn't sound right
Agree. Will fix to return correct err code. 

> 
> > +	}
> > +
> >  	return 0;
> >  }
> --
> pw-bot: cr