Re: [PATCH v2 bpf-next 1/8] net: Refactor xdp_convert_buff_to_frame

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Sat, 20 Jun 2020 00:57:17 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> From: David Ahern <dahern@digitalocean.com>
> 
> Move the guts of xdp_convert_buff_to_frame to a new helper,
> xdp_update_frame_from_buff so it can be reused removing code duplication
> 
> Suggested-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: David Ahern <dahern@digitalocean.com>
> ---
>  include/net/xdp.h | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 609f819ed08b..ab1c503808a4 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -121,39 +121,48 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp)
>  	xdp->frame_sz = frame->frame_sz;
>  }
>  
> -/* Convert xdp_buff to xdp_frame */
>  static inline
> -struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
> +int xdp_update_frame_from_buff(struct xdp_buff *xdp,
> +			       struct xdp_frame *xdp_frame)
>  {
> -	struct xdp_frame *xdp_frame;
> -	int metasize;
> -	int headroom;
> -
> -	if (xdp->rxq->mem.type == MEM_TYPE_XSK_BUFF_POOL)
> -		return xdp_convert_zc_to_xdp_frame(xdp);
> +	int metasize, headroom;
>  
>  	/* Assure headroom is available for storing info */
>  	headroom = xdp->data - xdp->data_hard_start;
>  	metasize = xdp->data - xdp->data_meta;
>  	metasize = metasize > 0 ? metasize : 0;
>  	if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
> -		return NULL;
> +		return -ENOMEM;

IMHO I think ENOMEM is reserved for memory allocations failures.
I think ENOSPC will be more appropriate here (or EOVERFLOW).

>  
>  	/* Catch if driver didn't reserve tailroom for skb_shared_info */
>  	if (unlikely(xdp->data_end > xdp_data_hard_end(xdp))) {
>  		XDP_WARN("Driver BUG: missing reserved tailroom");
> -		return NULL;
> +		return -ENOMEM;

Same here.

>  	}
>  
> -	/* Store info in top of packet */
> -	xdp_frame = xdp->data_hard_start;
> -
>  	xdp_frame->data = xdp->data;
>  	xdp_frame->len  = xdp->data_end - xdp->data;
>  	xdp_frame->headroom = headroom - sizeof(*xdp_frame);
>  	xdp_frame->metasize = metasize;
>  	xdp_frame->frame_sz = xdp->frame_sz;
>  
> +	return 0;
> +}
> +
> +/* Convert xdp_buff to xdp_frame */
> +static inline
> +struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
> +{
> +	struct xdp_frame *xdp_frame;
> +
> +	if (xdp->rxq->mem.type == MEM_TYPE_XSK_BUFF_POOL)
> +		return xdp_convert_zc_to_xdp_frame(xdp);
> +
> +	/* Store info in top of packet */
> +	xdp_frame = xdp->data_hard_start;
> +	if (unlikely(xdp_update_frame_from_buff(xdp, xdp_frame) < 0))
> +		return NULL;
> +
>  	/* rxq only valid until napi_schedule ends, convert to xdp_mem_info */
>  	xdp_frame->mem = xdp->rxq->mem;
>  



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer