On Tue, Jul 09, 2024 at 11:10:13AM +0200, Petr Tesařík wrote:
> Reviewed-by: Petr Tesarik <p...@tesarici.cz>
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.
