netdev - Re: [RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr

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: <bf8450d2-ced4-e59d-f811-0f970ac4cbb1@mellanox.com>
Date:   Wed, 13 Feb 2019 08:46:58 +0000
From:   Tariq Toukan <tariqt@...lanox.com>
To:     Alexander Duyck <alexander.duyck@...il.com>,
        Eric Dumazet <eric.dumazet@...il.com>
CC:     Tariq Toukan <tariqt@...lanox.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Matthew Wilcox <willy@...radead.org>,
        "brouer@...hat.com" <brouer@...hat.com>,
        David Miller <davem@...emloft.net>,
        "toke@...hat.com" <toke@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "mgorman@...hsingularity.net" <mgorman@...hsingularity.net>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [RFC, PATCH] net: page_pool: Don't use page->private to store
 dma_addr_t



On 2/12/2019 8:13 PM, Alexander Duyck wrote:
> On Tue, Feb 12, 2019 at 7:16 AM Eric Dumazet <eric.dumazet@...il.com> wrote:
>>
>>
>>
>> On 02/12/2019 04:39 AM, Tariq Toukan wrote:
>>>
>>>
>>> On 2/11/2019 7:14 PM, Eric Dumazet wrote:
>>>>
>>>>
>>>> On 02/11/2019 12:53 AM, Tariq Toukan wrote:
>>>>>
>>>>
>>>>> Hi,
>>>>>
>>>>> It's great to use the struct page to store its dma mapping, but I am
>>>>> worried about extensibility.
>>>>> page_pool is evolving, and it would need several more per-page fields.
>>>>> One of them would be pageref_bias, a planned optimization to reduce the
>>>>> number of the costly atomic pageref operations (and replace existing
>>>>> code in several drivers).
>>>>>
>>>>
>>>> But the point about pageref_bias is to place it in a different cache line than "struct page"
>>>>
>>>> The major cost is having a cache line bouncing between producer and consumer.
>>>>
>>>
>>> pageref_bias is meant to be dirtied only by the page requester, i.e. the
>>> NIC driver / page_pool.
>>> All other components (basically, SKB release flow / put_page) should
>>> continue working with the atomic page_refcnt, and not dirty the
>>> pageref_bias.
>>
>> This is exactly my point.
>>
>> You suggested to put pageref_bias in struct page, which breaks this completely.
>>
>> pageref_bias is better kept in a driver structure, with appropriate prefetching
>> since most NIC use a ring buffer for their queues.
>>
>> The dma address _can_ be put in the struct page, since the driver does not dirty it
>> and does not even read it when page can be recycled.
> 
> Instead of maintaining the pageref_bias in the page itself it could be
> maintained in some sort of separate structure. You could just maintain
> a pointer to a slot in an array somewhere. Then you can still access
> it if needed, the pointer would be static for as long as it is in the
> page pool, and you could invalidate the pointer prior to removing the
> bias from the page.
> 

Hi Alex,

That's right.
But as I describe in my other reply yesterday, there is a more serious 
issue with combining the pageref_bias feature with the new page_pool 
capability for elevated refcount pages.
It won't work on top, and that's fine, as the idea of the new elevated 
refcount queue capability is more promising and important, and it wins here.

Regards,
Tariq