Re: [PATCH net-next 1/2] page_pool: check for PP direct cache locality later

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/3/30 0:55, Alexander Lobakin wrote:
> Since we have pool->p.napi (Jakub) and pool->cpuid (Lorenzo) to check
> whether it's safe to use direct recycling, we can use both globally for
> each page instead of relying solely on @allow_direct argument.
> Let's assume that @allow_direct means "I'm sure it's local, don't waste
> time rechecking this" and when it's false, try the mentioned params to
> still recycle the page directly. If neither is true, we'll lose some
> CPU cycles, but then it surely won't be hotpath. On the other hand,
> paths where it's possible to use direct cache, but not possible to
> safely set @allow_direct, will benefit from this move.
> The whole propagation of @napi_safe through a dozen of skb freeing
> functions can now go away, which saves us some stack space.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  include/linux/skbuff.h | 12 ++++----
>  net/core/page_pool.c   | 31 +++++++++++++++++--
>  net/core/skbuff.c      | 70 +++++++++++++-----------------------------
>  net/ipv4/esp4.c        |  2 +-
>  net/ipv6/esp6.c        |  2 +-
>  5 files changed, 58 insertions(+), 59 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index dadd3f55d549..f7f6e42c6814 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3515,25 +3515,25 @@ int skb_pp_cow_data(struct page_pool *pool, struct sk_buff **pskb,
>  		    unsigned int headroom);
>  int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
>  			 struct bpf_prog *prog);
> -bool napi_pp_put_page(struct page *page, bool napi_safe);
> +bool napi_pp_put_page(struct page *page);
>  
>  static inline void
> -skb_page_unref(const struct sk_buff *skb, struct page *page, bool napi_safe)
> +skb_page_unref(const struct sk_buff *skb, struct page *page)
>  {
>  #ifdef CONFIG_PAGE_POOL
> -	if (skb->pp_recycle && napi_pp_put_page(page, napi_safe))
> +	if (skb->pp_recycle && napi_pp_put_page(page))
>  		return;
>  #endif
>  	put_page(page);
>  }
>  
>  static inline void
> -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> +napi_frag_unref(skb_frag_t *frag, bool recycle)
>  {
>  	struct page *page = skb_frag_page(frag);
>  
>  #ifdef CONFIG_PAGE_POOL
> -	if (recycle && napi_pp_put_page(page, napi_safe))
> +	if (recycle && napi_pp_put_page(page))
>  		return;
>  #endif
>  	put_page(page);
> @@ -3549,7 +3549,7 @@ napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
>   */
>  static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
>  {
> -	napi_frag_unref(frag, recycle, false);
> +	napi_frag_unref(frag, recycle);
>  }
>  
>  /**
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index dd364d738c00..9d56257e444b 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -690,8 +690,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>  			page_pool_dma_sync_for_device(pool, page,
>  						      dma_sync_size);
>  
> -		if (allow_direct && in_softirq() &&
> -		    page_pool_recycle_in_cache(page, pool))
> +		if (allow_direct && page_pool_recycle_in_cache(page, pool))
>  			return NULL;
>  
>  		/* Page found as candidate for recycling */
> @@ -716,9 +715,35 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>  	return NULL;
>  }
>  
> +static bool page_pool_napi_local(const struct page_pool *pool)
> +{
> +	const struct napi_struct *napi;
> +	u32 cpuid;
> +
> +	if (unlikely(!in_softirq()))
> +		return false;
> +
> +	/* Allow direct recycle if we have reasons to believe that we are
> +	 * in the same context as the consumer would run, so there's
> +	 * no possible race.
> +	 * __page_pool_put_page() makes sure we're not in hardirq context
> +	 * and interrupts are enabled prior to accessing the cache.
> +	 */
> +	cpuid = smp_processor_id();
> +	if (READ_ONCE(pool->cpuid) == cpuid)
> +		return true;
> +
> +	napi = READ_ONCE(pool->p.napi);
> +
> +	return napi && READ_ONCE(napi->list_owner) == cpuid;
> +}
> +
>  void page_pool_put_unrefed_page(struct page_pool *pool, struct page *page,
>  				unsigned int dma_sync_size, bool allow_direct)
>  {
> +	if (!allow_direct)

It seems we are changing some semantics here, in_softirq() is checked
even if allow_direct is true before this patch. And it seems in_softirq()
is not checked if allow_direct is true after this patch? I think we might
need some assertion to ensure @allow_direct really means "I'm sure it's
local, don't waste time rechecking this". As my understanding, it is really
hard to debug this kind of problem, so in_softirq() is always checking.

Perhaps add something like WARN_ONCE() or DEBUG_NET_WARN_ON_ONCE for
allow_direct being true case to catch the API misuse?

> +		allow_direct = page_pool_napi_local(pool);
> +
>  	page = __page_pool_put_page(pool, page, dma_sync_size, allow_direct);
>  	if (page && !page_pool_recycle_in_ring(pool, page)) {
>  		/* Cache full, fallback to free pages */
> @@ -969,7 +994,7 @@ void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>  static void page_pool_disable_direct_recycling(struct page_pool *pool)
>  {
>  	/* Disable direct recycling based on pool->cpuid.
> -	 * Paired with READ_ONCE() in napi_pp_put_page().
> +	 * Paired with READ_ONCE() in page_pool_napi_local().
>  	 */
>  	WRITE_ONCE(pool->cpuid, -1);
>