On Tue, Oct 8, 2024 at 12:54 PM Taehee Yoo <ap420073@xxxxxxxxx> wrote:
>
> On Tue, Oct 8, 2024 at 11:45 AM David Wei <dw@xxxxxxxxxxx> wrote:
> >
>
> Hi David,
> Thanks a lot for your review!
>
> > On 2024-10-03 09:06, Taehee Yoo wrote:
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > index 872b15842b11..64e07d247f97 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > @@ -55,6 +55,7 @@
> > > #include <net/page_pool/helpers.h>
> > > #include <linux/align.h>
> > > #include <net/netdev_queues.h>
> > > +#include <net/netdev_rx_queue.h>
> > >
> > > #include "bnxt_hsi.h"
> > > #include "bnxt.h"
> > > @@ -863,6 +864,22 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
> > > bnapi->events &= ~BNXT_TX_CMP_EVENT;
> > > }
> > >
> > > +static netmem_ref __bnxt_alloc_rx_netmem(struct bnxt *bp, dma_addr_t *mapping,
> > > + struct bnxt_rx_ring_info *rxr,
> > > + unsigned int *offset,
> > > + gfp_t gfp)
> >
> > gfp is unused
>
> I will remove unnecessary gfp parameter in v4.
Oh sorry,
I will use gfp parameter, not remove it.
>
> >
> > > +{
> > > + netmem_ref netmem;
> > > +
> > > + netmem = page_pool_alloc_netmem(rxr->page_pool, GFP_ATOMIC);
> > > + if (!netmem)
> > > + return 0;
> > > + *offset = 0;
> > > +
> > > + *mapping = page_pool_get_dma_addr_netmem(netmem) + *offset;
> >
> > offset is always 0
>
> Okay, I will remove this too in v4.
>
> >
> > > + return netmem;
> > > +}
> > > +
> > > static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,
> > > struct bnxt_rx_ring_info *rxr,
> > > unsigned int *offset,
> >
> > [...]
> >
> > > @@ -1192,6 +1209,7 @@ static struct sk_buff *bnxt_rx_skb(struct bnxt *bp,
> > >
> > > static u32 __bnxt_rx_agg_pages(struct bnxt *bp,
> > > struct bnxt_cp_ring_info *cpr,
> > > + struct sk_buff *skb,
> > > struct skb_shared_info *shinfo,
> > > u16 idx, u32 agg_bufs, bool tpa,
> > > struct xdp_buff *xdp)
> > > @@ -1211,7 +1229,7 @@ static u32 __bnxt_rx_agg_pages(struct bnxt *bp,
> > > u16 cons, frag_len;
> > > struct rx_agg_cmp *agg;
> > > struct bnxt_sw_rx_agg_bd *cons_rx_buf;
> > > - struct page *page;
> > > + netmem_ref netmem;
> > > dma_addr_t mapping;
> > >
> > > if (p5_tpa)
> > > @@ -1223,9 +1241,15 @@ static u32 __bnxt_rx_agg_pages(struct bnxt *bp,
> > > RX_AGG_CMP_LEN) >> RX_AGG_CMP_LEN_SHIFT;
> > >
> > > cons_rx_buf = &rxr->rx_agg_ring[cons];
> > > - skb_frag_fill_page_desc(frag, cons_rx_buf->page,
> > > - cons_rx_buf->offset, frag_len);
> > > - shinfo->nr_frags = i + 1;
> > > + if (skb) {
> > > + skb_add_rx_frag_netmem(skb, i, cons_rx_buf->netmem,
> > > + cons_rx_buf->offset, frag_len,
> > > + BNXT_RX_PAGE_SIZE);
> > > + } else {
> > > + skb_frag_fill_page_desc(frag, netmem_to_page(cons_rx_buf->netmem),
> > > + cons_rx_buf->offset, frag_len);
> > > + shinfo->nr_frags = i + 1;
> > > + }
> >
> > I feel like this function needs a refactor at some point to split out
> > the skb and xdp paths.
>
> Okay, I will add __bnxt_rx_agg_netmem() in v4 patch.
>
> >
> > > __clear_bit(cons, rxr->rx_agg_bmap);
> > >
> > > /* It is possible for bnxt_alloc_rx_page() to allocate
> >
> > [...]
> >
> > > @@ -3608,9 +3629,11 @@ static void bnxt_free_rx_rings(struct bnxt *bp)
> > >
> > > static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
> > > struct bnxt_rx_ring_info *rxr,
> > > + int queue_idx,
> >
> > To save a parameter, the index is available already in rxr->bnapi->index
>
> Okay, I also remove the queue_idx parameter in v4.
>
> >
> > > int numa_node)
> > > {
> > > struct page_pool_params pp = { 0 };
> > > + struct netdev_rx_queue *rxq;
> > >
> > > pp.pool_size = bp->rx_agg_ring_size;
> > > if (BNXT_RX_PAGE_MODE(bp))
> > > @@ -3621,8 +3644,15 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
> > > pp.dev = &bp->pdev->dev;
> > > pp.dma_dir = bp->rx_dir;
> > > pp.max_len = PAGE_SIZE;
> > > - pp.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
> > > + pp.order = 0;
> > > +
> > > + rxq = __netif_get_rx_queue(bp->dev, queue_idx);
> > > + if (rxq->mp_params.mp_priv)
> > > + pp.flags = PP_FLAG_DMA_MAP | PP_FLAG_ALLOW_UNREADABLE_NETMEM;
> > > + else
> > > + pp.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
> > >
> > > + pp.queue_idx = queue_idx;
> > > rxr->page_pool = page_pool_create(&pp);
> > > if (IS_ERR(rxr->page_pool)) {
> > > int err = PTR_ERR(rxr->page_pool);
> > > @@ -3655,7 +3685,7 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > > cpu_node = cpu_to_node(cpu);
> > > netdev_dbg(bp->dev, "Allocating page pool for rx_ring[%d] on numa_node: %d\n",
> > > i, cpu_node);
> > > - rc = bnxt_alloc_rx_page_pool(bp, rxr, cpu_node);
> > > + rc = bnxt_alloc_rx_page_pool(bp, rxr, i, cpu_node);
> > > if (rc)
> > > return rc;
> > >
>
> Thanks a lot for catching things,
> I will send v4 if there is no problem after some tests.
>
> Thanks!
> Taehee Yoo
