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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZdCR6i85oEvoxMzF@lore-desk>
Date: Sat, 17 Feb 2024 12:00:58 +0100
From: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
	kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, hawk@...nel.org, ilias.apalodimas@...aro.org,
	toke@...hat.com, aleksander.lobakin@...el.com
Subject: Re: [PATCH net-next] net: page_pool: fix recycle stats for system
 page_pool allocator

> 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
> >  }
> >  
> > 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)