Re: [PATCH] swiotlb: fix the check whether a device has used software IO TLB

[Catalin Marinas]

On Wed, Sep 13, 2023 at 02:26:56PM +0200, Petr Tesařík wrote:
> On Wed, 13 Sep 2023 14:14:03 +0200
> Christoph Hellwig <hch@xxxxxx> wrote:
> > Thanks, I've applied this to get it into linux-next ASAP.  I'd love
> > to have reviews before sending it on to Linus, though.
> 
> Fully understood, given my past record. ;-)
> 
> @Catalin Marinas: I have added you to the list of recipient, because you
> spotted some issues with memory barriers in one of my previous attempts.

I seem to have forgotten everything about that thread (trying to
remember what I meant in
https://lore.kernel.org/all/ZGJdtmP13pv06xDH@xxxxxxx/ ;)).

What do the smp_wmb() barriers in swiotlb_find_slots() and
swiotlb_dyn_alloc() order? The latter is even more unclear as it's at
the end of the function and the "pairing" comment doesn't help.

I found the "pairing" description in memory-barriers.txt not helping
much and lots of code comments end up stating that some barrier is
paired with another as if it is some magic synchronisation event. In
general a wmb() barrier ends up paired with an rmb() one (or some other
form of dependency, acquire semantics etc.) but they should be described
in isolation first - what memory accesses on a CPU before and after the
*mb() need ordering - and then how this pairing solves the accesses
observability across multiple CPUs. The comments should also describe
things like "ensure payload is visible before flag update" rather than
"the wmb here pairs with the rmb over there".

Presumably in swiotlb_find_slots(), the smp_wmb() needs to ensure the
write to dev->dma_uses_io_tlb is visible before the *retpool write? For
swiotlb_dyn_alloc(), I'm completely lost on what the smp_wmb() orders.
You really need at least two memory accesses on a CPU for the barrier to
make sense.

-- 
Catalin