Messages in this thread | | | Date | Fri, 22 Sep 2023 19:12:13 +0200 | From | Petr Tesařík <> | Subject | Re: [PATCH] swiotlb: fix the check whether a device has used software IO TLB |
| |
Hi Catalin,
On Fri, 22 Sep 2023 15:31:29 +0200 Petr Tesařík <petr@tesarici.cz> wrote:
>[...] > On Mon, 18 Sep 2023 16:45:34 +0100 > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Sun, Sep 17, 2023 at 11:47:41AM +0200, Petr Tesařík wrote: >[...] > > > Ah... You may have a point after all if this sequence of events is > > > possible: > > > > > > - CPU 0 writes new value to mem->pools->next in swiotlb_dyn_alloc(). > > > > > > - CPU 1 observes the new value in swiotlb_find_slots(), even though it > > > is not guaranteed by any barrier, allocates a slot and sets the > > > dev->dma_uses_io_tlb flag. > > > > > > - CPU 1 (driver code) writes the returned buffer address into its > > > private struct. This write is ordered after dev->dma_uses_io_tlb > > > thanks to the smp_wmb() in swiotlb_find_slots(). > > > > > > - CPU 2 (driver code) reads the buffer address, and DMA core passes it > > > to is_swiotlb_buffer(), which contains smp_rmb(). > > > > > > - IIUC CPU 2 is guaranteed to observe the new value of > > > dev->dma_uses_io_tlb, but it may still use the old value of > > > mem->pools->next, because the write on CPU 0 was not ordered > > > against anything. The fact that the new value was observed by CPU 1 > > > does not mean that it is also observed by CPU 2. > > > > Yes, that's possible. On CPU 1 there is a control dependency between the > > read of mem->pools->next and the write of dev->dma_uses_io_tlb but I > > don't think this is sufficient to claim multi-copy atomicity (if CPU 1 > > sees mem->pools->next write by CPU 0, CPU 2 must see it as well), at > > least not on all architectures supported by Linux. memory-barriers.txt > > says that a full barrier on CPU 1 is needed between the read and write, > > i.e. smp_mb() before WRITE_ONCE(dev->dma_uses_io_tlb). You could add it > > just before "goto found" in swiotlb_find_slots() since it's only needed > > on this path. > > Let me check my understanding. This smp_mb() is not needed to make sure > that the write to dev->dma_uses_io_tlb cannot be visible before the > read of mem->pools->next. Since stores are not speculated, that > ordering is provided by the control dependency alone. > > But a general barrier ensures that a third CPU will observe the write to > mem->pools->next after the read of mem->pools->next. Makes sense.
Now that I'm writing the patch, I get your idea to replace WRITE_ONCE() with smp_store_release(). Since a full memory barrier is required for multicopy atomicity, it is not "more than I need". Instead, the ordering contraints may be possibly restricted to "CPUs participating in a release-acquire chain" if I also replace READ_ONCE() in is_swiotlb_buffer() with smp_read_acquire().
I believe it does not matter that the CPU which writes a new value to mem->pools->next in swiotlb_dyn_alloc() does not participate in the chain, because the value must have been visible to the CPU which executes swiotlb_find_slots() and which does participate in the chain.
Let me double-check this thinking with a litmus test.
> I think I can send a v2 of my patch now, with abundant comments on the > memory barriers.
Eh, this must be delayed a bit again...
Petr T
| |