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

[Jason Gunthorpe <jgg@ziepe.ca>]

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?

At least in this mlx5 case there is no WC, right? The kernel UAR is
mapped UC?

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.

> > 
> >> 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://marc.info/?l=linux-netdev&m=157798859215697&w=2
> (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.

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.

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.

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.

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?

> >> 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.

> >> 	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. __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.

> 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.

Jason