Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 4 Aug 2018 10:20:50 -0500
From:      Justin Hibbits <jhibbits@freebsd.org>
To:        Ian Lepore <ian@freebsd.org>
Cc:        Konstantin Belousov <kostikbel@gmail.com>, John Baldwin <jhb@freebsd.org>,  src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r337273 - head/sys/dev/nvme
Message-ID:  <CAHSQbTCztx1HQr6CuV2TWwjpzt3wH8pSuUv6gZb3nR3MW24d=g@mail.gmail.com>
In-Reply-To: <1533395475.9860.9.camel@freebsd.org>
References:  <201808032004.w73K46XJ053249@repo.freebsd.org> <20180804080840.GI6049@kib.kiev.ua> <c931140b-0334-de19-35f1-5b00cd10c2d9@FreeBSD.org> <20180804130315.GK6049@kib.kiev.ua> <CAHSQbTCE%2Bbngg0NpDB38b3HBH6zugykt9U4mFXNYJTSYHD4asQ@mail.gmail.com> <1533395475.9860.9.camel@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Aug 4, 2018, 10:11 Ian Lepore <ian@freebsd.org> wrote:

> On Sat, 2018-08-04 at 08:29 -0500, Justin Hibbits wrote:
> > On Sat, Aug 4, 2018, 08:03 Konstantin Belousov <kostikbel@gmail.com>
> wrote:
> >
> > >
> > > On Sat, Aug 04, 2018 at 05:14:31AM -0700, John Baldwin wrote:
> > > >
> > > > On 8/4/18 1:08 AM, Konstantin Belousov wrote:
> > > > >
> > > > > On Fri, Aug 03, 2018 at 08:04:06PM +0000, Justin Hibbits wrote:
> > > > > >
> > > > > > Author: jhibbits
> > > > > > Date: Fri Aug  3 20:04:06 2018
> > > > > > New Revision: 337273
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/337273
> > > > > >
> > > > > > Log:
> > > > > >   nvme(4): Add bus_dmamap_sync() at the end of the request path
> > > > > >
> > > > > >   Summary:
> > > > > >   Some architectures, in this case powerpc64, need explicit
> > > synchronization
> > > >
> > > > >
> > > > > >
> > > > > >   barriers vs device accesses.
> > > > > >
> > > > > >   Prior to this change, when running 'make buildworld -j72' on a
> > > 18-core
> > > >
> > > > >
> > > > > >
> > > > > >   (72-thread) POWER9, I would see controller resets often.  With
> this
> > > change, I
> > > >
> > > > >
> > > > > >
> > > > > >   don't see these resets messages, though another tester still
> does,
> > > for yet to be
> > > >
> > > > >
> > > > > >
> > > > > >   determined reasons, so this may not be a complete fix.
> > > Additionally, I see a
> > > >
> > > > >
> > > > > >
> > > > > >   ~5-10% speed up in buildworld times, likely due to not needing
> to
> > > reset the
> > > >
> > > > >
> > > > > >
> > > > > >   controller.
> > > > > >
> > > > > >   Reviewed By: jimharris
> > > > > >   Differential Revision: https://reviews.freebsd.org/D16570
> > > > > >
> > > > > > Modified:
> > > > > >   head/sys/dev/nvme/nvme_qpair.c
> > > > > >
> > > > > > Modified: head/sys/dev/nvme/nvme_qpair.c
> > > > > >
> > >
> ==============================================================================
> > > >
> > > > >
> > > > > >
> > > > > > --- head/sys/dev/nvme/nvme_qpair.c Fri Aug  3 19:24:04 2018
> > > (r337272)
> > > >
> > > > >
> > > > > >
> > > > > > +++ head/sys/dev/nvme/nvme_qpair.c Fri Aug  3 20:04:06 2018
> > > (r337273)
> > > >
> > > > >
> > > > > >
> > > > > > @@ -401,9 +401,13 @@ nvme_qpair_complete_tracker(struct
> nvme_qpair
> > > *qpair,
> > > >
> > > > >
> > > > > >
> > > > > >            req->retries++;
> > > > > >            nvme_qpair_submit_tracker(qpair, tr);
> > > > > >    } else {
> > > > > > -          if (req->type != NVME_REQUEST_NULL)
> > > > > > +          if (req->type != NVME_REQUEST_NULL) {
> > > > > > +                  bus_dmamap_sync(qpair->dma_tag_payload,
> > > > > > +                      tr->payload_dma_map,
> > > > > > +                      BUS_DMASYNC_POSTREAD |
> BUS_DMASYNC_POSTWRITE);
> > > > > >                    bus_dmamap_unload(qpair->dma_tag_payload,
> > > > > >                        tr->payload_dma_map);
> > > > > > +          }
> > > > > >
> > > > > >            nvme_free_request(req);
> > > > > >            tr->req = NULL;
> > > > > > @@ -487,6 +491,8 @@ nvme_qpair_process_completions(struct
> nvme_qpair
> > > *qpai
> > > >
> > > > >
> > > > > >
> > > > > >             */
> > > > > >            return (false);
> > > > > >
> > > > > > +  bus_dmamap_sync(qpair->dma_tag, qpair->queuemem_map,
> > > > > > +      BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
> > > > > >    while (1) {
> > > > > >            cpl = qpair->cpl[qpair->cq_head];
> > > > > >
> > > > > > @@ -828,7 +834,16 @@ nvme_qpair_submit_tracker(struct nvme_qpair
> > > *qpair, st
> > > >
> > > > >
> > > > > >
> > > > > >    if (++qpair->sq_tail == qpair->num_entries)
> > > > > >            qpair->sq_tail = 0;
> > > > > >
> > > > > > +  bus_dmamap_sync(qpair->dma_tag, qpair->queuemem_map,
> > > > > > +      BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
> > > > > > +#ifndef __powerpc__
> > > > > > +  /*
> > > > > > +   * powerpc's bus_dmamap_sync() already includes a heavyweight
> > > sync, but
> > > >
> > > > >
> > > > > >
> > > > > > +   * no other archs do.
> > > > > > +   */
> > > > > >    wmb();
> > > > > > +#endif
> > > > > What is the purpose of this call ?  It is useless without paired
> read
> > > > > barrier.  So where is the reciprocal rmb() ?
> > > > For DMA, the rmb is in the device controller.  However, architectures
> > > > that need this kind of ordering should do it in their bus_dmmap_sync
> op,
> > > > and this explicit one needs to be removed.  (Alpha had a wmb in its
> > > > bus_dmamap_sync op for this reason.)
> > > Yes, if something special is needed, it should happen in
> platform-specific
> > > busdma code.
> > >
> > > Also, if wmb() is needed, then it is not a supposed semantic or
> > > wmb(), but a specific side-effects of one of the instruction in the
> > > implementation of wmb().
> > >
> > > As I noted, on x86 it is not needed and detrimental to the performance.
> > >
> > According to Jim Harris it is needed for x86. I tried to remove it
> thinking
> > it was unnecessary with the sync in place now.  The wmb() was added way
> > back in r240616 by him, when only x86 was supported by the driver.
> > bus_dmamap_sync() for all archs except powerpc lack a barrier, from what
> I
> > can see.  See the review for more discussion that took place. If it isn't
> > needed I will gladly remove it.
> >
>
> The arm busdma sync routines definitely invoke the necessary barriers,
> which on arm are spelled 'dsb' and 'isb' and are in the low-level
> routines called from the busdma code, such as dcache_wb_pou(),
> dcache_inv_poc(), etc.
>
> -- Ian
>

You're right, I missed that when I searched. X86 does not, it appears, even
in the case of needing bounce buffers.  The wmb() had been in since the
beginning. Someone with x86 hardware should test before it's removed. I
have none myself.

- Justin

>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTCztx1HQr6CuV2TWwjpzt3wH8pSuUv6gZb3nR3MW24d=g>