1. Patchew
  2. linux
  3. [PATCH v1 00/17] Provide a new two step DMA mapping API
  4. View patch

[PATCH v1 08/17] dma-mapping: add a dma_need_unmap helper

Leon Romanovsky posted 17 patches 3 weeks, 4 days ago
There is a newer version of this series
[PATCH v1 08/17] dma-mapping: add a dma_need_unmap helper
Posted by Leon Romanovsky 3 weeks, 4 days ago 
From: Christoph Hellwig <hch@lst.de>

Add helper that allows a driver to skip calling dma_unmap_*
if the DMA layer can guarantee that they are no-nops.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/dma-mapping.h |  5 +++++
 kernel/dma/mapping.c        | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 8074a3b5c807..6906edde505d 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -410,6 +410,7 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
 {
 	return dma_dev_need_sync(dev) ? __dma_need_sync(dev, dma_addr) : false;
 }
+bool dma_need_unmap(struct device *dev);
 #else /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
 static inline bool dma_dev_need_sync(const struct device *dev)
 {
@@ -435,6 +436,10 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
 {
 	return false;
 }
+static inline bool dma_need_unmap(struct device *dev)
+{
+	return false;
+}
 #endif /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
 
 struct page *dma_alloc_pages(struct device *dev, size_t size,
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 864a1121bf08..daa97a650778 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -442,6 +442,26 @@ bool __dma_need_sync(struct device *dev, dma_addr_t dma_addr)
 }
 EXPORT_SYMBOL_GPL(__dma_need_sync);
 
+/**
+ * dma_need_unmap - does this device need dma_unmap_* operations
+ * @dev: device to check
+ *
+ * If this function returns %false, drivers can skip calling dma_unmap_* after
+ * finishing an I/O.  This function must be called after all mappings that might
+ * need to be unmapped have been performed.
+ */
+bool dma_need_unmap(struct device *dev)
+{
+	if (!dma_map_direct(dev, get_dma_ops(dev)))
+		return true;
+#ifdef CONFIG_DMA_NEED_SYNC
+	if (!dev->dma_skip_sync)
+		return true;
+#endif
+	return IS_ENABLED(CONFIG_DMA_API_DEBUG);
+}
+EXPORT_SYMBOL_GPL(dma_need_unmap);
+
 static void dma_setup_need_sync(struct device *dev)
 {
 	const struct dma_map_ops *ops = get_dma_ops(dev);
-- 
2.46.2
Re: [PATCH v1 08/17] dma-mapping: add a dma_need_unmap helper
Posted by Robin Murphy 3 weeks, 3 days ago 
On 30/10/2024 3:12 pm, Leon Romanovsky wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> Add helper that allows a driver to skip calling dma_unmap_*
> if the DMA layer can guarantee that they are no-nops.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>   include/linux/dma-mapping.h |  5 +++++
>   kernel/dma/mapping.c        | 20 ++++++++++++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 8074a3b5c807..6906edde505d 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -410,6 +410,7 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
>   {
>   	return dma_dev_need_sync(dev) ? __dma_need_sync(dev, dma_addr) : false;
>   }
> +bool dma_need_unmap(struct device *dev);
>   #else /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
>   static inline bool dma_dev_need_sync(const struct device *dev)
>   {
> @@ -435,6 +436,10 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
>   {
>   	return false;
>   }
> +static inline bool dma_need_unmap(struct device *dev)
> +{
> +	return false;
> +}
>   #endif /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
>   
>   struct page *dma_alloc_pages(struct device *dev, size_t size,
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 864a1121bf08..daa97a650778 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -442,6 +442,26 @@ bool __dma_need_sync(struct device *dev, dma_addr_t dma_addr)
>   }
>   EXPORT_SYMBOL_GPL(__dma_need_sync);
>   
> +/**
> + * dma_need_unmap - does this device need dma_unmap_* operations
> + * @dev: device to check
> + *
> + * If this function returns %false, drivers can skip calling dma_unmap_* after
> + * finishing an I/O.  This function must be called after all mappings that might
> + * need to be unmapped have been performed.

In terms of the unmap call itself, why don't we just use dma_skip_sync 
to short-cut dma_direct_unmap_*() and make sure it's as cheap as possible?

In terms of not having to unmap implying not having to store addresses 
at all, it doesn't seem super-useful when you still have to store them 
for long enough to find out that you don't :/

Thanks,
Robin.

> + */
> +bool dma_need_unmap(struct device *dev)
> +{
> +	if (!dma_map_direct(dev, get_dma_ops(dev)))
> +		return true;
> +#ifdef CONFIG_DMA_NEED_SYNC
> +	if (!dev->dma_skip_sync)
> +		return true;
> +#endif
> +	return IS_ENABLED(CONFIG_DMA_API_DEBUG);
> +}
> +EXPORT_SYMBOL_GPL(dma_need_unmap);
> +
>   static void dma_setup_need_sync(struct device *dev)
>   {
>   	const struct dma_map_ops *ops = get_dma_ops(dev);
Re: [PATCH v1 08/17] dma-mapping: add a dma_need_unmap helper
Posted by Christoph Hellwig 2 weeks, 6 days ago 
On Thu, Oct 31, 2024 at 09:18:11PM +0000, Robin Murphy wrote:
>>   +/**
>> + * dma_need_unmap - does this device need dma_unmap_* operations
>> + * @dev: device to check
>> + *
>> + * If this function returns %false, drivers can skip calling dma_unmap_* after
>> + * finishing an I/O.  This function must be called after all mappings that might
>> + * need to be unmapped have been performed.
>
> In terms of the unmap call itself, why don't we just use dma_skip_sync to 
> short-cut dma_direct_unmap_*() and make sure it's as cheap as possible?
>
> In terms of not having to unmap implying not having to store addresses at 
> all, it doesn't seem super-useful when you still have to store them for 
> long enough to find out that you don't :/

I don't fully understand the comment, mostly because the way I read the
two sentences appear to contradict each other.

Bypassing dma_direct_unmap_ is not the important part, because it already
is pretty cheap.  Storing the addresses is not.

That being said now that we never check need_unmap in the iova path
it might make sense to not have a separate helper, but it needs to
be exposed and documented.
Re: [PATCH v1 08/17] dma-mapping: add a dma_need_unmap helper
Posted by Leon Romanovsky 3 weeks, 2 days ago 
On Thu, Oct 31, 2024 at 09:18:11PM +0000, Robin Murphy wrote:
> On 30/10/2024 3:12 pm, Leon Romanovsky wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > Add helper that allows a driver to skip calling dma_unmap_*
> > if the DMA layer can guarantee that they are no-nops.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >   include/linux/dma-mapping.h |  5 +++++
> >   kernel/dma/mapping.c        | 20 ++++++++++++++++++++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 8074a3b5c807..6906edde505d 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -410,6 +410,7 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
> >   {
> >   	return dma_dev_need_sync(dev) ? __dma_need_sync(dev, dma_addr) : false;
> >   }
> > +bool dma_need_unmap(struct device *dev);
> >   #else /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
> >   static inline bool dma_dev_need_sync(const struct device *dev)
> >   {
> > @@ -435,6 +436,10 @@ static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
> >   {
> >   	return false;
> >   }
> > +static inline bool dma_need_unmap(struct device *dev)
> > +{
> > +	return false;
> > +}
> >   #endif /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
> >   struct page *dma_alloc_pages(struct device *dev, size_t size,
> > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> > index 864a1121bf08..daa97a650778 100644
> > --- a/kernel/dma/mapping.c
> > +++ b/kernel/dma/mapping.c
> > @@ -442,6 +442,26 @@ bool __dma_need_sync(struct device *dev, dma_addr_t dma_addr)
> >   }
> >   EXPORT_SYMBOL_GPL(__dma_need_sync);
> > +/**
> > + * dma_need_unmap - does this device need dma_unmap_* operations
> > + * @dev: device to check
> > + *
> > + * If this function returns %false, drivers can skip calling dma_unmap_* after
> > + * finishing an I/O.  This function must be called after all mappings that might
> > + * need to be unmapped have been performed.
> 
> In terms of the unmap call itself, why don't we just use dma_skip_sync to
> short-cut dma_direct_unmap_*() and make sure it's as cheap as possible?

From what I see dma_skip_sync is not available when kernel is built
without CONFIG_DMA_NEED_SYNC.

> 
> In terms of not having to unmap implying not having to store addresses at
> all, it doesn't seem super-useful when you still have to store them for long
> enough to find out that you don't :/

Why? The decision if DMA addresses are needed is taken when allocating
relevant arrays, before we have any DMA address to store. If we know
that we don't need to unmap, we can skip allocation of the array for
free. So what and when "you still have to store them"?

Thanks

> 
> Thanks,
> Robin.
> 
> > + */
> > +bool dma_need_unmap(struct device *dev)
> > +{
> > +	if (!dma_map_direct(dev, get_dma_ops(dev)))
> > +		return true;
> > +#ifdef CONFIG_DMA_NEED_SYNC
> > +	if (!dev->dma_skip_sync)
> > +		return true;
> > +#endif
> > +	return IS_ENABLED(CONFIG_DMA_API_DEBUG);
> > +}
> > +EXPORT_SYMBOL_GPL(dma_need_unmap);
> > +
> >   static void dma_setup_need_sync(struct device *dev)
> >   {
> >   	const struct dma_map_ops *ops = get_dma_ops(dev);
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.