Messages in this thread | | | From | Ratheesh Kannoth <> | Subject | RE: Re: Re: [PATCH net-next] octeontx2-pf: Add support for page pool | Date | Tue, 16 May 2023 09:21:38 +0000 |
| |
> -----Original Message----- > From: Paolo Abeni <pabeni@redhat.com> > Sent: Tuesday, May 16, 2023 2:21 PM > To: Ratheesh Kannoth <rkannoth@marvell.com>; Yunsheng Lin > <linyunsheng@huawei.com>; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: Sunil Kovvuri Goutham <sgoutham@marvell.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org > Subject: Re: Re: [PATCH net-next] octeontx2-pf: Add support for page > pool > > > > > .... > > > > @@ -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. > > Uhm... the above looks like a questionable choice, as page pull is a small infra, > and the performance delta should be relevant. > Okay. Will select PAGE_POOL in config.
> Anyway if you really want to use such strategy, please be consistent and > guard any relevant chunck of code with compiler guards. Likely it would be > better providing dummy helpers for the few page_pool functions still missing > them when !CONFIG_PAGE_POOL
As page pool brings in performance improvement and is selected by default. I am thinking, not to implement dummy functions.
> > > > Cheers, > > Paolo
| |