From owner-svn-src-head@freebsd.org Sat Aug 4 13:30:04 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CC13F106B615; Sat, 4 Aug 2018 13:30:04 +0000 (UTC) (envelope-from chmeeedalf@gmail.com) Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4848076E8C; Sat, 4 Aug 2018 13:30:04 +0000 (UTC) (envelope-from chmeeedalf@gmail.com) Received: by mail-lj1-f176.google.com with SMTP id f1-v6so7081316ljc.9; Sat, 04 Aug 2018 06:30:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TMHtv96tmTtFm/sRDodgETRQFo7xuccXDITvZDd9Vyo=; b=G1BisB7EwZ8rTkW8javMB7oNfTmHYINHrJ5uWUuz3ubp5l9R+cAFQxRL8O9/F0G3/D ODe5JGgd8MLVlF9jSVlwOLFIKXLEQP3LO2DXseVd7MhpA5zn2Gm8cbSYM0pdgH/UWdvP 5Rrm49DL3etSHgv17vjesYATpPyZIIuWFdhFftRYbkARd1xNst8u/0IfWwCLFek6sTeF MQgJ2GHyVg4YWNGAeYrfV9oi/RtiimmmXwPnq9AMQlZqOonKqFOAs1pgtY5Lox/x5Vih BYN1uGsXpYUo/fWlART7ojn4/9aAyqeqj/oqajarzONqGehWIvLptiDOT6MPL8um02w/ llYQ== X-Gm-Message-State: AOUpUlHhjHg7AcurwbC2lqTspxqrE0Y9hRYmDYgixzUzvNCj5EEmMaE4 WTu8aLgqqu+TcF1WxQuxxLNejtw6neJoWVUSLMA= X-Google-Smtp-Source: AAOMgpc640xyusMwP/aq3h98cOjZMsmhlGyiPJxwZ4QlyU+fjMQWjW/MkoisLqDKSQxWxtCGhto6+J63qmFEg6qdLaY= X-Received: by 2002:a2e:558c:: with SMTP id g12-v6mr7944443lje.4.1533389397199; Sat, 04 Aug 2018 06:29:57 -0700 (PDT) MIME-Version: 1.0 References: <201808032004.w73K46XJ053249@repo.freebsd.org> <20180804080840.GI6049@kib.kiev.ua> <20180804130315.GK6049@kib.kiev.ua> In-Reply-To: <20180804130315.GK6049@kib.kiev.ua> From: Justin Hibbits Date: Sat, 4 Aug 2018 08:29:47 -0500 Message-ID: Subject: Re: svn commit: r337273 - head/sys/dev/nvme To: Konstantin Belousov Cc: John Baldwin , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.27 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 04 Aug 2018 13:30:05 -0000 On Sat, Aug 4, 2018, 08:03 Konstantin Belousov 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. - Justin >