netdev - Re: [PATCH bpf-next] net: xdp: introduce xdp_init

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] [day] [month] [year] [list]

Message-ID: <20201210212431.GD462213@lore-desk>
Date:   Thu, 10 Dec 2020 22:24:31 +0100
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        bpf@...r.kernel.org, netdev@...r.kernel.org, davem@...emloft.net,
        kuba@...nel.org, ast@...nel.org, daniel@...earbox.net,
        brouer@...hat.com, alexander.duyck@...il.com
Subject: Re: [PATCH bpf-next] net: xdp: introduce xdp_init_buff utility
 routine

> On Thu, 2020-12-10 at 20:28 +0100, Lorenzo Bianconi wrote:
> > > On Thu, 2020-12-10 at 18:59 +0100, Lorenzo Bianconi wrote:
> > > > On Dec 10, Maciej Fijalkowski wrote:
> > > > > On Thu, Dec 10, 2020 at 05:32:41PM +0100, Lorenzo Bianconi
> > > > > wrote:
> > > > > > > On Thu, Dec 10, 2020 at 04:50:42PM +0100, Lorenzo Bianconi
> > > > > > > wrote:
> > > > > > > > Introduce xdp_init_buff utility routine to initialize
> > > > > > > > xdp_buff data
> > > > > > > > structure. Rely on xdp_init_buff in all XDP capable
> > > > > > > > drivers.
> > > > > > > 
> > > > > > > Hm, Jesper was suggesting two helpers, one that you
> > > > > > > implemented
> > > > > > > for things
> > > > > > > that are set once per NAPI and the other that is set per
> > > > > > > each
> > > > > > > buffer.
> > > > > > > 
> > > > > > > Not sure about the naming for a second one -
> > > > > > > xdp_prepare_buff ?
> > > > > > > xdp_init_buff that you have feels ok.
> > > > > > 
> > > > > > ack, so we can have xdp_init_buff() for initialization done
> > > > > > once
> > > > > > per NAPI run and 
> > > > > > xdp_prepare_buff() for per-NAPI iteration initialization,
> > > > > > e.g.
> > > > > > 
> > > > > > static inline void
> > > > > > xdp_prepare_buff(struct xdp_buff *xdp, unsigned char
> > > > > > *hard_start,
> > > > > > 		 int headroom, int data_len)
> > > > > > {
> > > > > > 	xdp->data_hard_start = hard_start;
> > > > > > 	xdp->data = hard_start + headroom;
> > > > > > 	xdp->data_end = xdp->data + data_len;
> > > > > > 	xdp_set_data_meta_invalid(xdp);
> > > > > > }
> > > > > 
> > > > > I think we should allow for setting the data_meta as well.
> > > > > x64 calling convention states that first four args are placed
> > > > > onto
> > > > > registers, so to keep it fast maybe have a third helper:
> > > > > 
> > > > > static inline void
> > > > > xdp_prepare_buff_meta(struct xdp_buff *xdp, unsigned char
> > > > > *hard_start,
> > > > > 		      int headroom, int data_len)
> > > > > {
> > > > > 	xdp->data_hard_start = hard_start;
> > > > > 	xdp->data = hard_start + headroom;
> > > > > 	xdp->data_end = xdp->data + data_len;
> > > > > 	xdp->data_meta = xdp->data;
> > > > > }
> > > > > 
> > > > > Thoughts?
> > > > 
> > > > ack, I am fine with it. Let's wait for some feedback.
> > > > 
> > > > Do you prefer to have xdp_prepare_buff/xdp_prepare_buff_meta in
> > > > the
> > > > same series
> > > > of xdp_buff_init() or is it ok to address it in a separate patch?
> > > > 
> > > 
> > > you only need 2
> > > why do you need xpd_prepare_buff_meta? that's exactly
> > > what xdp_set_data_meta_invalid(xdp) is all about.
> > 
> > IIUC what Maciej means is to avoid to overwrite xdp->data_meta with
> > xdp_set_data_meta_invalid() after setting it to xdp->data in
> > xdp_prepare_buff_meta().
> > I guess setting xdp->data_meta to xdp->data is valid, it means an
> > empty meta
> > area.
> > Anyway I guess we can set xdp->data_meta to xdp->data wherever we
> > need and just
> > keep xdp_prepare_buff(). Agree?
> > 
> 
> hmm, i agree, but I would choose a default that is best for common use
> case performance, so maybe do xd->data_meta = xdp->data by default and
> drivers can override it, as they are already doing today if they don't
> support it.

ack, fine. I will fix int v2.

Regards,
Lorenzo

> 
> > Regards,
> > Lorenzo
> > 
> > > 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)