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

Message-ID: <4d9e2dd071314f73136d77912cb9cc40d4557c80.camel@kernel.org>
Date:   Thu, 17 Dec 2020 12:31:09 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, alexander.duyck@...il.com
Subject: Re: [PATCH v3 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff
 utility routine

On Thu, 2020-12-17 at 19:28 +0100, Maciej Fijalkowski wrote:
> On Thu, Dec 17, 2020 at 10:16:06AM -0800, Saeed Mahameed wrote:
> > On Wed, 2020-12-16 at 16:01 +0100, Lorenzo Bianconi wrote:
> > > > On Tue, 15 Dec 2020 14:47:10 +0100
> > > > Lorenzo Bianconi <lorenzo.bianconi@...hat.com> wrote:
> > > > 
> > > > > [...]
> > > > > > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > > > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > > > > > b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > > > > > index 4dbbbd49c389..fcd1ca3343fb 100644
> > > > > > > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > > > > > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > > > > > @@ -2393,12 +2393,12 @@ static int
> > > > > > > i40e_clean_rx_irq(struct
> > > > > > > i40e_ring *rx_ring, int budget)
> > > > > > >  
> > > > > > >  		/* retrieve a buffer from the ring */
> > > > > > >  		if (!skb) {
> > > > > > > -			xdp.data = page_address(rx_buffer-
> > > > > > > > page) +
> > > > > > > -				   rx_buffer->page_offset;
> > > > > > > -			xdp.data_meta = xdp.data;
> > > > > > > -			xdp.data_hard_start = xdp.data -
> > > > > > > -					      i40e_rx_offset(rx
> > > > > > > _ring);
> > > > > > > -			xdp.data_end = xdp.data + size;
> > > > > > > +			unsigned int offset =
> > > > > > > i40e_rx_offset(rx_ring);  
> > > > > > 
> > > > > > I now see that we could call the i40e_rx_offset() once per
> > > > > > napi, so can
> > > > > > you pull this variable out and have it initialized a single
> > > > > > time? Applies
> > > > > > to other intel drivers as well.  
> > 
> > How is this related to this series? i suggest to keep this series
> > clean
> > of vendor specific unrelated optimizations, this must be done in a
> > separate patchset.
> 
> Well, Lorenzo explicitly is touching the thing that I referred to, so
> I
> just ask if he can optimize it while he's at it.
> 
> Of course I'm fine with addressing this by myself once -next opens :)
> 
Oh, don't get me wrong I am ok with doing this now, and i can do it my
self if you want :), but it shouldn't be part of the this series, so we
won't confuse others who want to implement XDP in the future, that's
all.