From owner-svn-src-all@freebsd.org Fri Jun 8 03:42:59 2018 Return-Path: Delivered-To: svn-src-all@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 29A90100E320; Fri, 8 Jun 2018 03:42:59 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AB8D16FBAA; Fri, 8 Jun 2018 03:42:58 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-io0-x242.google.com with SMTP id e15-v6so14367163iog.1; Thu, 07 Jun 2018 20:42:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=AHR2KZ0fkywotFAz3NDTuq7latJpvMx8jHqJ7JR1qBo=; b=QNvs5MEYiqKd/8U3ULrh0HOydiroKXHEI7HSb067ppnlRjdAup+tcOoMHYV9Yujbye /9twPhf+j7taP+7LeJmerikKtyhXSYgMi8PeMj5qYZ4c3CLPeXx2ruKHTRA6lxmRCAjh GE+p0HfihQsIdPCAuKsgyxtOxbZPZPIOcnIPkBHNjH5eRDdE4Ejo+4GfVyjmd24Jo9MW i5DhYy2+u9MevCCm6d0LII6piEk/Gnxi6mFpG8LqNYTklivtUU7KAICFFW6P9L1xwcVU +iv/+0ok1z8OwKffn5ao4dwNxl1ihf73RKtdfREDzTzvjaSqBSTfGZxXM+QiZy5RrP75 Ig0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=AHR2KZ0fkywotFAz3NDTuq7latJpvMx8jHqJ7JR1qBo=; b=TWQLR/nCsbdTvSl05+sKxObW69Ed2sEnC2NN+VrFpFwiA3/BqAPSdtHNP4Nm/Ung+P 4jlGAmxT3T+TcPvNGPBRCgrqPLYRtbu7smrNHM87mINjFC7lB+RNH3QFo2bvDNBe6K1U dOZ629sTfaLwBjR0FF5VlH3XGpEE+XZ0LNXTuHu9M8ck/WAd0wyq8fqwPgXLKypmlhG1 KG3hmACeddKFMPUDjsDpZF4t0NG27nEeB4PgVdvIkbOhQIoZvV2ZxyLihifEYFr0mvZd Zl3KDSfeMjGaR11hKVNXrkl9XHUjJP7iZj2T/fMqr9YpPrPA++s8GQpl4s0xS3FdiBMc fMpQ== X-Gm-Message-State: APt69E1FyW43RnYx+1d7b+xFWlAi4UxxoqlmDRKEl1cTtONll7DrM3w3 C331FgsEIsePCmaWBOVfLDHqO5X9 X-Google-Smtp-Source: ADUXVKKNfiBljas+y3tk31Bt1u/hMlj5fgIrmLunrN+pNam+Tkmq/EtcaEIgsqXxPHRs8MIsl2VXfQ== X-Received: by 2002:a6b:d00c:: with SMTP id x12-v6mr3880532ioa.5.1528429378073; Thu, 07 Jun 2018 20:42:58 -0700 (PDT) Received: from pesky ([72.142.115.10]) by smtp.gmail.com with ESMTPSA id s126-v6sm297065itc.9.2018.06.07.20.42.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Jun 2018 20:42:57 -0700 (PDT) Sender: Mark Johnston Date: Thu, 7 Jun 2018 23:32:42 -0400 From: Mark Johnston To: Konstantin Belousov Cc: Justin Hibbits , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, rlibby@freebsd.org Subject: Re: svn commit: r334708 - head/sys/kern Message-ID: <20180608033242.GA54099@pesky> References: <201806061257.w56CvCwq089369@repo.freebsd.org> <20180606140311.GU2450@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180606140311.GU2450@kib.kiev.ua> User-Agent: Mutt/1.10.0 (2018-05-17) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Jun 2018 03:42:59 -0000 On Wed, Jun 06, 2018 at 05:03:11PM +0300, Konstantin Belousov wrote: > On Wed, Jun 06, 2018 at 12:57:12PM +0000, Justin Hibbits wrote: > > Author: jhibbits > > Date: Wed Jun 6 12:57:11 2018 > > New Revision: 334708 > > URL: https://svnweb.freebsd.org/changeset/base/334708 > > > > Log: > > Add a memory barrier after taking a reference on the vnode holdcnt in _vhold > > > > This is needed to avoid a race between the VNASSERT() below, and another > > thread updating the VI_FREE flag, on weakly-ordered architectures. > > > > On a 72-thread POWER9, without this barrier a 'make -j72 buildworld' would > > panic on the assert regularly. > > > > It may be possible to use a weaker barrier, and I'll investigate that once > > all stability issues are worked out on POWER9. > > > > Modified: > > head/sys/kern/vfs_subr.c > > > > Modified: head/sys/kern/vfs_subr.c > > ============================================================================== > > --- head/sys/kern/vfs_subr.c Wed Jun 6 10:46:24 2018 (r334707) > > +++ head/sys/kern/vfs_subr.c Wed Jun 6 12:57:11 2018 (r334708) > > @@ -2807,6 +2807,9 @@ _vhold(struct vnode *vp, bool locked) > > CTR2(KTR_VFS, "%s: vp %p", __func__, vp); > > if (!locked) { > > if (refcount_acquire_if_not_zero(&vp->v_holdcnt)) { > > +#if !defined(__amd64__) && !defined(__i386__) > > + mb(); > > +#endif > > VNASSERT((vp->v_iflag & VI_FREE) == 0, vp, > > ("_vhold: vnode with holdcnt is free")); > > return; > First, mb() must not be used in the FreeBSD code at all. > Look at atomic_thread_fenceXXX(9) KPI. > > Second, you need the reciprocal fence between clearing of VI_FREE and > refcount_acquire(), otherwise the added barrier is nop. Most likely, > you got away with it because there is a mutex unlock between clearing > of VI_FREE and acquire, which release semantic you abused. I note that vnlru_free_locked() clears VI_FREE and increments v_holdcnt without an intervening release fence. At this point the caller has not purged the vnode from the name cache, so it seems possible that the panicking thread observed the two stores out of order. In particular, it seems to me that the patch below is necessary, but possibly (probably?) not sufficient: > Does the fence needed for the non-invariants case ? > > Fourth, doesn't v_usecount has the same issues WRT inactivation ? diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c index 286a871c3631..c97a8ba63612 100644 --- a/sys/kern/vfs_subr.c +++ b/sys/kern/vfs_subr.c @@ -1018,6 +1018,7 @@ vnlru_free_locked(int count, struct vfsops *mnt_op) */ freevnodes--; vp->v_iflag &= ~VI_FREE; + atomic_thread_fence_rel(); refcount_acquire(&vp->v_holdcnt); mtx_unlock(&vnode_free_list_mtx); @@ -2807,9 +2808,7 @@ _vhold(struct vnode *vp, bool locked) CTR2(KTR_VFS, "%s: vp %p", __func__, vp); if (!locked) { if (refcount_acquire_if_not_zero(&vp->v_holdcnt)) { -#if !defined(__amd64__) && !defined(__i386__) - mb(); -#endif + atomic_thread_fence_acq(); VNASSERT((vp->v_iflag & VI_FREE) == 0, vp, ("_vhold: vnode with holdcnt is free")); return;