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

[Christoph Hellwig <hch@xxxxxx>]

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.

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.

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.  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)) {
                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;


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