netdev - Re: [PATCH v3 bpf-next 2/2] net: xdp: introduce xdp_prepare

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201216093015.3a0b78e2@carbon>
Date:   Wed, 16 Dec 2020 09:30:15 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, alexander.duyck@...il.com, saeed@...nel.org,
        brouer@...hat.com
Subject: Re: [PATCH v3 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff
 utility routine

On Tue, 15 Dec 2020 16:13:44 +0100
Maciej Fijalkowski <maciej.fijalkowski@...el.com> wrote:

> On Tue, Dec 15, 2020 at 04:06:20PM +0100, Lorenzo Bianconi wrote:
> > > 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?  
> 
> That's what I was trying to say previously.
> 
> > 
> > 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