> 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.
>
> 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;
> }yes, to follow-up the possible approaches we have here are: - have 2 different helpers (xdp_prepare_buff_meta and xdp_prepare_buff) as suggested by Maciej - move the data_meta initialization out of the helper and do it in each driver - use the current approach and overwrite data_meta with xdp_set_data_meta_invalid() when necessary - introduce a branch in order to have just one helper what is the best for you? Regards, Lorenzo > > > } > > > > Regards, > > Lorenzo > > > > > > > > Thanks, > > > Daniel > > > > >
signature.asc
Description: PGP signature
