[PATCH] swiotlb: add the missing dynamic swiotlb check in is_swiotlb_buffer

[PATCH] swiotlb: add the missing dynamic swiotlb check in is_swiotlb_buffer

[Christoph Hellwig]

Commit 1395706a1490 ("swiotlb: search the software IO TLB only if the
device makes use of it") claims to avoid the dynamic swiotlb lookup
unless dynamic allocations are actually used.  And while it adds all
the infrastructure for doing that, it misses the final bit of actually
checking the flag.  Actually add the check for this functionality to
work.

Fixes: 1395706a1490 ("swiotlb: search the software IO TLB only if the device makes use of it")
Reported-by: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/swiotlb.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index b4536626f8ff35..a188025389836b 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -172,13 +172,17 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
 	if (!mem)
 		return false;
 
-	if (IS_ENABLED(CONFIG_SWIOTLB_DYNAMIC)) {
-		/* Pairs with smp_wmb() in swiotlb_find_slots() and
+#ifdef CONFIG_SWIOTLB_DYNAMIC
+	if (dev->dma_uses_io_tlb) {
+		/*
+		 * Pairs with smp_wmb() in swiotlb_find_slots() and
 		 * swiotlb_dyn_alloc(), which modify the RCU lists.
 		 */
 		smp_rmb();
 		return swiotlb_find_pool(dev, paddr);
 	}
+#endif
+
 	return paddr >= mem->defpool.start && paddr < mem->defpool.end;
 }
 
-- 
2.39.2

Re: [PATCH] swiotlb: add the missing dynamic swiotlb check in is_swiotlb_buffer

[Petr Tesařík]

On Tue,  5 Sep 2023 08:44:41 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Commit 1395706a1490 ("swiotlb: search the software IO TLB only if the
> device makes use of it") claims to avoid the dynamic swiotlb lookup
> unless dynamic allocations are actually used.  And while it adds all
> the infrastructure for doing that, it misses the final bit of actually
> checking the flag.  Actually add the check for this functionality to
> work.
> 
> Fixes: 1395706a1490 ("swiotlb: search the software IO TLB only if the device makes use of it")
> Reported-by: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/swiotlb.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index b4536626f8ff35..a188025389836b 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -172,13 +172,17 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
>  	if (!mem)
>  		return false;
>  
> -	if (IS_ENABLED(CONFIG_SWIOTLB_DYNAMIC)) {
> -		/* Pairs with smp_wmb() in swiotlb_find_slots() and
> +#ifdef CONFIG_SWIOTLB_DYNAMIC
> +	if (dev->dma_uses_io_tlb) {
> +		/*
> +		 * Pairs with smp_wmb() in swiotlb_find_slots() and
>  		 * swiotlb_dyn_alloc(), which modify the RCU lists.
>  		 */
>  		smp_rmb();
>  		return swiotlb_find_pool(dev, paddr);
>  	}
> +#endif
> +

Not quite. This code is not SMP safe. I believe that:

1. the value must be read with a READ_ONCE(),
2. the memory barrier must precede the read operation.

Do you want to send a v2, or should I send my version instead?

Petr T

Re: [PATCH] swiotlb: add the missing dynamic swiotlb check in is_swiotlb_buffer

[Christoph Hellwig]

On Fri, Sep 08, 2023 at 02:44:18PM +0200, Petr Tesařík wrote:
> 
> Not quite. This code is not SMP safe. I believe that:
> 
> 1. the value must be read with a READ_ONCE(),
> 2. the memory barrier must precede the read operation.
> 
> Do you want to send a v2, or should I send my version instead?

As I'm travelling right now I'd love you to take care of it.  It's
probably a new thing and not a v2 so feel free to take full credit.

Re: [PATCH] swiotlb: add the missing dynamic swiotlb check in is_swiotlb_buffer

[Petr Tesařík]

On Sat, 9 Sep 2023 10:40:45 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Fri, Sep 08, 2023 at 02:44:18PM +0200, Petr Tesařík wrote:
> > 
> > Not quite. This code is not SMP safe. I believe that:
> > 
> > 1. the value must be read with a READ_ONCE(),
> > 2. the memory barrier must precede the read operation.
> > 
> > Do you want to send a v2, or should I send my version instead?  
> 
> As I'm travelling right now I'd love you to take care of it.  It's
> probably a new thing and not a v2 so feel free to take full credit.

I'm sorry it took a bit longer than anticipated. I unexpectedly ran
into an apparent performance regression, but all looks good after I
fixed my test setup to compare apples with apples and oranges with
oranges. ;-)

Patch coming soon.

Petr T