Received-SPF: pass (google.com: domain of linux-kernel+bounces-156579-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <81df4e0f-07f3-40f6-8d71-ffad791ab611@intel.com>
Date: Wed, 24 Apr 2024 10:52:49 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v4 6/7] page_pool: check for DMA sync shortcut
 earlier
To: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>
CC: Christoph Hellwig <hch@lst.de>, Marek Szyprowski
	<m.szyprowski@samsung.com>, Robin Murphy <robin.murphy@arm.com>, Joerg Roedel
	<joro@8bytes.org>, Will Deacon <will@kernel.org>, "Rafael J. Wysocki"
	<rafael@kernel.org>, Magnus Karlsson <magnus.karlsson@intel.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <bpf@vger.kernel.org>,
	<netdev@vger.kernel.org>, <iommu@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>
References: <20240423135832.2271696-1-aleksander.lobakin@intel.com>
 <20240423135832.2271696-7-aleksander.lobakin@intel.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <20240423135832.2271696-7-aleksander.lobakin@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U3dUd2lOT0gxSWx1TlI2TG9pQW5uR0RVQ05BWDEzNVY2MjdidGhVbmxHcnp0?=
 =?utf-8?B?VFAzZmVBa0ViV0RWUlF6YnhCTUlrVVVLUWF5d2hQTEpQMmJxdGtPWmt2Y2g4?=
 =?utf-8?B?SjF0dU51dUwxQUZUeE40cDNDVlN3MzZoeXVxV0dQQjFiZEhoQmJWeFdGQVNq?=
 =?utf-8?B?S0I2MmtlWnRXcXlkQU1ZMlU1NGdXdHdhenBkRkI4YmZneGdSZW1aVUpIUTEx?=
 =?utf-8?B?ZzNZOEo2SUZuanBPVk41Vk9RREoxbFF3U1QrWTYvaFNGcm02SENqNVNRc01H?=
 =?utf-8?B?bUFOMm1qV3NtUnFvVlgrTnFvc1Ztc2JPY3dtS00ybVc1NVR0ekRVc2lYUzFp?=
 =?utf-8?B?bEVXVEJrTzRrcXhtNXFudnRCSndvOEx6ZkZrcjVjai9zUC8yTWZqcE1UMkg0?=
 =?utf-8?B?MUg4b2ZhTy92NS9uNWVRU25Gc1IrMkJiTTRLTTlhOXJncnRRRTVhQ3BDNFV5?=
 =?utf-8?B?ZWVqOFltNXQ3YnZNL0t0ajdvZ2JraVNVRk5rWHd0MWxvdWdlOXBwWlFXU25L?=
 =?utf-8?B?OHAvRVBxVlRFZEZRTjZlOFB0bS9lQm1aTkpoNm9RaWRMaTgzcGZDTE1XZEkz?=
 =?utf-8?B?bndpZzllL2hQZHRGZzR0ejA1cnhuVHNWS3FmNlRYSHhwN1N6SVZlOUw3Qy8v?=
 =?utf-8?B?ajQ1WkVMQmpLNzNxVGRLQk1tT045ZlJrMmhoc09pRy8wNlJLU1lRTHlYcHVU?=
 =?utf-8?B?Z2lvY09acXZrZG1aMkRiMTlXeU1tY2FMd1FjdEd1cEIwd2VvdDRzNGVaL3lZ?=
 =?utf-8?B?aUF5VEc4VnAzckVxRGJDUEYveHRRM1hhSXRwVzM3dXNQcG44bnNuY3BXdUZP?=
 =?utf-8?B?amxlRmVsZnpUbHdSeGFITUVtYjNpcUNwWktmT1RFSjRZeWdtMkhSN1JlVHJx?=
 =?utf-8?B?UU9GeFUxeDd5ZTJGK2o0cGxycWRNa0RWb0Qya2xSem93dHJmc1B5VllDcVpQ?=
 =?utf-8?B?MlBlNW4ya3dsUTFGQ2E1SXRicjBRMGw5blFpdUszbGltZjNUcmJSMjU1Tjhu?=
 =?utf-8?B?WUV2UUFvdTlaVW1IbzdTcVB3YVB2ajdtWXloQllIR2JqMFFvZktSaG4yeDh4?=
 =?utf-8?B?U1ZOcmFZRU43UUpPTWFQSU1RcDg2dkxyT2J1YmljQzVWdjJyLzRWT0NkMlZU?=
 =?utf-8?B?ZmMxaFpBNFJERnJzMzFFbkU0TEg0RGJvT3ZjUVFXRVk1SzhtS1NNa2FlTTdk?=
 =?utf-8?B?OEJTRXRIZEVqeWFVTm5EdDNyZzMzT1prR3hRVDJ2a3hxbzFwSE5UZjNVTnVR?=
 =?utf-8?B?eHgrSktTcnl5a0hvQ0tXMkNIYlU5ZmlFUGJaU3lWUGIza2czT3VEVldCOXo4?=
 =?utf-8?B?bW1ISmxnU3ZRcktSN0NyNHFSYkxQTHZBV0Z5NlZHVGdreGVPbGp3U3d2cUp2?=
 =?utf-8?B?aFVudlM2N2MxeEw3UzFqZXBVN1FvTWs5YldlTnBuVCtJSjNldUFQVzdsZkhO?=
 =?utf-8?B?NWN0SitteVhyNEtJT05jMjhzdE02VVZERDFtN0ZqbXhIYXVEcnJQbjR2cGVh?=
 =?utf-8?B?Y1dhdEJpVlp3TFRjeWVwRUkvUDJZOWFFZWZjek1pNTFSenNOcVpIVVFlZHRP?=
 =?utf-8?B?OW84Mnd6b1pFc1ljM1Y1cEd1bUI0ZTFJSE8vYkJPbXdnakcvL29nMnhva2Vs?=
 =?utf-8?B?VHRvcFl3OXlZSUZuTVR6dzE0R2hMbjBBcWI2TmRoR2hjaWhZTkNOeWVmZ3NG?=
 =?utf-8?B?VzNiQUEwMWI3UFNXSzBnNjFUMytlUWxkRStJMzJTNUdDSExiQk81blR6cXRC?=
 =?utf-8?B?SFVUeXluUVdXL1F5M2ZmaGxkdjJYbkQ5VWlaTndOaE1YcHBudldwaGVOb3Fz?=
 =?utf-8?B?UlhSdEY5eVQ0ck8rZy9WSlhzek11bWFqVkhLZ093OXVFZXRuMWFibFBYYUx2?=
 =?utf-8?B?SFJtcDl5elBReXN6UHlUK1NaYWZYYkYrR2ZwOXBpNGZBZUwrVG5ndU1UQ2cz?=
 =?utf-8?B?K0F1bitXSjFuZXdncWYvNU9sYiswWk1Ka09WTlBoVS94eTN1cDdwR0UrRVJl?=
 =?utf-8?B?SXdTRk5zNUZ0a3hybkt4TGtFUnIrUnpPVGp5SUNBNjdmandnbXJPV0VDVnJF?=
 =?utf-8?B?aXh5THJVVVJ4UXFacHY5YUxGVmhpMmMvNkphMFJTQS9HdmpRQ3pIbHY0cFE0?=
 =?utf-8?B?YVdiZVhBQ3lWZ053ZEVLQUxYK0IyWWFjZW4yenN2RWhlK3hkWHRBTmFwUVpu?=
 =?utf-8?B?Y2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bd4ae2c0-6e65-473b-249d-08dc643bee63
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2024 08:52:54.7289
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jI1ylxAw58YMcsrtKok7GV7YeMRmfebSlZcmQUwmFTr3V3yx2tgUwt+gpD6ENA96Ju1iGuqNwPQo7mxllmifEGwTmyQMjfmn0sfrqFH0xmE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR11MB8510
X-OriginatorOrg: intel.com

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Tue, 23 Apr 2024 15:58:31 +0200

> We can save a couple more function calls in the Page Pool code if we
> check for dma_need_sync() earlier, just when we test pp->p.dma_sync.
> Move both these checks into an inline wrapper and call the PP wrapper
> over the generic DMA sync function only when both are true.
> You can't cache the result of dma_need_sync() in &page_pool, as it may
> change anytime if an SWIOTLB buffer is allocated or mapped.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  net/core/page_pool.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 6cf26a68fa91..87319c6365e0 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -398,16 +398,24 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
>  	return page;
>  }
>  
> -static void page_pool_dma_sync_for_device(const struct page_pool *pool,
> -					  const struct page *page,
> -					  unsigned int dma_sync_size)
> +static void __page_pool_dma_sync_for_device(const struct page_pool *pool,
> +					    const struct page *page,
> +					    u32 dma_sync_size)
>  {
>  	dma_addr_t dma_addr = page_pool_get_dma_addr(page);
>  
>  	dma_sync_size = min(dma_sync_size, pool->p.max_len);
> -	dma_sync_single_range_for_device(pool->p.dev, dma_addr,
> -					 pool->p.offset, dma_sync_size,
> -					 pool->p.dma_dir);
> +	__dma_sync_single_for_device(pool->p.dev, dma_addr + pool->p.offset,
> +				     dma_sync_size, pool->p.dma_dir);

Breh, this breaks builds on !DMA_NEED_SYNC systems, as this function
isn't defined there, and my CI didn't catch it in time... I'll ifdef
this in the next spin after some reviews for this one.

> +}
> +
> +static __always_inline void
> +page_pool_dma_sync_for_device(const struct page_pool *pool,
> +			      const struct page *page,
> +			      u32 dma_sync_size)
> +{
> +	if (pool->dma_sync && dma_dev_need_sync(pool->p.dev))
> +		__page_pool_dma_sync_for_device(pool, page, dma_sync_size);
>  }
>  
>  static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
> @@ -429,8 +437,7 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>  	if (page_pool_set_dma_addr(page, dma))
>  		goto unmap_failed;
>  
> -	if (pool->dma_sync)
> -		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
> +	page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
>  
>  	return true;
>  
> @@ -699,9 +706,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>  	if (likely(__page_pool_page_can_be_recycled(page))) {
>  		/* Read barrier done in page_ref_count / READ_ONCE */
>  
> -		if (pool->dma_sync)
> -			page_pool_dma_sync_for_device(pool, page,
> -						      dma_sync_size);
> +		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
>  
>  		if (allow_direct && page_pool_recycle_in_cache(page, pool))
>  			return NULL;
> @@ -840,9 +845,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
>  		return NULL;
>  
>  	if (__page_pool_page_can_be_recycled(page)) {
> -		if (pool->dma_sync)
> -			page_pool_dma_sync_for_device(pool, page, -1);
> -
> +		page_pool_dma_sync_for_device(pool, page, -1);
>  		return page;
>  	}

Thanks,
Olek