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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 31 Dec 2020 00:16:41 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
Cc:     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>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Saeed Mahameed <saeed@...nel.org>
Subject: Re: [PATCH v5 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff
 utility routine

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!