Skip site navigation (1)Skip section navigation (2)
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+p_cp1FESRzfDzepDZgkTcQ@mail.gmail.com>
References:  <201806061257.w56CvCwq089369@repo.freebsd.org> <CANCZdfq0E9u-_GiSdBrZWsH53pV+p_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: <http://docs.FreeBSD.org/cgi/mid.cgi?CAHSQbTAQuopFVKo5_8TLWPVZE6bAzyy7T6inK_wvPLJKiwttAg>