Re: [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers

From: Alexander Lobakin <aleksander.lobakin@intel.com>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Paolo Abeni <pabeni@redhat.com>,
	"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
	Magnus Karlsson <magnus.karlsson@intel.com>,
	Michal Kubiak <michal.kubiak@intel.com>,
	"Larysa Zaremba" <larysa.zaremba@intel.com>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	Christoph Hellwig <hch@lst.de>, <netdev@vger.kernel.org>,
	<intel-wired-lan@lists.osuosl.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers
Date: Thu, 18 May 2023 15:53:43 +0200	[thread overview]
Message-ID: <b19f43b8-f973-2627-3ef1-ce53007ece0f@intel.com> (raw)
In-Reply-To: <ZGXNzX77/5cXqAhe@hera>

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Date: Thu, 18 May 2023 10:03:41 +0300

> Hi all,
> 
>> On Wed, May 17, 2023 at 09:12:11PM -0700, Jakub Kicinski wrote:
>> On Tue, 16 May 2023 18:18:37 +0200 Alexander Lobakin wrote:
>>> Each driver is responsible for syncing buffers written by HW for CPU
>>> before accessing them. Almost each PP-enabled driver uses the same
>>> pattern, which could be shorthanded into a static inline to make driver
>>> code a little bit more compact.

[...]

>>> +	dma_sync_single_range_for_cpu(pool->p.dev,
>>> +				      page_pool_get_dma_addr(page),
>>> +				      pool->p.offset, dma_sync_size,
>>> +				      page_pool_get_dma_dir(pool));
>>
>> Likely a dumb question but why does this exist?
>> Is there a case where the "maybe" version is not safe?
>>
> 
> I got similar concerns here.  Syncing for the cpu is currently a
> responsibility for the driver.  The reason for having an automated DMA sync
> is that we know when we allocate buffers for the NIC to consume so we can
> safely sync them accordingly.  I am fine having a page pool version for the
> cpu sync, but do we really have to check the pp flags for that?  IOW if you
> are at the point that you need to sync a buffer for the cpu *someone*
> already mapped it for you.  Regardsless of who mapped it the sync is
> identical

The flag in the "maybe" version is the continuation of the shortcut from
6/11. If the flag is not set, but you asked PP to do syncs, that means
it enabled the shortcut to not go through function call ladders for
nothing. The ladder is basically the same for sync-for-CPU as the one
described in 6/11 for sync-for-dev.
I could place that in the driver, but I feel like it's better to have
that one level up to reduce boilerplating.

> 
>>> +}
>>> +
>>> +/**
>>> + * page_pool_dma_maybe_sync_for_cpu - sync Rx page for CPU if needed
>>> + * @pool: page_pool which this page belongs to
>>> + * @page: page to sync
>>> + * @dma_sync_size: size of the data written to the page
>>> + *
>>> + * Performs DMA sync for CPU, but only when required (swiotlb, IOMMU etc.).
>>> + */
>>> +static inline void
>>> +page_pool_dma_maybe_sync_for_cpu(const struct page_pool *pool,
>>> +				 const struct page *page, u32 dma_sync_size)
>>> +{
>>> +	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
>>> +		page_pool_dma_sync_for_cpu(pool, page, dma_sync_size);
>>> +}
> 
> Thanks
> /Ilias

Thanks,
Olek