netdev - Re: [PATCH v5 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device

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: <3DD728CA-CF0B-4F26-AF64-4E1C357D0F0C@gmail.com>
Date:   Wed, 20 Nov 2019 10:42:47 -0800
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Jesper Dangaard Brouer" <brouer@...hat.com>
Cc:     "Lorenzo Bianconi" <lorenzo@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, ilias.apalodimas@...aro.org,
        lorenzo.bianconi@...hat.com, mcroce@...hat.com
Subject: Re: [PATCH v5 net-next 2/3] net: page_pool: add the possibility to
 sync DMA memory for device



On 20 Nov 2019, at 9:49, Jesper Dangaard Brouer wrote:

> On Wed, 20 Nov 2019 16:54:18 +0200
> Lorenzo Bianconi <lorenzo@...nel.org> wrote:
>
>> Introduce the following parameters in order to add the possibility to 
>> sync
>> DMA memory for device before putting allocated pages in the page_pool
>> caches:
>> - PP_FLAG_DMA_SYNC_DEV: if set in page_pool_params flags, all pages 
>> that
>>   the driver gets from page_pool will be DMA-synced-for-device 
>> according
>>   to the length provided by the device driver. Please note 
>> DMA-sync-for-CPU
>>   is still device driver responsibility
>> - offset: DMA address offset where the DMA engine starts copying rx 
>> data
>> - max_len: maximum DMA memory size page_pool is allowed to flush. 
>> This
>>   is currently used in __page_pool_alloc_pages_slow routine when 
>> pages
>>   are allocated from page allocator
>> These parameters are supposed to be set by device drivers.
>>
>> This optimization reduces the length of the DMA-sync-for-device.
>> The optimization is valid because pages are initially
>> DMA-synced-for-device as defined via max_len. At RX time, the driver
>> will perform a DMA-sync-for-CPU on the memory for the packet length.
>> What is important is the memory occupied by packet payload, because
>> this is the area CPU is allowed to read and modify. As we don't track
>> cache-lines written into by the CPU, simply use the packet payload 
>> length
>> as dma_sync_size at page_pool recycle time. This also take into 
>> account
>> any tail-extend.
>>
>> Tested-by: Matteo Croce <mcroce@...hat.com>
>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>> ---
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>
> [...]
>> @@ -281,8 +309,8 @@ static bool __page_pool_recycle_direct(struct 
>> page *page,
>>  	return true;
>>  }
>>
>> -void __page_pool_put_page(struct page_pool *pool,
>> -			  struct page *page, bool allow_direct)
>> +void __page_pool_put_page(struct page_pool *pool, struct page *page,
>> +			  unsigned int dma_sync_size, bool allow_direct)
>>  {
>>  	/* This allocator is optimized for the XDP mode that uses
>>  	 * one-frame-per-page, but have fallbacks that act like the
>> @@ -293,6 +321,10 @@ void __page_pool_put_page(struct page_pool 
>> *pool,
>>  	if (likely(page_ref_count(page) == 1)) {
>>  		/* Read barrier done in page_ref_count / READ_ONCE */
>>
>> +		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
>> +			page_pool_dma_sync_for_device(pool, page,
>> +						      dma_sync_size);
>> +
>>  		if (allow_direct && in_serving_softirq())
>>  			if (__page_pool_recycle_direct(page, pool))
>>  				return;
>
> I am slightly concerned this touch the fast-path code. But at-least on
> Intel, I don't think this is measurable.  And for the ARM64 board it
> was a huge win... thus I'll accept this.

For the next series:

The "in_serving_softirq()" check shows up on profiling.  I'd
like to remove this and just have a "direct" flag, where the
caller takes the responsibility of the correct context.
-- 
Jonathan