Date: Thu, 7 Jun 2018 23:32:42 -0400 From: Mark Johnston <markj@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Justin Hibbits <jhibbits@freebsd.org>, 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> In-Reply-To: <20180606140311.GU2450@kib.kiev.ua> References: <201806061257.w56CvCwq089369@repo.freebsd.org> <20180606140311.GU2450@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180608033242.GA54099>