svn commit: r368279 - head/sys/dev/nvme

[Warner Losh]

On Wed, Dec 2, 2020 at 10:48 AM Konstantin Belousov <kostikbel at gmail.com>
wrote:

> On Wed, Dec 02, 2020 at 04:54:24PM +0000, Michal Meloun wrote:
> > Author: mmel
> > Date: Wed Dec  2 16:54:24 2020
> > New Revision: 368279
> > URL: https://svnweb.freebsd.org/changeset/base/368279
> >
> > Log:
> >   NVME: Multiple busdma related fixes.
> ...
>
> >   - in nvme_qpair_submit_tracker(), don't do explicit wmb() also for arm
> >     and arm64. Bus_dmamap_sync() on these architectures is sufficient to
> ensure
> >     that all CPU stores are visible to external (including DMA)
> observers.
> > @@ -982,7 +982,7 @@ nvme_qpair_submit_tracker(struct nvme_qpair *qpair,
> st
> >
> >       bus_dmamap_sync(qpair->dma_tag, qpair->queuemem_map,
> >           BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
> > -#ifndef __powerpc__
> > +#if !defined( __powerpc__) && !defined( __aarch64__)  && !defined(
> __arm__)
> >       /*
> >        * powerpc's bus_dmamap_sync() already includes a heavyweight
> sync, but
> >        * no other archs do.
> Does anybody have any evidence that the wmb() below is useful ?
> For instance, on x86, does nvme driver use any write-combining mappings ?
>

It translates to a sfence on x86. It is done just before the write
that moves the tail pointer. sfence ensures that all writes are done before
that write is done. I believe that the nvme spec requires that the entire
submission queue entry be fully filled out before the write to the tailq
pointer moving it.[*]

I'm not enough of an expert on the exact details here to know if it's
absolutely required or not, but given the ordering requirements in the
spec, the intent appears to be related to enforcing that ordering. I don't
know enough to know if it accomplishes this goal or not.

Warner

[*] I know I said in the review it  may be due to getting lower latency to
the device, but I now believe that to be mistaken after studying it further.