diff mbox series

[net-next,v2,2/7] dma: avoid redundant calls for sync operations

Message ID 20240205110426.764393-3-aleksander.lobakin@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series dma: skip calling no-op sync ops when possible | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 14997 this patch: 14997
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: james@equiv.tech
netdev/build_clang success Errors and warnings before: 3169 this patch: 3169
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16238 this patch: 16238
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 155 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline fail Was 0 now: 1
netdev/contest success net-next-2024-02-05--18-00 (tests: 720)

Commit Message

Alexander Lobakin Feb. 5, 2024, 11:04 a.m. UTC 
Quite often, NIC devices do not need dma_sync operations on x86_64
at least.
Indeed, when dev_is_dma_coherent(dev) is true and
dev_use_swiotlb(dev) is false, iommu_dma_sync_single_for_cpu()
and friends do nothing.

However, indirectly calling them when CONFIG_RETPOLINE=y consumes about
10% of cycles on a cpu receiving packets from softirq at ~100Gbit rate.
Even if/when CONFIG_RETPOLINE is not set, there is a cost of about 3%.

Add dev->skip_dma_sync boolean which is set during the device
initialization depending on the setup: dev_is_dma_coherent() for the
direct DMA, !(sync_single_for_device || sync_single_for_cpu) or the new
dma_map_ops flag, %DMA_F_CAN_SKIP_SYNC, advertised for non-NULL DMA ops.
Then later, if/when swiotlb is used for the first time, the flag
is turned off, from swiotlb_tbl_map_single().

On iavf, the UDP trafficgen with XDP_DROP in skb mode test shows
+3-5% increase for direct DMA.

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.

Suggested-by: Christoph Hellwig <hch@lst.de> # direct DMA shortcut
Co-developed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 include/linux/device.h      |  5 +++++
 include/linux/dma-map-ops.h | 20 ++++++++++++++++++
 include/linux/dma-mapping.h |  6 +++++-
 drivers/base/dd.c           |  2 ++
 kernel/dma/mapping.c        | 42 ++++++++++++++++++++++++++++++++++++-
 kernel/dma/swiotlb.c        | 14 +++++++++++++
 6 files changed, 87 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Feb. 13, 2024, 6:11 a.m. UTC | #1 
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.
Alexander Lobakin Feb. 13, 2024, 10:19 a.m. UTC | #2 
From: Christoph Hellwig <hch@lst.de>
Date: Tue, 13 Feb 2024 07:11:20 +0100

> 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.

This sentence is from the original Eric's commit message, but I'll
reword it :D

> 
>> 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.

That's why I didn't move it to dma-mapping.h -- in general, drivers
should not call it, so it would be a workaround. I added this paragraph
in v2 as a couple folks asked "what if some weird device will break with
this optimization". I can drop it anyway.

> 
>>  	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.

Sounds good!

Thanks,
Olek
diff mbox series

Patch

diff --git a/include/linux/device.h b/include/linux/device.h
index 97c4b046c09d..f23e6a32bea0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -686,6 +686,8 @@  struct device_physical_location {
  *		other devices probe successfully.
  * @dma_coherent: this particular device is dma coherent, even if the
  *		architecture supports non-coherent devices.
+ * @dma_skip_sync: DMA sync operations can be skipped for coherent non-SWIOTLB
+ *		buffers.
  * @dma_ops_bypass: If set to %true then the dma_ops are bypassed for the
  *		streaming DMA operations (->map_* / ->unmap_* / ->sync_*),
  *		and optionall (if the coherent mask is large enough) also
@@ -800,6 +802,9 @@  struct device {
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
 	bool			dma_coherent:1;
 #endif
+#ifdef CONFIG_DMA_NEED_SYNC
+	bool			dma_skip_sync:1;
+#endif
 #ifdef CONFIG_DMA_OPS_BYPASS
 	bool			dma_ops_bypass : 1;
 #endif
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index 4abc60f04209..3406fb950980 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -18,8 +18,11 @@  struct iommu_ops;
  *
  * DMA_F_PCI_P2PDMA_SUPPORTED: Indicates the dma_map_ops implementation can
  * handle PCI P2PDMA pages in the map_sg/unmap_sg operation.
+ * DMA_F_CAN_SKIP_SYNC: DMA sync operations can be skipped if the device is
+ * coherent and it's not an SWIOTLB buffer.
  */
 #define DMA_F_PCI_P2PDMA_SUPPORTED     (1 << 0)
+#define DMA_F_CAN_SKIP_SYNC		BIT(1)
 
 struct dma_map_ops {
 	unsigned int flags;
@@ -111,6 +114,23 @@  static inline void set_dma_ops(struct device *dev,
 }
 #endif /* CONFIG_DMA_OPS */
 
+#ifdef CONFIG_DMA_NEED_SYNC
+void dma_setup_skip_sync(struct device *dev);
+
+static inline void dma_set_skip_sync(struct device *dev, bool skip)
+{
+	dev->dma_skip_sync = skip;
+}
+#else /* !CONFIG_DMA_NEED_SYNC */
+static inline void dma_setup_skip_sync(struct device *dev)
+{
+}
+
+static inline void dma_set_skip_sync(struct device *dev, bool skip)
+{
+}
+#endif /* !CONFIG_DMA_NEED_SYNC */
+
 #ifdef CONFIG_DMA_CMA
 extern struct cma *dma_contiguous_default_area;
 
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 569a4da68f56..03711ae6c4db 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -370,7 +370,11 @@  __dma_sync_single_range_for_device(struct device *dev, dma_addr_t addr,
 
 static inline bool dma_skip_sync(const struct device *dev)
 {
-	return !IS_ENABLED(CONFIG_DMA_NEED_SYNC);
+#ifdef CONFIG_DMA_NEED_SYNC
+	return dev->dma_skip_sync;
+#else
+	return true;
+#endif
 }
 
 static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 85152537dbf1..67ad3e1d51f6 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -642,6 +642,8 @@  static int really_probe(struct device *dev, struct device_driver *drv)
 			goto pinctrl_bind_failed;
 	}
 
+	dma_setup_skip_sync(dev);
+
 	ret = driver_sysfs_add(dev);
 	if (ret) {
 		pr_err("%s: driver_sysfs_add(%s) failed\n",
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 8716e5e8281c..b815e1bbc2d0 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -846,8 +846,14 @@  bool __dma_need_sync(struct device *dev, dma_addr_t dma_addr)
 	const struct dma_map_ops *ops = get_dma_ops(dev);
 
 	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);
-	return ops->sync_single_for_cpu || ops->sync_single_for_device;
+
+	return true;
 }
 EXPORT_SYMBOL_GPL(__dma_need_sync);
 
@@ -861,3 +867,37 @@  unsigned long dma_get_merge_boundary(struct device *dev)
 	return ops->get_merge_boundary(dev);
 }
 EXPORT_SYMBOL_GPL(dma_get_merge_boundary);
+
+#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);
+}
+#endif /* CONFIG_DMA_NEED_SYNC */
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b079a9a8e087..b62ea0a4f106 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -1286,6 +1286,16 @@  static unsigned long mem_used(struct io_tlb_mem *mem)
 
 #endif /* CONFIG_DEBUG_FS */
 
+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);
+}
+
 phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
 		size_t mapping_size, size_t alloc_size,
 		unsigned int alloc_align_mask, enum dma_data_direction dir,
@@ -1323,6 +1333,8 @@  phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
 		return (phys_addr_t)DMA_MAPPING_ERROR;
 	}
 
+	swiotlb_disable_dma_skip_sync(dev);
+
 	/*
 	 * Save away the mapping from the original address to the DMA address.
 	 * This is needed when we sync the memory.  Then we sync the buffer if
@@ -1640,6 +1652,8 @@  struct page *swiotlb_alloc(struct device *dev, size_t size)
 	if (index == -1)
 		return NULL;
 
+	swiotlb_disable_dma_skip_sync(dev);
+
 	tlb_addr = slot_addr(pool->start, index);
 
 	return pfn_to_page(PFN_DOWN(tlb_addr));