From: Robin Murphy <robin.murphy@arm.com>
To: Alexander Lobakin <aleksander.lobakin@intel.com>,
"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>,
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 2/7] dma: avoid expensive redundant calls for sync operations
Date: Fri, 26 Jan 2024 15:48:54 +0000 [thread overview]
Message-ID: <0f6f550c-3eee-46dc-8c42-baceaa237610@arm.com> (raw)
In-Reply-To: <20240126135456.704351-3-aleksander.lobakin@intel.com>
On 26/01/2024 1:54 pm, Alexander Lobakin wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 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 direct
> DMA, !(sync_single_for_device || sync_single_for_cpu) or positive result
> from the new callback, dma_map_ops::can_skip_sync 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().
I think you could probably just promote the dma_uses_io_tlb flag from
SWIOTLB_DYNAMIC to a general SWIOTLB thing to serve this purpose now.
Similarly I don't think a new op is necessary now that we have
dma_map_ops.flags. A simple static flag to indicate that sync may be
skipped under the same conditions as implied for dma-direct - i.e.
dev_is_dma_coherent(dev) && !dev->dma_use_io_tlb - seems like it ought
to suffice.
Thanks,
Robin.
> On iavf, the UDP trafficgen with XDP_DROP in skb mode test shows
> +3-5% increase for direct DMA.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Co-developed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
> include/linux/device.h | 5 +++++
> include/linux/dma-map-ops.h | 17 +++++++++++++++++
> include/linux/dma-mapping.h | 12 ++++++++++--
> drivers/base/dd.c | 2 ++
> kernel/dma/mapping.c | 34 +++++++++++++++++++++++++++++++---
> kernel/dma/swiotlb.c | 14 ++++++++++++++
> 6 files changed, 79 insertions(+), 5 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..937c295e9da8 100644
> --- a/include/linux/dma-map-ops.h
> +++ b/include/linux/dma-map-ops.h
> @@ -78,6 +78,7 @@ struct dma_map_ops {
> int nents, enum dma_data_direction dir);
> void (*cache_sync)(struct device *dev, void *vaddr, size_t size,
> enum dma_data_direction direction);
> + bool (*can_skip_sync)(struct device *dev);
> int (*dma_supported)(struct device *dev, u64 mask);
> u64 (*get_required_mask)(struct device *dev);
> size_t (*max_mapping_size)(struct device *dev);
> @@ -111,6 +112,22 @@ static inline void set_dma_ops(struct device *dev,
> }
> #endif /* CONFIG_DMA_OPS */
>
> +#ifdef CONFIG_DMA_NEED_SYNC
> +
> +static inline void dma_set_skip_sync(struct device *dev, bool skip)
> +{
> + dev->dma_skip_sync = skip;
> +}
> +
> +void dma_setup_skip_sync(struct device *dev);
> +
> +#else /* !CONFIG_DMA_NEED_SYNC */
> +
> +#define dma_set_skip_sync(dev, skip) do { } while (0)
> +#define dma_setup_skip_sync(dev) do { } while (0)
> +
> +#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 9dd7e1578bf6..bc9f67e0c139 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -365,9 +365,17 @@ __dma_sync_single_range_for_device(struct device *dev, dma_addr_t addr,
>
> #ifdef CONFIG_DMA_NEED_SYNC
>
> -#define dma_skip_sync(dev) false
> +static inline bool dma_skip_sync(const struct device *dev)
> +{
> + return dev->dma_skip_sync;
> +}
> +
> +bool __dma_need_sync(struct device *dev, dma_addr_t dma_addr);
>
> -bool dma_need_sync(struct device *dev, dma_addr_t dma_addr);
> +static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
> +{
> + return dma_skip_sync(dev) ? false : __dma_need_sync(dev, dma_addr);
> +}
>
> #else /* !CONFIG_DMA_NEED_SYNC */
>
> 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 a30f37f9d4db..8fa464b3954e 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -842,15 +842,43 @@ size_t dma_opt_mapping_size(struct device *dev)
> EXPORT_SYMBOL_GPL(dma_opt_mapping_size);
>
> #ifdef CONFIG_DMA_NEED_SYNC
> -bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
> +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);
> +
> +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)
> + skip = true;
> + else if (ops->can_skip_sync)
> + skip = ops->can_skip_sync(dev);
> + else
> + skip = false;
> +
> + dma_set_skip_sync(dev, skip);
> }
> -EXPORT_SYMBOL_GPL(dma_need_sync);
> #endif /* CONFIG_DMA_NEED_SYNC */
>
> unsigned long dma_get_merge_boundary(struct device *dev)
> 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));
next prev parent reply other threads:[~2024-01-26 15:48 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 13:54 [PATCH net-next 0/7] dma: skip calling no-op sync ops when possible Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 1/7] dma: compile-out DMA sync op calls when not used Alexander Lobakin
2024-01-29 6:11 ` Christoph Hellwig
2024-01-29 11:07 ` Alexander Lobakin
2024-01-29 12:15 ` Robin Murphy
2024-01-31 16:52 ` Simon Horman
2024-01-31 17:14 ` Robin Murphy
2024-01-26 13:54 ` [PATCH net-next 2/7] dma: avoid expensive redundant calls for sync operations Alexander Lobakin
2024-01-26 15:48 ` Robin Murphy [this message]
2024-01-26 16:45 ` Alexander Lobakin
2024-01-26 17:21 ` Robin Murphy
2024-01-26 18:48 ` Petr Tesařík
2024-01-26 19:13 ` Robin Murphy
2024-01-29 6:09 ` Christoph Hellwig
2024-01-29 14:36 ` Alexander Lobakin
2024-01-29 16:15 ` Petr Tesařík
2024-01-29 14:07 ` Alexander Lobakin
2024-01-29 14:29 ` Robin Murphy
2024-01-29 14:34 ` Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 3/7] iommu/dma: avoid expensive indirect " Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 4/7] page_pool: make sure frag API fields don't span between cachelines Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 5/7] page_pool: don't use driver-set flags field directly Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 6/7] page_pool: check for DMA sync shortcut earlier Alexander Lobakin
2024-01-26 13:54 ` [PATCH net-next 7/7] xsk: use generic DMA sync shortcut instead of a custom one Alexander Lobakin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0f6f550c-3eee-46dc-8c42-baceaa237610@arm.com \
--to=robin.murphy@arm.com \
--cc=aleksander.lobakin@intel.com \
--cc=alexanderduyck@fb.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=iommu@lists.linux.dev \
--cc=joro@8bytes.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=maciej.fijalkowski@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=rafael@kernel.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).