Date: Thu, 7 Jun 2018 21:29:30 -0700 From: Ryan Libby <rlibby@freebsd.org> To: Mark Johnston <markj@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, Justin Hibbits <jhibbits@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334708 - head/sys/kern Message-ID: <CAHgpiFyOQf6B3oGFGMz3avXqrrP0i6Puy9HqLER1XG5xE67BeQ@mail.gmail.com> In-Reply-To: <20180608033242.GA54099@pesky> References: <201806061257.w56CvCwq089369@repo.freebsd.org> <20180606140311.GU2450@kib.kiev.ua> <20180608033242.GA54099@pesky>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jun 7, 2018 at 8:32 PM, Mark Johnston <markj@freebsd.org> 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; >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHgpiFyOQf6B3oGFGMz3avXqrrP0i6Puy9HqLER1XG5xE67BeQ>