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

[Petr Tesařík]

On Sun, 7 Jul 2024 02:11:48 +0000
Michael Kelley <mhklinux@xxxxxxxxxxx> wrote:

> 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?

Yes. I have never had strong opinion about it, I merely saw the
opportunity when it was low-hanging fruit, but it's definitely not
worth adding complexity.

Petr T