Quite often, 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.
Suggested-by: Christoph Hellwig <[email protected]> # direct DMA shortcut
Co-developed-by: Eric Dumazet <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Signed-off-by: Alexander Lobakin <[email protected]>
---
include/linux/device.h | 5 +++++
include/linux/dma-map-ops.h | 21 ++++++++++++++++++++
include/linux/dma-mapping.h | 6 +++++-
drivers/base/dd.c | 2 ++
kernel/dma/mapping.c | 39 ++++++++++++++++++++++++++++++++++++-
kernel/dma/swiotlb.c | 8 ++++++++
6 files changed, 79 insertions(+), 2 deletions(-)
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..327b73f653ad 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,24 @@ 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_clear_skip_sync(struct device *dev)
+{
+ /* Clear it only once so that the function can be called on hotpath */
+ if (unlikely(dev->dma_skip_sync))
+ dev->dma_skip_sync = false;
+}
+#else /* !CONFIG_DMA_NEED_SYNC */
+static inline void dma_setup_skip_sync(struct device *dev)
+{
+}
+static inline void dma_clear_skip_sync(struct device *dev)
+{
+}
+#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 6c7640441214..d85ae541c267 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -364,7 +364,11 @@ static inline void __dma_sync_single_range_for_device(struct device *dev,
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 85feaa0e008c..5f588e31ea89 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,34 @@ 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);
+
+ 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.
+ */
+ dev->dma_skip_sync = 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.
+ */
+ dev->dma_skip_sync = 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.
+ */
+ dev->dma_skip_sync = dev_is_dma_coherent(dev);
+ else
+ dev->dma_skip_sync = false;
+}
+#endif /* CONFIG_DMA_NEED_SYNC */
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b079a9a8e087..0b737eab4d48 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -1323,6 +1323,12 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
return (phys_addr_t)DMA_MAPPING_ERROR;
}
+ /*
+ * If dma_skip_sync was set, reset it to false on first SWIOTLB buffer
+ * mapping to always sync SWIOTLB buffers.
+ */
+ dma_clear_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 +1646,8 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
if (index == -1)
return NULL;
+ dma_clear_skip_sync(dev);
+
tlb_addr = slot_addr(pool->start, index);
return pfn_to_page(PFN_DOWN(tlb_addr));
--
2.43.0
On 2024-02-14 4:21 pm, Alexander Lobakin wrote:
> Quite often, 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.
>
> Suggested-by: Christoph Hellwig <[email protected]> # direct DMA shortcut
> Co-developed-by: Eric Dumazet <[email protected]>
> Signed-off-by: Eric Dumazet <[email protected]>
> Signed-off-by: Alexander Lobakin <[email protected]>
> ---
> include/linux/device.h | 5 +++++
> include/linux/dma-map-ops.h | 21 ++++++++++++++++++++
> include/linux/dma-mapping.h | 6 +++++-
> drivers/base/dd.c | 2 ++
> kernel/dma/mapping.c | 39 ++++++++++++++++++++++++++++++++++++-
> kernel/dma/swiotlb.c | 8 ++++++++
> 6 files changed, 79 insertions(+), 2 deletions(-)
>
> 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..327b73f653ad 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)
Yuck, please be consistent - either match the style of the existing
code, or change that to BIT(0) as well.
> struct dma_map_ops {
> unsigned int flags;
> @@ -111,6 +114,24 @@ 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_clear_skip_sync(struct device *dev)
> +{
> + /* Clear it only once so that the function can be called on hotpath */
> + if (unlikely(dev->dma_skip_sync))
> + dev->dma_skip_sync = false;
> +}
> +#else /* !CONFIG_DMA_NEED_SYNC */
> +static inline void dma_setup_skip_sync(struct device *dev)
> +{
> +}
> +static inline void dma_clear_skip_sync(struct device *dev)
> +{
> +}
> +#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 6c7640441214..d85ae541c267 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -364,7 +364,11 @@ static inline void __dma_sync_single_range_for_device(struct device *dev,
>
> 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 85feaa0e008c..5f588e31ea89 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,34 @@ 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);
> +
> + if (dma_map_direct(dev, ops))
For DMA_OPS_BYPASS this will be making the decision based on the default
dma_mask, but a driver could subsequently set a smaller mask for which
the bypass condition will no longer be true.
Maybe instead of driver probe this setup should actually be tied in to
dma_set_mask() anyway?
> + /*
> + * 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.
> + */
> + dev->dma_skip_sync = dev_is_dma_coherent(dev);
> + else if (!ops->sync_single_for_device && !ops->sync_single_for_cpu)
I guess this was the existing condition from dma_need_sync(), but now
it's on a one-off slow path it might be nice to check the sync_sg_* ops
as well for completeness, or at least comment that nobody should be
implementing those without also implementing the sync_single_* ops.
> + /*
> + * Synchronization is not possible when none of DMA sync ops
> + * is set. This check precedes the below one as it disables
> + * the synchronization unconditionally.
> + */
> + dev->dma_skip_sync = true;
> + else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
Personally I'd combine this into the dma-direct condition.
> + /*
> + * Assume that when ``DMA_F_CAN_SKIP_SYNC`` is advertised,
> + * the conditions for synchronizing are the same as with
> + * the direct DMA.
> + */
> + dev->dma_skip_sync = dev_is_dma_coherent(dev);
> + else
> + dev->dma_skip_sync = false;
> +}
> +#endif /* CONFIG_DMA_NEED_SYNC */
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index b079a9a8e087..0b737eab4d48 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -1323,6 +1323,12 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> return (phys_addr_t)DMA_MAPPING_ERROR;
> }
>
> + /*
> + * If dma_skip_sync was set, reset it to false on first SWIOTLB buffer
> + * mapping to always sync SWIOTLB buffers.
> + */
> + dma_clear_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 +1646,8 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
> if (index == -1)
> return NULL;
>
> + dma_clear_skip_sync(dev);
We don't need this here, since this isn't a streaming API path.
Thanks,
Robin.
> +
> tlb_addr = slot_addr(pool->start, index);
>
> return pfn_to_page(PFN_DOWN(tlb_addr));
On Wed, Feb 14, 2024 at 05:55:23PM +0000, Robin Murphy wrote:
>> #define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0)
>> +#define DMA_F_CAN_SKIP_SYNC BIT(1)
>
> Yuck, please be consistent - either match the style of the existing code,
> or change that to BIT(0) as well.
Just don't use BIT() ever. It doesn't save any typing and creates a
totally pointless mental indirection.
> I guess this was the existing condition from dma_need_sync(), but now it's
> on a one-off slow path it might be nice to check the sync_sg_* ops as well
> for completeness, or at least comment that nobody should be implementing
> those without also implementing the sync_single_* ops.
Implementing only one and not the other doesn't make any sense. Maybe
a debug check for that is ok, but thing will break badly if they aren't
in sync anyway.
On 15/02/2024 5:08 am, Christoph Hellwig wrote:
> On Wed, Feb 14, 2024 at 05:55:23PM +0000, Robin Murphy wrote:
>>> #define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0)
>>> +#define DMA_F_CAN_SKIP_SYNC BIT(1)
>>
>> Yuck, please be consistent - either match the style of the existing code,
>> or change that to BIT(0) as well.
>
> Just don't use BIT() ever. It doesn't save any typing and creates a
> totally pointless mental indirection.
>
>> I guess this was the existing condition from dma_need_sync(), but now it's
>> on a one-off slow path it might be nice to check the sync_sg_* ops as well
>> for completeness, or at least comment that nobody should be implementing
>> those without also implementing the sync_single_* ops.
>
> Implementing only one and not the other doesn't make any sense. Maybe
> a debug check for that is ok, but thing will break badly if they aren't
> in sync anyway.
In principle we *could* have an implementation which used bouncing
purely to merge coherent scatterlist segments, thus didn't need to do
anything for single mappings. I agree that it wouldn't seem like a
particularly realistic thing to do these days, but I don't believe the
API rules it out, so it might be nice to enforce that assumption
somewhere if we are actually relying on it (although I also concur that
this may not necessarily be the ideal place to do that in general).
Thanks,
Robin.
From: Robin Murphy <[email protected]>
Date: Wed, 14 Feb 2024 17:55:23 +0000
> On 2024-02-14 4:21 pm, Alexander Lobakin wrote:
[...]
>> + /*
>> + * Synchronization is not possible when none of DMA sync ops
>> + * is set. This check precedes the below one as it disables
>> + * the synchronization unconditionally.
>> + */
>> + dev->dma_skip_sync = true;
>> + else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
>
> Personally I'd combine this into the dma-direct condition.
Please read the code comment a couple lines above :D
>
>> + /*
>> + * Assume that when ``DMA_F_CAN_SKIP_SYNC`` is advertised,
>> + * the conditions for synchronizing are the same as with
>> + * the direct DMA.
>> + */
>> + dev->dma_skip_sync = dev_is_dma_coherent(dev);
>> + else
>> + dev->dma_skip_sync = false;
>> +}
>> +#endif /* CONFIG_DMA_NEED_SYNC */
[...]
Thanks,
Olek
On 19/02/2024 12:49 pm, Alexander Lobakin wrote:
> From: Robin Murphy <[email protected]>
> Date: Wed, 14 Feb 2024 17:55:23 +0000
>
>> On 2024-02-14 4:21 pm, Alexander Lobakin wrote:
>
> [...]
>
>>> + /*
>>> + * Synchronization is not possible when none of DMA sync ops
>>> + * is set. This check precedes the below one as it disables
>>> + * the synchronization unconditionally.
>>> + */
>>> + dev->dma_skip_sync = true;
>>> + else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
>>
>> Personally I'd combine this into the dma-direct condition.
>
> Please read the code comment a couple lines above :D
And my point is that that logic is not actually useful, since it would
be nonsensical for ops to set DMA_F_CAN_SKIP_SYNC if they don't even
implement sync ops anyway.
If the intent of DMA_F_CAN_SKIP_SYNC is to mean "behaves like
dma-direct", then "if (dma_map_direct(...) || ops->flags &
DMA_F_CAN_SKIP_SYNC)" is an entirely logical and expected condition.
Thanks,
Robin.