> On 2 Jan 2020, at 22:58, Jason Gunthorpe <jgg@xxxxxxxx> wrote: > > On Thu, Jan 02, 2020 at 09:45:52PM +0200, Liran Alon wrote: >> >> >>> On 2 Jan 2020, at 21:29, Jason Gunthorpe <jgg@xxxxxxxx> wrote: >>> >>> On Thu, Jan 02, 2020 at 07:44:36PM +0200, Liran Alon wrote: >>>> Currently, mlx5e_notify_hw() executes wmb() to complete writes to cache-coherent >>>> memory before ringing doorbell. Doorbell is written to by mlx5_write64() >>>> which use __raw_writeX(). >>>> >>>> This is semantically correct but executes reduntant wmb() in some architectures. >>>> For example, in x86, a write to UC memory guarantees that any previous write to >>>> WB memory will be globally visible before the write to UC memory. Therefore, there >>>> is no need to also execute wmb() before write to doorbell which is mapped as UC memory. >>>> >>>> The consideration regarding this between different architectures is handled >>>> properly by the writeX() macro. Which is defined differently for different >>>> architectures. E.g. On x86, it is just a memory write. However, on ARM, it >>>> is defined as __iowmb() folowed by a memory write. __iowmb() is defined >>>> as wmb(). >>> >>> This reasoning seems correct, though I would recommend directly >>> refering to locking/memory-barriers.txt which explains this. >> >> I find memory-barriers.txt not explicit enough on the semantics of writeX(). >> (For example: Should it flush write-combined buffers before writing to the UC memory?) >> That’s why I preferred to explicitly state here how I perceive it. > > AFAIK WC is largely unspecified by the memory model. Is wmb() even > formally specified to interact with WC? As I said, I haven’t seen such semantics defined in kernel documentation such as memory-barriers.txt. However, in practice, it does flush WC buffers. At least for x86 and ARM which I’m familiar enough with. I think it’s reasonable to assume that wmb() should flush WC buffers while dma_wmb()/smp_wmb() doesn’t necessarily have to do this. But this is exactly the types of things that bothered me with memory-barriers.txt. That it doesn’t define semantics regarding these. > > At least in this mlx5 case there is no WC, right? Right. As I made sure to explicitly mention in commit message. This is also why I haven’t yet also fixed a similar issue in mlx4 kernel driver that does use BlueFlame (WC memory). > The kernel UAR is > mapped UC? Yes. > > So we don't need to worry about the poor specification of WC access > and you can refer to memory-barriers.txt at least for this patch. Right. I just gave it as a general example about that I wasn’t personally able to understand the exact intended semantics of writeX() by just reading memory-barriers.txt. Anyway, sure I will rephrase the commit message to refer to memory-barriers.txt. No objection there. > >>> >>>> Therefore, change mlx5_write64() to use writeX() and remove wmb() from >>>> it's callers. >>> >>> Yes, wmb(); writel(); is always redundant >> >> Well, unfortunately not… >> See: https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dnetdev-26m-3D157798859215697-26w-3D2&d=DwIDaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=Ox1lCS1KAGBvJrf24kiFQrranIaNi_zeo05sqCUEf7Y&s=Mz6MJzUQ862DGjgGnj3neX4ZpjI88nOI9KpZhNF9TqQ&e= >> (See my suggestion to add flush_wc_writeX()) > > Well, the last time wmb & writel came up Linus was pretty clear that > writel is supposed to remain in program order and have the barriers > needed to do that. Right. But that doesn’t take into account that WC writes are considered completed when they are still posted in CPU WC buffers. The semantics as I understand of writeX() is that it guarantees all prior writes have been completed. It means that all prior stores have executed and that store-buffer is flushed. But it doesn’t mean that WC buffers is flushed as-well. > > I don't think WC was considered when that discussion happened, but we > really don't have a formal model for how WC works at all within the > kernel. Agree. I kinda assumed that wmb() have an extra semantics of also guaranteeing that WC buffers are flushed. (Because in practice, I think it’s true for all cases. In contrast to using dma_wmb()/smp_wmb()). This is why I think we are missing a flush_wc_writeX() macro as I specified in that commit message. > > The above patch is really not a wmb(); writel() pairing, the wmb() is > actually closing/serializing an earlier WC transaction, and yes you need various > special things to keep WC working right. Right. > > IMHO you should start there before going around and adding/removing wmbs > related to WC. Update membory-barriers.txt and related with the model > ordering for WC access and get agreement. I disagree here. It’s more important to fix a real bug (e.g. Not flushing WC buffers on x86 AMD). Then, we can later formalise this and refactor code as necessary. Which will also optimise it as-well. Bug fix can be merged before we finish all these discussions and get agreement. I do completely agree we should have this discussion on WC and barriers and I already sent an email on this to all the memory-barriers.txt maintainers. Waiting to see how that discussion go and get community feedback before I will submit a patch-series that will introduce new changes to memory-barriers.txt and probably also new barrier macro. > > For instance does wmb() even effect WC? Does WC have to be contained > by spinlocks? Do we need extra special barriers like flush_wc and > flush_wc_before_spin_unlock ? etc. > > Perhaps Will has some advice? I also sent Will an email about this a few days ago. Thanks for Cc him. :) > >>>> diff --git a/include/linux/mlx5/cq.h b/include/linux/mlx5/cq.h >>>> index 40748fc1b11b..28744a725e64 100644 >>>> +++ b/include/linux/mlx5/cq.h >>>> @@ -162,11 +162,6 @@ static inline void mlx5_cq_arm(struct mlx5_core_cq *cq, u32 cmd, >>>> >>>> *cq->arm_db = cpu_to_be32(sn << 28 | cmd | ci); >>>> >>>> - /* Make sure that the doorbell record in host memory is >>>> - * written before ringing the doorbell via PCI MMIO. >>>> - */ >>>> - wmb(); >>>> - >>> >>> Why did this one change? The doorbell memory here is not a writel(): >> >> Well, it’s not seen in the diff but actually the full code is: >> >> /* Make sure that the doorbell record in host memory is >> * written before ringing the doorbell via PCI MMIO. >> */ >> wmb(); >> >> doorbell[0] = cpu_to_be32(sn << 28 | cmd | ci); >> doorbell[1] = cpu_to_be32(cq->cqn); >> >> mlx5_write64(doorbell, uar_page + MLX5_CQ_DOORBELL); > > Ah OK, we have another thing called doorbell which is actually DMA'ble > memory. Yep. > >>>> doorbell[0] = cpu_to_be32(sn << 28 | cmd | ci); >>>> doorbell[1] = cpu_to_be32(cq->cqn); >>> >>>> static inline void mlx5_write64(__be32 val[2], void __iomem *dest) >>>> { >>>> #if BITS_PER_LONG == 64 >>>> - __raw_writeq(*(u64 *)val, dest); >>>> + writeq(*(u64 *)val, dest); >>> >>> I want to say this might cause problems with endian swapping as writeq >>> also does some swaps that __raw does not? Is this true? >> >> Hmm... Looking at ARM64 version, writeq() indeed calls cpu_to_le64() >> on parameter before passing it to __raw_writeq(). Quite surprising >> from API perspective to be honest. > > For PCI-E devices writel(x) is defined to generate the same TLP on the > PCI-E bus, across all arches. Good to know. Question: Where is this documented? > __raw_* does something arch specific and > should not be called from drivers. It is a long standing bug that this > code is written like this. Agree. That’s what caught my eye to this in the first place. > >> So should I change this instead to iowrite64be(*(u64 *)val, dest)? > > This always made my head hurt, but IIRC, when I looked at it years ago > the weird array construction caused problems with that simple conversion. > > The userspace version looks like this now: > > uint64_t doorbell; > uint32_t sn; > uint32_t ci; > uint32_t cmd; > > sn = cq->arm_sn & 3; > ci = cq->cons_index & 0xffffff; > cmd = solicited ? MLX5_CQ_DB_REQ_NOT_SOL : MLX5_CQ_DB_REQ_NOT; > > doorbell = sn << 28 | cmd | ci; > doorbell <<= 32; > doorbell |= cq->cqn; > > mmio_write64_be(ctx->uar[0].reg + MLX5_CQ_DOORBELL, htobe64(doorbell)); > > Where on all supported platforms the mmio_write64_be() expands to a > simple store (no swap) > > Which does look functionally the same as > > iowrite64be(doorbell, dest); > > So this patch should change the mlx5_write64 to accept a u64 like we > did in userspace when this was all cleaned there. If I understand you correctly, you suggest to change callers to pass here a standard u64 and then modify mlx5_write64() to just call iowrite64be(). If so, I agree. Just want to confirm before sending v2. Thanks for the insightful review, -Liran > > Jason
