From owner-svn-src-all@freebsd.org Fri Jun 8 04:36:31 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 D497B100F96D; Fri, 8 Jun 2018 04:36:30 +0000 (UTC) (envelope-from rlibby@gmail.com) Received: from mail-pf0-f196.google.com (mail-pf0-f196.google.com [209.85.192.196]) (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 5F64771985; Fri, 8 Jun 2018 04:36:30 +0000 (UTC) (envelope-from rlibby@gmail.com) Received: by mail-pf0-f196.google.com with SMTP id q1-v6so5972245pff.13; Thu, 07 Jun 2018 21:36:30 -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:in-reply-to:references:from:date :message-id:subject:to:cc; bh=eWr+q89s0juPBIN1Pvdeheg2vsdFk8HUypAsXVFxk4A=; b=K7Xjv+v3d8E4MM0Tio/5+cuMy3gVE92+AVjroQkQkdIsNLUor9DOm9W4AKmjKwfWah KBbk3+TdWcTv0GRKvbLakl8an37npxbOb2SrKb4j7jUeD16qTrEJtEPc1y/fODtHZFWZ 20ZW3KraJ2dzEENr+7A2xdCDJulOkXxSnFgc2xgYFQ9X8dESkMh5jSVD2UvnNPL15aWt sAIjvS65cyS6pN/yv/1Ex1H3GmwgTg+oA/i/sjCvE/uWlyI+FT2eQRTdzuaZJfg9kL1w It+wHny6M5ohj+d+BaiTvdTzfdKQlHMV8cxa3nQOJ+HGPphZPeY2EuT+PUwNnAIjJpCW L5sw== X-Gm-Message-State: APt69E3w8+oJi0KKzvTNgDvpk5VQE810DKAVt2FN9mOumcooISkZzjdv J/092pHS7cdQneussWno4t/3X1hmHFc= X-Google-Smtp-Source: ADUXVKLCbCTyQFPkezuo7Y6YqJanZ1aI5FfNLefkvLbKSDB39HleHi1kCHQtAKskk4PT/auZKVEznQ== X-Received: by 2002:a62:1bc2:: with SMTP id b185-v6mr4363283pfb.225.1528432171491; Thu, 07 Jun 2018 21:29:31 -0700 (PDT) Received: from mail-pf0-f173.google.com (mail-pf0-f173.google.com. [209.85.192.173]) by smtp.gmail.com with ESMTPSA id i88-v6sm10614922pfi.153.2018.06.07.21.29.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Jun 2018 21:29:31 -0700 (PDT) Received: by mail-pf0-f173.google.com with SMTP id q1-v6so5964459pff.13; Thu, 07 Jun 2018 21:29:31 -0700 (PDT) X-Received: by 2002:a63:7b1e:: with SMTP id w30-v6mr3718759pgc.402.1528432170913; Thu, 07 Jun 2018 21:29:30 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a17:90a:1581:0:0:0:0 with HTTP; Thu, 7 Jun 2018 21:29:30 -0700 (PDT) In-Reply-To: <20180608033242.GA54099@pesky> References: <201806061257.w56CvCwq089369@repo.freebsd.org> <20180606140311.GU2450@kib.kiev.ua> <20180608033242.GA54099@pesky> From: Ryan Libby Date: Thu, 7 Jun 2018 21:29:30 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r334708 - head/sys/kern To: Mark Johnston Cc: Konstantin Belousov , Justin Hibbits , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" 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 04:36:31 -0000 On Thu, Jun 7, 2018 at 8:32 PM, Mark Johnston wrote: > 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: Mark, Justin, and I looked at this. I think that the VNASSERT itself is not quite valid, since it is checking the value of v_iflag without the vnode interlock held (and without the vop lock also). If the rule is that we normally need the vnode interlock to check VI_FREE, then I don't think that possible reordering between the refcount_acquire and VI_FREE clearing in vnlru_free_locked is necessarily a problem in production. It might just be that unlocked assertions about v_iflag actually need additional synchronization (although it would be a little unfortunate to add synchronization only to INVARIANTS builds). > >> 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; >