Received-SPF: pass (google.com: domain of linux-kernel+bounces-62992-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 13 Feb 2024 07:11:20 +0100
From: Christoph Hellwig <hch@lst.de>
To: Alexander Lobakin <aleksander.lobakin@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	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>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Magnus Karlsson <magnus.karlsson@intel.com>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	Alexander Duyck <alexanderduyck@fb.com>, bpf@vger.kernel.org,
	netdev@vger.kernel.org, iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 2/7] dma: avoid redundant calls for sync
 operations
Message-ID: <20240213061120.GC22451@lst.de>
References: <20240205110426.764393-1-aleksander.lobakin@intel.com> <20240205110426.764393-3-aleksander.lobakin@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240205110426.764393-3-aleksander.lobakin@intel.com>
User-Agent: Mutt/1.5.17 (2007-11-01)

On Mon, Feb 05, 2024 at 12:04:21PM +0100, Alexander Lobakin wrote:
> Quite often, NIC devices do not need dma_sync operations on x86_64
> at least.

This is a fundamental property of the platform being DMA coherent,
and devices / platforms not having addressing limitations or other
need for bounce buffering (like all those whacky trusted platform
schemes).  Nothing NIC-specific here.

> In case some device doesn't work with the shortcut:
> * include <linux/dma-map-ops.h> to the driver source;
> * call dma_set_skip_sync(dev, false) at the beginning of the probe
>   callback. This will disable the shortcut and force DMA syncs.

No, drivers should never include dma-map-ops.h.  If we have a legit
reason for drivers to ever call it it would have to move to
dma-mapping.h.  But I see now reason why there would be such a need.
For now I'd suggest simply dropping this paragraph from the commit
message.

>  	if (dma_map_direct(dev, ops))
> +		/*
> +		 * dma_skip_sync could've been set to false on first SWIOTLB
> +		 * buffer mapping, but @dma_addr is not necessary an SWIOTLB
> +		 * buffer. In this case, fall back to more granular check.
> +		 */
>  		return dma_direct_need_sync(dev, dma_addr);
> +

Nit: with such a long block comment adding curly braces would make the
code a bit more readable.

> +#ifdef CONFIG_DMA_NEED_SYNC
> +void dma_setup_skip_sync(struct device *dev)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +	bool skip;
> +
> +	if (dma_map_direct(dev, ops))
> +		/*
> +		 * dma_skip_sync will be set to false on first SWIOTLB buffer
> +		 * mapping, if any. During the device initialization, it's
> +		 * enough to check only for DMA coherence.
> +		 */
> +		skip = dev_is_dma_coherent(dev);
> +	else if (!ops->sync_single_for_device && !ops->sync_single_for_cpu)
> +		/*
> +		 * Synchronization is not possible when none of DMA sync ops
> +		 * is set. This check precedes the below one as it disables
> +		 * the synchronization unconditionally.
> +		 */
> +		skip = true;
> +	else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
> +		/*
> +		 * Assume that when ``DMA_F_CAN_SKIP_SYNC`` is advertised,
> +		 * the conditions for synchronizing are the same as with
> +		 * the direct DMA.
> +		 */
> +		skip = dev_is_dma_coherent(dev);
> +	else
> +		skip = false;
> +
> +	dma_set_skip_sync(dev, skip);

I'd just assign directly to dev->dma_skip_sync instead of using a
local variable and the dma_set_skip_sync call - we are under
ifdef CONFIG_DMA_NEED_SYNC here and thus know is is available.

> +static inline void swiotlb_disable_dma_skip_sync(struct device *dev)
> +{
> +	/*
> +	 * If dma_skip_sync was set, reset it to false on first SWIOTLB buffer
> +	 * mapping/allocation to always sync SWIOTLB buffers.
> +	 */
> +	if (unlikely(dma_skip_sync(dev)))
> +		dma_set_skip_sync(dev, false);
> +}

Nothing really swiotlb-specific here.  Also the naming is a bit odd.
Maybe have a dma_set_skip_sync helper without the bool to enable
skipping, and a dma_clear_skip_sync that clear the flag.  The optimization
to first check the flag here could just move into that latter
helper.