[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-netdev
Subject:    Re: [PATCH v5 bpf-next 2/2] net: xdp: introduce xdp_prepare_buff utility routine
From:       Daniel Borkmann <daniel () iogearbox ! net>
Date:       2020-12-30 23:16:41
Message-ID: b76a6fc5-55fa-1ca8-f2b9-ae0332450333 () iogearbox ! net
[Download RAW message or body]

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!
[prev in list] [next in list] [prev in thread] [next in thread]