RE: [PATCH v2 1/1] swiotlb: Reduce swiotlb pool lookups

[Michael Kelley <mhklinux@xxxxxxxxxxx>]

From: Christoph Hellwig <hch@xxxxxx> Sent: Friday, July 5, 2024 10:50 PM
> 
> Hi Michael,
> 
> I like the idea behind this, but can you respin it to avoid some of
> the added code duplication.  We have a lot of this pattern:
> 
>       pool = swiotlb_find_pool(dev, paddr);
>       if (pool)
>               swiotlb_foo(dev, ...
> 
> duplicated in all three swiotlb users.  If we rename the original
> swiotlb_foo to __swiotlb_foo and add a little inline wrapper this is
> de-duplicated and also avoids exposing swiotlb_find_pool to the
> callers.

This works pretty well. It certainly avoids the messiness of declaring
a "pool" local variable and needing a separate assignment before the
"if" statement, in each of the 9 call sites. The small downside is that
it looks like a swiotlb function is called every time, even though
there's usually an inline bailout. But that pattern occurs throughout
the kernel, so not a big deal.

I initially coded this change as a separate patch that goes first. But
the second patch ends up changing about 20 lines that are changed
by the first patch. It's hard to cleanly tease them apart. So I've gone
back to a single unified patch. But let me know if you think it's worth
the extra churn to break them apart.

> 
> If we then stub out swiotlb_find_pool to return NULL for !CONFIG_SWIOTLB,
> we also don't need extra stubs for all the __swiotlb_ helpers as the
> compiler will eliminate the calls as dead code.

Yes, this works as long as the declarations for the __swiotlb_foo
functions are *not* under CONFIG_SWIOTLB. But when compiling with
!CONFIG_SWIOTLB on arm64 with gcc-8.5.0, two tangentially related
compile errors occur. iommu_dma_map_page() references
swiotlb_tlb_map_single(). The declaration for the latter is under
CONFIG_SWIOTLB. A similar problem occurs with dma_direct_map_page()
and swiotlb_map(). Do later versions of gcc not complain when the
reference is in dead code? Or are these just bugs that occurred because
!CONFIG_SWIOTLB is rare? If the latter, I can submit a separate patch to
move the declarations out from under CONFIG_SWIOTLB.

> 
> I might be missing something, but what is the reason for using the
> lower-level __swiotlb_find_pool in swiotlb_map and xen_swiotlb_map_page?
> I can't see a reason why the simple checks in swiotlb_find_pool itself
> are either wrong or a performance problem there.  

Yes, swiotlb_find_pool() could be used instead of __swiotlb_find_pool().

> Because if we don't
> need these separate calls we can do away with __swiotlb_find_pool
> for !CONFIG_SWIOTLB_DYNAMIC and simplify swiotlb_find_pool quite
> a bit like this:
> 
>       ...
> 
>       if (!mem)
>               return NULL;
> 
>       if (IS_ENABLED(CONFIG_SWIOTLB_DYNAMIC)) {

The "IS_ENABLED" doesn't work because the dma_uses_io_tlb
field in struct dev is under CONFIG_SWIOTLB_DYNAMIC. I guess
it could be moved out, but that's going further afield. So I'm back
to using #ifdef.

>               smp_rmb();
>               if (!READ_ONCE(dev->dma_uses_io_tlb))
>                       return NULL;
>               return __swiotlb_find_pool(dev, paddr);
>       }
> 
>       if (paddr < mem->defpool.start || paddr >= mem->defpool.end)
>               return NULL;
>       return &dev->dma_io_tlb_mem->defpool;

Petr Tesařík had commented [1] on my original RFC suggesting that
__swiotlb_find_pool() be used here instead of open coding it. With
the changes you suggest, __swiotlb_find_pool() is needed only in
the CONFIG_SWIOTLB_DYNAMIC case, and I would be fine with just
open coding the address of defpool here. Petr -- are you OK with
removing __swiotlb_find_pool when !CONFIG_SWIOTLB_DYNAMIC,
since this is the only place it would be used?

> 
> 
> While you're at it please fix the > 80 character lines as this patch
> is adding plenty.

Many cases already go away with your first suggestion above,
but I'll fix the others.

Michael

[1] 
https://lore.kernel.org/linux-iommu/20240627092049.1dbec746@xxxxxxxxxxxxxxxxxxxx/