Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]

> On 2024/2/16 17:25, Lorenzo Bianconi wrote:
> > Use global percpu page_pool_recycle_stats counter for system page_pool
> > allocator instead of allocating a separate percpu variable for each
> > (also percpu) page pool instance.
> 
> I may missed some obvious discussion in previous version due to spring
> holiday.
> 
> > 
[...]
> >  #ifdef CONFIG_PAGE_POOL_STATS
> > -	pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > -	if (!pool->recycle_stats)
> > -		return -ENOMEM;
> > +	if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
> > +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > +		if (!pool->recycle_stats)
> > +			return -ENOMEM;
> > +	} else {
> > +		/* For system page pool instance we use a singular stats object
> > +		 * instead of allocating a separate percpu variable for each
> > +		 * (also percpu) page pool instance.
> > +		 */
> > +		pool->recycle_stats = &pp_system_recycle_stats;
> 
> Do we need to return -EINVAL here if page_pool_init() is called with
> pool->p.flags & PP_FLAG_SYSTEM_POOL being true and cpuid being a valid
> cpu?
> If yes, it seems we may be able to use the cpuid to decide if we need
> to allocate a new pool->recycle_stats without adding a new flag.

for the current use-cases cpuid is set to a valid core id just for system
page_pool but in the future probably there will not be a 1:1 relation (e.g.
we would want to pin a per-cpu page_pool instance to a particular CPU?)

> 
> If no, the API for page_pool_create_percpu() seems a litte weird as it
> relies on the user calling it correctly.
> 
> Also, do we need to enforce that page_pool_create_percpu() is only called
> once for the same cpu? if no, we may have two page_pool instance sharing
> the same stats.

do you mean for the same pp instance? I guess it is not allowed by the APIs.

Regards,
Lorenzo

> 
> > +	}
> >  #endif
> >  
> >  	if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) {
> >  #ifdef CONFIG_PAGE_POOL_STATS
> > -		free_percpu(pool->recycle_stats);
> > +		if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> > +			free_percpu(pool->recycle_stats);
> >  #endif
> >  		return -ENOMEM;
> >  	}
> > @@ -251,7 +262,8 @@ static void page_pool_uninit(struct page_pool *pool)
> >  		put_device(pool->p.dev);
> >  
> >  #ifdef CONFIG_PAGE_POOL_STATS
> > -	free_percpu(pool->recycle_stats);
> > +	if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> > +		free_percpu(pool->recycle_stats);
> >  #endif
> >  }
> >  
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]