Re: [PATCH] net: mlx5: Use writeX() to ring doorbell and remove reduntant wmb()

[Liran Alon <liran.alon@oracle.com>]

> On 2 Jan 2020, at 22:58, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> On Thu, Jan 02, 2020 at 09:45:52PM +0200, Liran Alon wrote:
>> 
>> 
>>> On 2 Jan 2020, at 21:29, Jason Gunthorpe <jgg@ziepe.ca> 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