On Tue, 15 Dec 2020 16:13:44 +0100
Maciej Fijalkowski [off-list ref] wrote:

On Tue, Dec 15, 2020 at 04:06:20PM +0100, Lorenzo Bianconi wrote:

quoted

quoted

On 12/15/20 2:47 PM, Lorenzo Bianconi wrote:
[...]  

quoted

quoted

quoted

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
  

quoted

quoted

quoted

quoted

+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?  

That's what I was trying to say previously.

quoted

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;  

This will introduce branch, so for intel drivers we're getting the
overhead of one add and a branch. I'm still opting for a separate helper.

I should think, as this gets inlined the compiler should be able to
remove the branch.  I assume that the usage of 'meta_valid' will be a
const in the drivers.  Maybe we should have the API be 'const bool meta_valid'?

static __always_inline void
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_set_data_meta_invalid(xdp);
}

static __always_inline void
xdp_prepare_buff_meta(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;
}

Thanks to you Maciej for reviewing this! :-)

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