* [PATCH v2] octeontx2-pf: Use the napi_alloc_frag() to alloc the pool buffers
@ 2020-05-08 11:49 Kevin Hao
2020-05-09 0:38 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Hao @ 2020-05-08 11:49 UTC (permalink / raw)
To: netdev
Cc: Sunil Goutham, Geetha sowjanya, Subbaraya Sundeep, hariprasad,
davem, Sunil Kovvuri
In the current codes, the octeontx2 uses its own method to allocate
the pool buffers, but there are some issues in this implementation.
1. We have to run the otx2_get_page() for each allocation cycle and
this is pretty error prone. As I can see there is no invocation
of the otx2_get_page() in otx2_pool_refill_task(), this will leave
the allocated pages have the wrong refcount and may be freed wrongly.
2. It wastes memory. For example, if we only receive one packet in a
NAPI RX cycle, and then allocate a 2K buffer with otx2_alloc_rbuf()
to refill the pool buffers and leave the remain area of the allocated
page wasted. On a kernel with 64K page, 62K area is wasted.
IMHO it is really unnecessary to implement our own method for the
buffers allocate, we can reuse the napi_alloc_frag() to simplify
our code.
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: Use DMA_ATTR_SKIP_CPU_SYNC while mapping the buffer as indicated by
Sunil Kovvuri.
.../marvell/octeontx2/nic/otx2_common.c | 52 ++++++++-----------
.../marvell/octeontx2/nic/otx2_common.h | 15 +-----
.../marvell/octeontx2/nic/otx2_txrx.c | 3 +-
.../marvell/octeontx2/nic/otx2_txrx.h | 4 --
4 files changed, 23 insertions(+), 51 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index f1d2dea90a8c..612d33207326 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -379,40 +379,33 @@ void otx2_config_irq_coalescing(struct otx2_nic *pfvf, int qidx)
(pfvf->hw.cq_ecount_wait - 1));
}
-dma_addr_t otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
- gfp_t gfp)
+dma_addr_t _otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool)
{
dma_addr_t iova;
+ u8 *buf;
- /* Check if request can be accommodated in previous allocated page */
- if (pool->page && ((pool->page_offset + pool->rbsize) <=
- (PAGE_SIZE << pool->rbpage_order))) {
- pool->pageref++;
- goto ret;
- }
-
- otx2_get_page(pool);
-
- /* Allocate a new page */
- pool->page = alloc_pages(gfp | __GFP_COMP | __GFP_NOWARN,
- pool->rbpage_order);
- if (unlikely(!pool->page))
+ buf = napi_alloc_frag(pool->rbsize);
+ if (unlikely(!buf))
return -ENOMEM;
- pool->page_offset = 0;
-ret:
- iova = (u64)otx2_dma_map_page(pfvf, pool->page, pool->page_offset,
- pool->rbsize, DMA_FROM_DEVICE);
- if (!iova) {
- if (!pool->page_offset)
- __free_pages(pool->page, pool->rbpage_order);
- pool->page = NULL;
+ iova = dma_map_single_attrs(pfvf->dev, buf, pool->rbsize,
+ DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ if (unlikely(dma_mapping_error(pfvf->dev, iova)))
return -ENOMEM;
- }
- pool->page_offset += pool->rbsize;
+
return iova;
}
+static dma_addr_t otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool)
+{
+ dma_addr_t addr;
+
+ local_bh_disable();
+ addr = _otx2_alloc_rbuf(pfvf, pool);
+ local_bh_enable();
+ return addr;
+}
+
void otx2_tx_timeout(struct net_device *netdev, unsigned int txq)
{
struct otx2_nic *pfvf = netdev_priv(netdev);
@@ -805,7 +798,7 @@ static void otx2_pool_refill_task(struct work_struct *work)
free_ptrs = cq->pool_ptrs;
while (cq->pool_ptrs) {
- bufptr = otx2_alloc_rbuf(pfvf, rbpool, GFP_KERNEL);
+ bufptr = otx2_alloc_rbuf(pfvf, rbpool);
if (bufptr <= 0) {
/* Schedule a WQ if we fails to free atleast half of the
* pointers else enable napi for this RQ.
@@ -1064,7 +1057,6 @@ static int otx2_pool_init(struct otx2_nic *pfvf, u16 pool_id,
return err;
pool->rbsize = buf_size;
- pool->rbpage_order = get_order(buf_size);
/* Initialize this pool's context via AF */
aq = otx2_mbox_alloc_msg_npa_aq_enq(&pfvf->mbox);
@@ -1152,13 +1144,12 @@ int otx2_sq_aura_pool_init(struct otx2_nic *pfvf)
return -ENOMEM;
for (ptr = 0; ptr < num_sqbs; ptr++) {
- bufptr = otx2_alloc_rbuf(pfvf, pool, GFP_KERNEL);
+ bufptr = otx2_alloc_rbuf(pfvf, pool);
if (bufptr <= 0)
return bufptr;
otx2_aura_freeptr(pfvf, pool_id, bufptr);
sq->sqb_ptrs[sq->sqb_count++] = (u64)bufptr;
}
- otx2_get_page(pool);
}
return 0;
@@ -1204,13 +1195,12 @@ int otx2_rq_aura_pool_init(struct otx2_nic *pfvf)
for (pool_id = 0; pool_id < hw->rqpool_cnt; pool_id++) {
pool = &pfvf->qset.pool[pool_id];
for (ptr = 0; ptr < num_ptrs; ptr++) {
- bufptr = otx2_alloc_rbuf(pfvf, pool, GFP_KERNEL);
+ bufptr = otx2_alloc_rbuf(pfvf, pool);
if (bufptr <= 0)
return bufptr;
otx2_aura_freeptr(pfvf, pool_id,
bufptr + OTX2_HEAD_ROOM);
}
- otx2_get_page(pool);
}
return 0;
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
index 0b1c653b3449..5d806252efd0 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
@@ -434,18 +434,6 @@ static inline void otx2_aura_freeptr(struct otx2_nic *pfvf,
otx2_get_regaddr(pfvf, NPA_LF_AURA_OP_FREE0));
}
-/* Update page ref count */
-static inline void otx2_get_page(struct otx2_pool *pool)
-{
- if (!pool->page)
- return;
-
- if (pool->pageref)
- page_ref_add(pool->page, pool->pageref);
- pool->pageref = 0;
- pool->page = NULL;
-}
-
static inline int otx2_get_pool_idx(struct otx2_nic *pfvf, int type, int idx)
{
if (type == AURA_NIX_SQ)
@@ -589,8 +577,7 @@ int otx2_txschq_config(struct otx2_nic *pfvf, int lvl);
int otx2_txsch_alloc(struct otx2_nic *pfvf);
int otx2_txschq_stop(struct otx2_nic *pfvf);
void otx2_sqb_flush(struct otx2_nic *pfvf);
-dma_addr_t otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
- gfp_t gfp);
+dma_addr_t _otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool);
int otx2_rxtx_enable(struct otx2_nic *pfvf, bool enable);
void otx2_ctx_disable(struct mbox *mbox, int type, bool npa);
int otx2_nix_config_bp(struct otx2_nic *pfvf, bool enable);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 45abe0cd0e7b..62e95c6f38eb 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -286,7 +286,7 @@ static int otx2_rx_napi_handler(struct otx2_nic *pfvf,
/* Refill pool with new buffers */
while (cq->pool_ptrs) {
- bufptr = otx2_alloc_rbuf(pfvf, cq->rbpool, GFP_ATOMIC);
+ bufptr = _otx2_alloc_rbuf(pfvf, cq->rbpool);
if (unlikely(bufptr <= 0)) {
struct refill_work *work;
struct delayed_work *dwork;
@@ -304,7 +304,6 @@ static int otx2_rx_napi_handler(struct otx2_nic *pfvf,
otx2_aura_freeptr(pfvf, cq->cq_idx, bufptr + OTX2_HEAD_ROOM);
cq->pool_ptrs--;
}
- otx2_get_page(cq->rbpool);
return processed_cqe;
}
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
index 4ab32d3adb78..da97f2d4416f 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
@@ -113,11 +113,7 @@ struct otx2_cq_poll {
struct otx2_pool {
struct qmem *stack;
struct qmem *fc_addr;
- u8 rbpage_order;
u16 rbsize;
- u32 page_offset;
- u16 pageref;
- struct page *page;
};
struct otx2_cq_queue {
--
2.26.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] octeontx2-pf: Use the napi_alloc_frag() to alloc the pool buffers
2020-05-08 11:49 [PATCH v2] octeontx2-pf: Use the napi_alloc_frag() to alloc the pool buffers Kevin Hao
@ 2020-05-09 0:38 ` Jakub Kicinski
2020-05-09 0:53 ` Kevin Hao
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2020-05-09 0:38 UTC (permalink / raw)
To: Kevin Hao
Cc: netdev, Sunil Goutham, Geetha sowjanya, Subbaraya Sundeep,
hariprasad, davem, Sunil Kovvuri
On Fri, 8 May 2020 19:49:53 +0800 Kevin Hao wrote:
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index f1d2dea90a8c..612d33207326 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -379,40 +379,33 @@ void otx2_config_irq_coalescing(struct otx2_nic *pfvf, int qidx)
> (pfvf->hw.cq_ecount_wait - 1));
> }
>
> -dma_addr_t otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
> - gfp_t gfp)
> +dma_addr_t _otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool)
If you need to respin please use double underscore as a prefix, it's
a far more common style in the kernel.
> {
> dma_addr_t iova;
> + u8 *buf;
>
> - /* Check if request can be accommodated in previous allocated page */
> - if (pool->page && ((pool->page_offset + pool->rbsize) <=
> - (PAGE_SIZE << pool->rbpage_order))) {
> - pool->pageref++;
> - goto ret;
> - }
> -
> - otx2_get_page(pool);
> -
> - /* Allocate a new page */
> - pool->page = alloc_pages(gfp | __GFP_COMP | __GFP_NOWARN,
> - pool->rbpage_order);
> - if (unlikely(!pool->page))
> + buf = napi_alloc_frag(pool->rbsize);
> + if (unlikely(!buf))
> return -ENOMEM;
>
> - pool->page_offset = 0;
> -ret:
> - iova = (u64)otx2_dma_map_page(pfvf, pool->page, pool->page_offset,
> - pool->rbsize, DMA_FROM_DEVICE);
> - if (!iova) {
> - if (!pool->page_offset)
> - __free_pages(pool->page, pool->rbpage_order);
> - pool->page = NULL;
> + iova = dma_map_single_attrs(pfvf->dev, buf, pool->rbsize,
> + DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + if (unlikely(dma_mapping_error(pfvf->dev, iova)))
Thanks for doing this, but aren't you leaking the buf on DMA mapping
error?
> return -ENOMEM;
> - }
> - pool->page_offset += pool->rbsize;
> +
> return iova;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] octeontx2-pf: Use the napi_alloc_frag() to alloc the pool buffers
2020-05-09 0:38 ` Jakub Kicinski
@ 2020-05-09 0:53 ` Kevin Hao
0 siblings, 0 replies; 3+ messages in thread
From: Kevin Hao @ 2020-05-09 0:53 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Sunil Goutham, Geetha sowjanya, Subbaraya Sundeep,
hariprasad, davem, Sunil Kovvuri
[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]
On Fri, May 08, 2020 at 05:38:33PM -0700, Jakub Kicinski wrote:
> On Fri, 8 May 2020 19:49:53 +0800 Kevin Hao wrote:
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > index f1d2dea90a8c..612d33207326 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > @@ -379,40 +379,33 @@ void otx2_config_irq_coalescing(struct otx2_nic *pfvf, int qidx)
> > (pfvf->hw.cq_ecount_wait - 1));
> > }
> >
> > -dma_addr_t otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
> > - gfp_t gfp)
> > +dma_addr_t _otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool)
>
> If you need to respin please use double underscore as a prefix, it's
> a far more common style in the kernel.
Sure.
>
> > {
> > dma_addr_t iova;
> > + u8 *buf;
> >
> > - /* Check if request can be accommodated in previous allocated page */
> > - if (pool->page && ((pool->page_offset + pool->rbsize) <=
> > - (PAGE_SIZE << pool->rbpage_order))) {
> > - pool->pageref++;
> > - goto ret;
> > - }
> > -
> > - otx2_get_page(pool);
> > -
> > - /* Allocate a new page */
> > - pool->page = alloc_pages(gfp | __GFP_COMP | __GFP_NOWARN,
> > - pool->rbpage_order);
> > - if (unlikely(!pool->page))
> > + buf = napi_alloc_frag(pool->rbsize);
> > + if (unlikely(!buf))
> > return -ENOMEM;
> >
> > - pool->page_offset = 0;
> > -ret:
> > - iova = (u64)otx2_dma_map_page(pfvf, pool->page, pool->page_offset,
> > - pool->rbsize, DMA_FROM_DEVICE);
> > - if (!iova) {
> > - if (!pool->page_offset)
> > - __free_pages(pool->page, pool->rbpage_order);
> > - pool->page = NULL;
> > + iova = dma_map_single_attrs(pfvf->dev, buf, pool->rbsize,
> > + DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> > + if (unlikely(dma_mapping_error(pfvf->dev, iova)))
>
> Thanks for doing this, but aren't you leaking the buf on DMA mapping
> error?
Ouch, I missed that. Will fix.
Thanks,
Kevin
>
> > return -ENOMEM;
> > - }
> > - pool->page_offset += pool->rbsize;
> > +
> > return iova;
> > }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-09 0:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 11:49 [PATCH v2] octeontx2-pf: Use the napi_alloc_frag() to alloc the pool buffers Kevin Hao
2020-05-09 0:38 ` Jakub Kicinski
2020-05-09 0:53 ` Kevin Hao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.