Re: [PATCH v3 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff utility routine

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

> On 12/15/20 2:47 PM, Lorenzo Bianconi wrote:
> [...]
> > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > > > index 329397c60d84..61d3f5f8b7f3 100644
> > > > --- a/drivers/net/xen-netfront.c
> > > > +++ b/drivers/net/xen-netfront.c
> > > > @@ -866,10 +866,8 @@ static u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata,
> > > >   	xdp_init_buff(xdp, XEN_PAGE_SIZE - XDP_PACKET_HEADROOM,
> > > >   		      &queue->xdp_rxq);
> > > > -	xdp->data_hard_start = page_address(pdata);
> > > > -	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> > > > +	xdp_prepare_buff(xdp, page_address(pdata), XDP_PACKET_HEADROOM, len);
> > > >   	xdp_set_data_meta_invalid(xdp);
> > > > -	xdp->data_end = xdp->data + len;
> > > >   	act = bpf_prog_run_xdp(prog, xdp);
> > > >   	switch (act) {
> > > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > > index 3fb3a9aa1b71..66d8a4b317a3 100644
> > > > --- a/include/net/xdp.h
> > > > +++ b/include/net/xdp.h
> > > > @@ -83,6 +83,18 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
> > > >   	xdp->rxq = rxq;
> > > >   }
> > > > +static inline void
> 
> nit: maybe __always_inline

ack, I will add in v4

> 
> > > > +xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
> > > > +		 int headroom, int data_len)
> > > > +{
> > > > +	unsigned char *data = hard_start + headroom;
> > > > +
> > > > +	xdp->data_hard_start = hard_start;
> > > > +	xdp->data = data;
> > > > +	xdp->data_end = data + data_len;
> > > > +	xdp->data_meta = data;
> > > > +}
> > > > +
> > > >   /* Reserve memory area at end-of data area.
> > > >    *
> 
> For the drivers with xdp_set_data_meta_invalid(), we're basically setting xdp->data_meta
> twice unless compiler is smart enough to optimize the first one away (did you double check?).
> Given this is supposed to be a cleanup, why not integrate this logic as well so the
> xdp_set_data_meta_invalid() doesn't get extra treatment?

we discussed it before, but I am fine to add it in v4. Something like:

static __always_inline void
xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
		 int headroom, int data_len, bool meta_valid)
{
	unsigned char *data = hard_start + headroom;
	
	xdp->data_hard_start = hard_start;
	xdp->data = data;
	xdp->data_end = data + data_len;
	xdp->data_meta = meta_valid ? data : data + 1;
}

Regards,
Lorenzo

> 
> Thanks,
> Daniel
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]