| 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
| ||
|
Message-ID: <MWHPR1801MB191862CDE18A98A89E57E3D9D3799@MWHPR1801MB1918.namprd18.prod.outlook.com>
Date: Tue, 16 May 2023 03:36:45 +0000
From: Ratheesh Kannoth <rkannoth@...vell.com>
To: Yunsheng Lin <linyunsheng@...wei.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: Sunil Kovvuri Goutham <sgoutham@...vell.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>
Subject: RE: Re: [PATCH net-next] octeontx2-pf: Add support for page pool
> -----Original Message-----
> From: Yunsheng Lin <linyunsheng@...wei.com>
> Sent: Tuesday, May 16, 2023 6:44 AM
> To: Ratheesh Kannoth <rkannoth@...vell.com>; netdev@...r.kernel.org;
> linux-kernel@...r.kernel.org
> Cc: Sunil Kovvuri Goutham <sgoutham@...vell.com>;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com
> Subject: [EXT] Re: [PATCH net-next] octeontx2-pf: Add support for page pool
>
> ----------------------------------------------------------------------
> On 2023/5/15 13:56, 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.
>
> Any performance data to share here?
Performance improvement of 39Mpps when measured with Linux packet generator.
Will update the same in commit message.
> ....
> > @@ -1170,15 +1199,24 @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf,
> int type)
> > /* Free SQB and RQB pointers from the aura pool */
> > for (pool_id = pool_start; pool_id < pool_end; pool_id++) {
> > iova = otx2_aura_allocptr(pfvf, pool_id);
> > + pool = &pfvf->qset.pool[pool_id];
> > while (iova) {
> > if (type == AURA_NIX_RQ)
> > iova -= OTX2_HEAD_ROOM;
> >
> > pa = otx2_iova_to_phys(pfvf->iommu_domain,
> iova);
> > - dma_unmap_page_attrs(pfvf->dev, iova, size,
> > - DMA_FROM_DEVICE,
> > - DMA_ATTR_SKIP_CPU_SYNC);
> > - put_page(virt_to_page(phys_to_virt(pa)));
> > + page = virt_to_page(phys_to_virt(pa));
>
> virt_to_page() seems ok for order-0 page allocated from page pool as it does
> now, but it may break for order-1+ page as
> page_pool_put_page() expects head page of compound page or base page.
> Maybe add a comment for that or use virt_to_head_page() explicitly.
Thanks !!.
>
> > +
> > + if (pool->page_pool) {
> > + page_pool_put_page(pool->page_pool,
> page, size, true);
>
> page_pool_put_full_page() seems more appropriate here, as the
> PP_FLAG_DMA_SYNC_DEV flag is not set, even if it is set, it seems the whole
> page need to be synced instead of a frag.
Agree.
>
>
> > + } else {
> > + dma_unmap_page_attrs(pfvf->dev, iova,
> size,
> > + DMA_FROM_DEVICE,
> > +
> DMA_ATTR_SKIP_CPU_SYNC);
> > +
> > + put_page(page);
> > + }
> > +
> > iova = otx2_aura_allocptr(pfvf, pool_id);
> > }
> > }
> > @@ -1196,6 +1234,8 @@ void otx2_aura_pool_free(struct otx2_nic *pfvf)
> > pool = &pfvf->qset.pool[pool_id];
> > qmem_free(pfvf->dev, pool->stack);
> > qmem_free(pfvf->dev, pool->fc_addr);
> > + page_pool_destroy(pool->page_pool);
> > + pool->page_pool = NULL;
> > }
> > devm_kfree(pfvf->dev, pfvf->qset.pool);
> > pfvf->qset.pool = NULL;
> > @@ -1279,8 +1319,10 @@ static int otx2_aura_init(struct otx2_nic
> > *pfvf, int aura_id, }
> >
> > static int otx2_pool_init(struct otx2_nic *pfvf, u16 pool_id,
> > - int stack_pages, int numptrs, int buf_size)
> > + int stack_pages, int numptrs, int buf_size,
> > + int type)
> > {
> > + struct page_pool_params pp_params = { 0 };
> > struct npa_aq_enq_req *aq;
> > struct otx2_pool *pool;
> > int err;
> > @@ -1324,6 +1366,22 @@ static int otx2_pool_init(struct otx2_nic *pfvf,
> u16 pool_id,
> > aq->ctype = NPA_AQ_CTYPE_POOL;
> > aq->op = NPA_AQ_INSTOP_INIT;
> >
> > + if (type != AURA_NIX_RQ) {
> > + pool->page_pool = NULL;
> > + 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;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1358,7 +1416,7 @@ int otx2_sq_aura_pool_init(struct otx2_nic
> > *pfvf)
> >
> > /* Initialize pool context */
> > err = otx2_pool_init(pfvf, pool_id, stack_pages,
> > - num_sqbs, hw->sqb_size);
> > + num_sqbs, hw->sqb_size, AURA_NIX_SQ);
> > if (err)
> > goto fail;
> > }
> > @@ -1421,7 +1479,7 @@ int otx2_rq_aura_pool_init(struct otx2_nic *pfvf)
> > }
> > for (pool_id = 0; pool_id < hw->rqpool_cnt; pool_id++) {
> > err = otx2_pool_init(pfvf, pool_id, stack_pages,
> > - num_ptrs, pfvf->rbsize);
> > + num_ptrs, pfvf->rbsize, AURA_NIX_RQ);
> > if (err)
> > goto fail;
> > }
>
> ...
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > index 7045fedfd73a..df5f45aa6980 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > @@ -217,9 +217,10 @@ static bool otx2_skb_add_frag(struct otx2_nic
> *pfvf, struct sk_buff *skb,
> > skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
> > va - page_address(page) + off,
> > len - off, pfvf->rbsize);
> > -
> > +#ifndef CONFIG_PAGE_POOL
>
> Most driver does 'select PAGE_POOL' in config when adding page pool
> support, is there any reason it does not do it here?
We thought about it. User should be able to use the driver without PAGE_POOL support.
>
> > otx2_dma_unmap_page(pfvf, iova - OTX2_HEAD_ROOM,
> > pfvf->rbsize, DMA_FROM_DEVICE);
> > +#endif
> > return true;
> > }
> >
> > @@ -382,6 +383,8 @@ static void otx2_rcv_pkt_handler(struct otx2_nic
> *pfvf,
> > if (pfvf->netdev->features & NETIF_F_RXCSUM)
> > skb->ip_summed = CHECKSUM_UNNECESSARY;
> >
> > + skb_mark_for_recycle(skb);
> > +
> > napi_gro_frags(napi);
> > }
> >
> > @@ -1180,11 +1183,14 @@ bool otx2_sq_append_skb(struct net_device
> > *netdev, struct otx2_snd_queue *sq, }
> > EXPORT_SYMBOL(otx2_sq_append_skb);
> >
> > -void otx2_cleanup_rx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue
> > *cq)
> > +void otx2_cleanup_rx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue
> > +*cq, int qidx)
> > {
> > struct nix_cqe_rx_s *cqe;
> > int processed_cqe = 0;
> > + struct otx2_pool *pool;
> > + struct page *page;
> > u64 iova, pa;
> > + u16 pool_id;
> >
> > if (pfvf->xdp_prog)
> > xdp_rxq_info_unreg(&cq->xdp_rxq);
> > @@ -1192,6 +1198,9 @@ void otx2_cleanup_rx_cqes(struct otx2_nic *pfvf,
> struct otx2_cq_queue *cq)
> > if (otx2_nix_cq_op_status(pfvf, cq) || !cq->pend_cqe)
> > return;
> >
> > + pool_id = otx2_get_pool_idx(pfvf, AURA_NIX_RQ, qidx);
> > + pool = &pfvf->qset.pool[pool_id];
> > +
> > while (cq->pend_cqe) {
> > cqe = (struct nix_cqe_rx_s *)otx2_get_next_cqe(cq);
> > processed_cqe++;
> > @@ -1205,8 +1214,14 @@ void otx2_cleanup_rx_cqes(struct otx2_nic
> *pfvf, struct otx2_cq_queue *cq)
> > }
> > iova = cqe->sg.seg_addr - OTX2_HEAD_ROOM;
> > pa = otx2_iova_to_phys(pfvf->iommu_domain, iova);
> > - otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
> DMA_FROM_DEVICE);
> > - put_page(virt_to_page(phys_to_virt(pa)));
> > + page = virt_to_page(phys_to_virt(pa));
> > +
> > + if (pool->page_pool) {
> > + page_pool_put_page(pool->page_pool, page, pfvf-
> >rbsize, true);
> > + } else {
> > + otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
> DMA_FROM_DEVICE);
> > + put_page(page);
> > + }
>
> Maybe add a helper for the above as there is a similiar code block in the
> otx2_free_aura_ptr()
Ok.
>
>
> >
Powered by blists - more mailing lists