Date: Wed, 6 Jun 2018 09:21:49 -0400 From: Justin Hibbits <jhibbits@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: 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: <CAHSQbTAQuopFVKo5_8TLWPVZE6bAzyy7T6inK_wvPLJKiwttAg@mail.gmail.com> In-Reply-To: <CANCZdfq0E9u-_GiSdBrZWsH53pV%2Bp_cp1FESRzfDzepDZgkTcQ@mail.gmail.com> References: <201806061257.w56CvCwq089369@repo.freebsd.org> <CANCZdfq0E9u-_GiSdBrZWsH53pV%2Bp_cp1FESRzfDzepDZgkTcQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jun 6, 2018 at 9:02 AM, Warner Losh <imp@bsdimp.com> wrote: > > > On Wed, Jun 6, 2018 at 8:57 AM, Justin Hibbits <jhibbits@freebsd.org> 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; >> > > So why isn't the refcount_acquire() enough? > > Warner I'm not entirely sure. I had never seen this before, it's only cropped up on my POWER9 system. The refcount_acquire doesn't include a memory barrier, and mjg is reluctant to add one, since most refcount users don't care about ordering. Adding the memory barrier appeases the VNASSERT(). It may only be necessary for the VNASSERT(), and may not be strictly necessary for proper operation, but I think it's safest this way, until better performance metrics can be done. - Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTAQuopFVKo5_8TLWPVZE6bAzyy7T6inK_wvPLJKiwttAg>