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

[Christoph Hellwig <hch@xxxxxx>]

On Tue, Jul 09, 2024 at 11:10:13AM +0200, Petr Tesařík wrote:
> Reviewed-by: Petr Tesarik <petr@xxxxxxxxxxx>

Thanks.

> 
> OK, so __swiotlb_find_pool() is now always declared (so the code
> compiles), but if CONFIG_SWIOTLB_DYNAMIC=n, it is never defined. The
> result still links, because the compiler optimizes away the whole
> if-clause, so there are no references to an undefined symbol in the
> object file.
> 
> I think I've already seen similar constructs elsewhere in the kernel,
> so relying on the optimization seems to be common practice.

Yes, it's a pretty common patter.  It's gone here now, though to not
add the struct device field unconditionally.

> > +{
> > +   struct io_tlb_pool *pool = swiotlb_find_pool(dev, addr);
> > +
> > +   if (unlikely(pool))
> > +           __swiotlb_tbl_unmap_single(dev, addr, size, dir, attrs, pool);
> > +}
> > +
> > +static inline void swiotlb_sync_single_for_device(struct device *dev,
> > +           phys_addr_t addr, size_t size, enum dma_data_direction dir)
> > +{
> > +   struct io_tlb_pool *pool = swiotlb_find_pool(dev, addr);
> > +
> > +   if (unlikely(pool))
> > +           __swiotlb_sync_single_for_device(dev, addr, size, dir, pool);
> 
> We're adding an unlikely() here, which wasn't originally present in
> iommu_dma_sync_single_for_device(). OTOH it should do no harm, and it
> was most likely an omission. 

I'm honestly not a big fan of the unlikely annotations unlike they
are proven to make a difference.  Normally the runtime branch predictor
should do a really good job here, and for some uses this will not
just be likely but the only case.