netdev - Re: [PATCH net-next v2 5/6] tsnep: Add RX queue info for XDP support

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

Message-ID: <82b5acef-6d2f-6631-326e-6ec12b2257ec@engleder-embedded.com>
Date:   Fri, 9 Dec 2022 09:11:53 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        kuba@...nel.org, edumazet@...gle.com, pabeni@...hat.com,
        ast@...nel.org, daniel@...earbox.net, hawk@...nel.org,
        john.fastabend@...il.com
Subject: Re: [PATCH net-next v2 5/6] tsnep: Add RX queue info for XDP support

On 09.12.22 01:53, Saeed Mahameed wrote:
> On 08 Dec 21:32, Gerhard Engleder wrote:
>> On 08.12.22 13:59, Maciej Fijalkowski wrote:
>>> On Thu, Dec 08, 2022 at 06:40:44AM +0100, Gerhard Engleder wrote:
>>>> Register xdp_rxq_info with page_pool memory model. This is needed for
>>>> XDP buffer handling.
>>>>
>>>> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
>>>> ---
>>>>  drivers/net/ethernet/engleder/tsnep.h      |  5 ++--
>>>>  drivers/net/ethernet/engleder/tsnep_main.c | 34 +++++++++++++++++-----
>>>>  2 files changed, 30 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/engleder/tsnep.h 
>>>> b/drivers/net/ethernet/engleder/tsnep.h
>>>> index 0e7fc36a64e1..70bc133d4a9d 100644
>>>> --- a/drivers/net/ethernet/engleder/tsnep.h
>>>> +++ b/drivers/net/ethernet/engleder/tsnep.h
>>>> @@ -127,6 +127,7 @@ struct tsnep_rx {
>>>>      u32 owner_counter;
>>>>      int increment_owner_counter;
>>>>      struct page_pool *page_pool;
>>>> +    struct xdp_rxq_info xdp_rxq;
>>>
>>> this occupies full cacheline, did you make sure that you don't break
>>> tsnep_rx layout with having xdp_rxq_info in the middle of the way?
>>
>> Actually I did no cacheline optimisation for this structure so far.
>> I saw that igb/igc put xdp_rxq_info to the end. Is this best practice
>> to prevent other variables in the same cacheline of xdp_rxq?
> 
> a rule of thumb, organize the structure in the same order they are
> being accessed in the data path.. but this doesn't go without saying you
> need to do some layout testing via pahole for example..
> It's up to you and the maintainer of this driver to decide how critical 
> this
> is.
> 
> Reviewed-by: Saeed Mahameed <saeed@...nel.org>

Thanks for the clarification, I will think about it.

I wrote the driver and I'm responsible to keep it working. Is this equal
to being the maintainer?

Thanks for the review!

Gerhard