netdev - Re: [PATCH v5 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]

Date:   Mon, 18 Jan 2021 15:56:35 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        BPF-dev-list <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Jesper Brouer <brouer@...hat.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Saeed Mahameed <saeed@...nel.org>
Subject: Re: [PATCH v5 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff
 utility routine

On Thu, Dec 31, 2020 at 12:16:41AM +0100, Daniel Borkmann wrote:
> On 12/29/20 7:09 PM, Lorenzo Bianconi wrote:
> > > > +                     hard_start = page_address(rx_buffer->page) +
> > > > +                                  rx_buffer->page_offset - offset;
> > > > +                     xdp_prepare_buff(&xdp, hard_start, offset, size, true);
> > > >    #if (PAGE_SIZE > 4096)
> > > >                        /* At larger PAGE_SIZE, frame_sz depend on len size */
> > > >                        xdp.frame_sz = ixgbevf_rx_frame_truesize(rx_ring, size);
> > 
> > Hi Daniel,
> > 
> > thx for the review.
> > 
> > > [...]
> > > The design is very similar for most of the Intel drivers. Why the inconsistency on
> > > ice driver compared to the rest, what's the rationale there to do it in one but not
> > > the others? Generated code better there?
> > 
> > I applied the same logic for the ice driver but the code is just
> > slightly different.
> > 
> > > Couldn't you even move the 'unsigned int offset = xyz_rx_offset(rx_ring)' out of the
> > > while loop altogether for all of them? (You already use the xyz_rx_offset() implicitly
> > > for most of them when setting xdp.frame_sz.)
> > 
> > We discussed moving "offset = xyz_rx_offset(rx_ring)" out of the while
> > loop before but Saeed asked to address it in a dedicated series since
> > it is a little bit out of the scope. I have no strong opinion on it,
> > do you prefer to address it directly here?
> 
> Fair enough, I might have preferred it in this series as part of the overall cleanup,
> but if you plan to follow up on this then this is also fine by me. Applied the v5 to
> bpf-next in that case, thanks!

I initially pointed out the fact that we could store the output of
xyz_rx_offset(rx_ring) onto a variable rather than call it per each
processed buffer because value returned by that func can not change
throughout the napi execution. It is based on ethtool priv flag which if
changed, resets the PF (so disables napi, frees irqs, loads different Rx
mem model, etc).

I realised that there is yet another place where we have unnecessary call
to xyz_rx_offset() in hot path which is xyz_alloc_mapped_page(), so I
expanded the idea of this optimization and I store the offset directly
onto Rx ring and refer to that value.

I am including the patches that do what described above onto pending
series of fixes that I had back in december '20 I suppose.