Message-ID: <20200103133749.GA9706@ziepe.ca>
Date:   Fri, 3 Jan 2020 09:37:49 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Liran Alon <liran.alon@...cle.com>
Cc:     Will Deacon <will@...nel.org>, saeedm@...lanox.com,
        leon@...nel.org, netdev@...r.kernel.org,
        linux-rdma@...r.kernel.org, eli@...lanox.com, tariqt@...lanox.com,
        danielm@...lanox.com,
        Håkon Bugge <haakon.bugge@...cle.com>
Subject: Re: [PATCH] net: mlx5: Use writeX() to ring doorbell and remove
 reduntant wmb()

On Fri, Jan 03, 2020 at 12:21:06AM +0200, Liran Alon wrote:

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

It is because WC is rarely used and laregly undefined for the kernel
:(

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

The semantic for writel is that prior program order stores will be
observable by DMA from the device receiving the writel. This is
required for UC and NC stores today. WC is undefined, I think.

This is why ARM has the additional barrier in writel.

It would logically make sense if WC followed the same rule, however,
adding a barrier to writel to make WC ordered would not be popular, so
I think we are left with using special accessors for WC and placing
the barrier there..

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

Is it a real bug that people actually hit? It wasn't clear from the
commit message. If so, sure, it should be fixed and the commit message
clarified. (but I'd put the wmb near the WC writes..)

I am surprised that AMD is different here, the evolution of the WC
feature on x86 was to transparently speed up graphics, so I'm pretty
surprised AMD can get away with not ordering the same as Intel..

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

The barrier macros have been unpopular, ie the confusing mmiowb was
dumped, and I strongly suspect to contain WC within a spinlock (which
is very important!) you need a barrier instruction on some archs.

New accessors might work better.

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

Hmm, haven't ever seen it documented like that. It is sort of a
logical requirement. If writel(x) produces different TLPs (ie
different byte order) how could a driver ever work with a PCI-E device
that requires only one TLP for x?

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

Yes, this is what I did to userspace and it made this all make
sense. I strongly recommend to do the same in the kernel, particularly
now that we have iowrite64be()!

Jason

Powered by blists - more mailing lists