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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240709111812.GB4421@lst.de>
Date: Tue, 9 Jul 2024 13:18:12 +0200
From: Christoph Hellwig <hch@....de>
To: Petr Tesařík <petr@...arici.cz>
Cc: mhkelley58@...il.com, mhklinux@...look.com, robin.murphy@....com,
	joro@...tes.org, will@...nel.org, jgross@...e.com,
	sstabellini@...nel.org, oleksandr_tyshchenko@...m.com, hch@....de,
	m.szyprowski@...sung.com, iommu@...ts.linux.dev,
	linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org
Subject: Re: [PATCH v3 1/1] swiotlb: Reduce swiotlb pool lookups

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