Re: [PATCH net-next v6 2/2] net: add netmem to skb_frag_t

[Paolo Abeni <pabeni@redhat.com>]

Hi,

I'm sorry for the late feedback.

On Tue, 2024-01-23 at 14:17 -0800, Mina Almasry wrote:
> @@ -845,16 +863,24 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
>  }
>  EXPORT_SYMBOL(__napi_alloc_skb);
>  
> -void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> -		     int size, unsigned int truesize)
> +void skb_add_rx_frag_netmem(struct sk_buff *skb, int i, netmem_ref netmem,
> +			    int off, int size, unsigned int truesize)
>  {
>  	DEBUG_NET_WARN_ON_ONCE(size > truesize);
>  
> -	skb_fill_page_desc(skb, i, page, off, size);
> +	skb_fill_netmem_desc(skb, i, netmem, off, size);
>  	skb->len += size;
>  	skb->data_len += size;
>  	skb->truesize += truesize;
>  }
> +EXPORT_SYMBOL(skb_add_rx_frag_netmem);
> +
> +void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> +		     int size, unsigned int truesize)
> +{
> +	skb_add_rx_frag_netmem(skb, i, page_to_netmem(page), off, size,
> +			       truesize);
> +}
>  EXPORT_SYMBOL(skb_add_rx_frag);

Out of sheer ignorance, I'm unsure if the compiler will always inline
the above skb_add_rx_frag_netmem() call. What about moving this helper
to the header file?

> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 1184d40167b8..145ef22b2b35 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -636,9 +636,14 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
>  		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
>  			msize += skb_frag_size(&skb_shinfo(skb)->frags[i]);
>  
> +		if (WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

I feel like the following has been already discussed, but I could not
find the relevant reference... Are all frags constrained to carry the
same memref type? If not it would be better to move this check inside
the previous loop, it's already traversing all the skb frags, it should
not add measurable overhead.

Thanks!

Paolo