Re: [RFC PATCH v2 11/21] core/skbuff: add page recycling logic for netgpu pages

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jul 27, 2020 at 03:44:34PM -0700, Jonathan Lemon wrote:
> From: Jonathan Lemon <bsd@fb.com>
> 
> netgpu pages will always have a refcount of at least one (held by
> the netgpu module).  If the skb is marked as containing netgpu ZC
> pages, recycle them back to netgpu.

What???

> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
>  net/core/skbuff.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1422b99b7090..50dbb7ce1965 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -591,6 +591,27 @@ static void skb_free_head(struct sk_buff *skb)
>  		kfree(head);
>  }
>  
> +#if IS_ENABLED(CONFIG_NETGPU)
> +static void skb_netgpu_unref(struct skb_shared_info *shinfo)
> +{
> +	struct netgpu_ifq *ifq = shinfo->destructor_arg;
> +	struct page *page;
> +	int i;
> +
> +	/* pages attached for skbs for TX shouldn't come here, since
> +	 * the skb is not marked as "zc_netgpu". (only RX skbs have this).
> +	 * dummy page does come here, but always has elevated refc.
> +	 *
> +	 * Undelivered zc skb's will arrive at this point.
> +	 */
> +	for (i = 0; i < shinfo->nr_frags; i++) {
> +		page = skb_frag_page(&shinfo->frags[i]);
> +		if (page && page_ref_dec_return(page) <= 2)
> +			netgpu_put_page(ifq, page, false);
> +	}
> +}
> +#endif

Becides the basic "no #if in C files" issue here, why is this correct?

> +
>  static void skb_release_data(struct sk_buff *skb)
>  {
>  	struct skb_shared_info *shinfo = skb_shinfo(skb);
> @@ -601,8 +622,15 @@ static void skb_release_data(struct sk_buff *skb)
>  			      &shinfo->dataref))
>  		return;
>  
> -	for (i = 0; i < shinfo->nr_frags; i++)
> -		__skb_frag_unref(&shinfo->frags[i]);
> +#if IS_ENABLED(CONFIG_NETGPU)
> +	if (skb->zc_netgpu && shinfo->nr_frags) {
> +		skb_netgpu_unref(shinfo);
> +	} else
> +#endif
> +	{
> +		for (i = 0; i < shinfo->nr_frags; i++)
> +			__skb_frag_unref(&shinfo->frags[i]);
> +	}

Again, no #if in C code.  But even then, this feels really really wrong.

greg k-h