Re: [PATCH net-next] net: page_pool: optimize page pool page allocation in NUMA scenario

[wangjie (L)]

On 2022/6/27 17:50, Jesper Dangaard Brouer wrote:
> On 24/06/2022 11.36, Guangbin Huang wrote:
> > From: Jie Wang <wangjie125@xxxxxxxxxx>
> >
> > Currently NIC packet receiving performance based on page pool
> > deteriorates
> > occasionally. To analysis the causes of this problem page allocation
> > stats
> > are collected. Here are the stats when NIC rx performance deteriorates:
> >
> > bandwidth(Gbits/s)        16.8        6.91
> > rx_pp_alloc_fast        13794308    21141869
> > rx_pp_alloc_slow        108625        166481
> > rx_pp_alloc_slow_h        0        0
> > rx_pp_alloc_empty        8192        8192
> > rx_pp_alloc_refill        0        0
> > rx_pp_alloc_waive        100433        158289
> > rx_pp_recycle_cached        0        0
> > rx_pp_recycle_cache_full    0        0
> > rx_pp_recycle_ring        362400        420281
> > rx_pp_recycle_ring_full        6064893        9709724
> > rx_pp_recycle_released_ref    0        0
> >
> > The rx_pp_alloc_waive count indicates that a large number of pages' numa
> > node are inconsistent with the NIC device numa node. Therefore these
> > pages
> > can't be reused by the page pool. As a result, many new pages would be
> > allocated by __page_pool_alloc_pages_slow which is time consuming. This
> > causes the NIC rx performance fluctuations.
> >
> > The main reason of huge numa mismatch pages in page pool is that page
> > pool
> > uses alloc_pages_bulk_array to allocate original pages. This function is
> > not suitable for page allocation in NUMA scenario. So this patch uses
> > alloc_pages_bulk_array_node which has a NUMA id input parameter to ensure
> > the NUMA consistent between NIC device and allocated pages.
> >
> > Repeated NIC rx performance tests are performed 40 times. NIC rx
> > bandwidth
> > is higher and more stable compared to the datas above. Here are three
> > test
> > stats, the rx_pp_alloc_waive count is zero and rx_pp_alloc_slow which
> > indicates pages allocated from slow patch is relatively low.
> >
> > bandwidth(Gbits/s)        93        93.9        93.8
> > rx_pp_alloc_fast        60066264    61266386    60938254
> > rx_pp_alloc_slow        16512        16517        16539
> > rx_pp_alloc_slow_ho        0        0        0
> > rx_pp_alloc_empty        16512        16517        16539
> > rx_pp_alloc_refill        473841        481910        481585
> > rx_pp_alloc_waive        0        0        0
> > rx_pp_recycle_cached        0        0        0
> > rx_pp_recycle_cache_full    0        0        0
> > rx_pp_recycle_ring        29754145    30358243    30194023
> > rx_pp_recycle_ring_full        0        0        0
> > rx_pp_recycle_released_ref    0        0        0
> >
> > Signed-off-by: Jie Wang <wangjie125@xxxxxxxxxx>
> > ---
> >   net/core/page_pool.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
>
> Thanks for improving this, but we need some small adjustments below.
> And then you need to send a V2 of the patch.
>
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index f18e6e771993..15997fcd78f3 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -377,6 +377,7 @@ static struct page
> > *__page_pool_alloc_pages_slow(struct page_pool *pool,
> >       unsigned int pp_order = pool->p.order;
> >       struct page *page;
> >       int i, nr_pages;
> > +    int pref_nid; /* preferred NUMA node */
> >         /* Don't support bulk alloc for high-order pages */
> >       if (unlikely(pp_order))
> > @@ -386,10 +387,18 @@ static struct page
> > *__page_pool_alloc_pages_slow(struct page_pool *pool,
> >       if (unlikely(pool->alloc.count > 0))
> >           return pool->alloc.cache[--pool->alloc.count];
> >   +#ifdef CONFIG_NUMA
> > +    pref_nid = (pool->p.nid == NUMA_NO_NODE) ? numa_mem_id() :
> > pool->p.nid;
> > +#else
> > +    /* Ignore pool->p.nid setting if !CONFIG_NUMA, helps compiler */
>
> Remove "helps compiler" from comments, it only make sense in the code
> this was copy-pasted from.
>
>
> > +    pref_nid = numa_mem_id(); /* will be zero like page_to_nid() */
>
> The comment about "page_to_nid()" is only relevant in the code
> this was copy-pasted from.
>
> Change to:
>     pref_nid = NUMA_NO_NODE;
>
> As alloc_pages_bulk_array_node() will be inlined, the effect (generated
> asm code) will be the same, but it will be better for code maintenance.
OK，thanks for your review, I will fix it in next version.
> > +#endif
> > +
> >       /* Mark empty alloc.cache slots "empty" for
> > alloc_pages_bulk_array */
> >       memset(&pool->alloc.cache, 0, sizeof(void *) * bulk);
> >   -    nr_pages = alloc_pages_bulk_array(gfp, bulk, pool->alloc.cache);
> > +    nr_pages = alloc_pages_bulk_array_node(gfp, pref_nid, bulk,
> > +                           pool->alloc.cache);
> >       if (unlikely(!nr_pages))
> >           return NULL;
>
>
> .