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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: 
 <MWHPR1801MB1918540ACD82ECEF1F7D92ACD37F9@MWHPR1801MB1918.namprd18.prod.outlook.com>
Date: Thu, 18 May 2023 05:27:27 +0000
From: Ratheesh Kannoth <rkannoth@...vell.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sunil Kovvuri
 Goutham <sgoutham@...vell.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com"
	<pabeni@...hat.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Srujana Challa
	<schalla@...vell.com>,
        Hariprasad Kelam <hkelam@...vell.com>
Subject: RE: Re: [PATH net-next v1] octeontx2-pf: Add support for page pool

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, May 18, 2023 9:17 AM
> To: Ratheesh Kannoth <rkannoth@...vell.com>
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@...vell.com>; davem@...emloft.net;
> edumazet@...gle.com; pabeni@...hat.com; Subbaraya Sundeep Bhatta
> <sbhatta@...vell.com>; Geethasowjanya Akula <gakula@...vell.com>;
> Srujana Challa <schalla@...vell.com>; Hariprasad Kelam
> <hkelam@...vell.com>
> 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@...vell.com>
> >
> 
> 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