netdev - Re: [PATCH net-next v2 1/3] net: skb: plumb napi state thru skb freeing paths

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: <f2d47b7a-40fe-d366-e1a2-a81842e43acf@huawei.com>
Date:   Fri, 14 Apr 2023 17:40:57 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Eric Dumazet <edumazet@...gle.com>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <pabeni@...hat.com>, <hawk@...nel.org>,
        <ilias.apalodimas@...aro.org>, <alexander.duyck@...il.com>,
        Tariq Toukan <tariqt@...dia.com>
Subject: Re: [PATCH net-next v2 1/3] net: skb: plumb napi state thru skb
 freeing paths

On 2023/4/14 13:20, Jakub Kicinski wrote:
> On Fri, 14 Apr 2023 08:57:03 +0800 Yunsheng Lin wrote:
>>>> Does it break the single-producer single-consumer assumption of tx queue?  
>>>
>>> We do not think so.  
>>
>> Then I guess it is ok to do direct recycling for page pool case as it is
>> per napi?
> 
> We're talking about the tx queue or the pp cache?

> Those have different producers and consumers.

I was trying to confirm if there are more than one contexts that will call
napi->poll() concurrently, if no, then it seems we only have one consumer
for pp cache. And it does not really matter here too when napi->poll() from
netpoll only do tx completion now.

If we are able to limit the context of producer for pp cache to be
the same as consumer, then we might avoid the !!bugget checking.

I do ack that it is a bigger change considering when we might need to
hold a persisent and safe reference to the NAPI instance for this too,
so we might leave it for now like you mentioned in the cover letter.

> 
>> It is per cpu cache case we are using !!bugget to protect it from preemption
>> while netpoll_poll_dev() is running?
> 
> This is the scenario - We are feeding the page pool cache from the
> deferred pages. An IRQ comes and interrupts us half way thru. 
> netpoll then tries to also feed the page pool cache. Unhappiness.

I assume feeding the page pool cache means producer for pp cache?
If netpoll only do tx completion by using zero bugget, it does not
seem to have any direct relation with page pool yet.

> 
> Note that netpoll is activated extremely rarely (only when something
> writes to the console), and even more rarely does it actually poll
> for space in the Tx queue.

That is my point, we turn out to need a checking in the fast path in order
to handle the netpoll case, which is a extreme rare case.

> .
>