Re: svn commit: r334708 - head/sys/kern

[Justin Hibbits]

On Fri, 17 Aug 2018 12:08:20 -0400
Mark Johnston  wrote:

> On Thu, Jul 19, 2018 at 12:09:21PM -0700, Bryan Drewery wrote:
> > Did this issue get resolved?  
> 
> It's fixed by r337974.

Thanks Mark!

- Justin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Mark Johnston]

On Thu, Jul 19, 2018 at 12:09:21PM -0700, Bryan Drewery wrote:
> Did this issue get resolved?

It's fixed by r337974.

> On 6/8/2018 11:37 AM, Konstantin Belousov wrote:
> > On Fri, Jun 08, 2018 at 02:30:10PM -0400, Mark Johnston wrote:
> >> On Fri, Jun 08, 2018 at 08:37:55PM +0300, Konstantin Belousov wrote:
> >>> On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
>  On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> > Checking it without any locks is perfectly valid in this case. It is 
> > done
> > after v_holdcnt gets bumped from a non-zero value. So at that time it
> > is at least two. Of course that result is stale as an arbitrary number 
> > of
> > other threads could have bumped and dropped the ref past that point.
> > The minimum value is 1 since we hold the ref. But this means the
> > vnode must not be on the free list and that's what the assertion is
> > verifying.
> >
> > The problem is indeed lack of ordering against the code clearing the
> > flag for the case where 2 threads to vhold and one does the 0->1
> > transition.
> >
> > That said, the fence is required for the assertion to work.
> >
> 
>  Yeah, I agree with this logic.  What I mean is that reordering between
>  v_holdcnt 0->1 and v_iflag is normally settled by the release and
>  acquisition of the vnode interlock, which we are supposed to hold for
>  v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
>  are not asserts do hold the vnode interlock.  So, I'm just saying that I
>  don't think the possible reordering affects them.
> >>> But do we know that only VI_FREE checks are affected ?
> >>>
> >>> My concern is that users of _vhold() rely on seeing up to date state of 
> >>> the
> >>> vnode, and VI_FREE is only an example of the problem.  Most likely, the
> >>> code which fetched the vnode pointer before _vhold() call, should 
> >>> guarantee
> >>> visibility.
> >>
> >> Wouldn't this be a problem only if we permit lockless accesses of vnode
> >> state outside of _vhold() and other vnode subroutines? The current
> >> protocol requires that the interlock be held, and this synchronizes with
> >> code which performs 0->1 and 1->0 transitions of the hold count. If this
> >> requirement is relaxed in the future, then fences would indeed be
> >> needed.
> > 
> > I do not claim that my concern is a real problem. I stated it as a
> > thing to look at when deciding whether the fences should be added
> > (unconditionally ?).
> > 
> > If you argument is that the only current lock-less protocol for the
> > struct vnode state is the v_holdcnt transitions for > 1, then I can
> > agree with it.
> > 
> 
> 
> -- 
> Regards,
> Bryan Drewery
> 



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Justin Hibbits]

To the best of my understanding, the underlying race condition within
the assert has not been solved.  I've worked around it for now by
simply removing the assert, but that's just a workaround to keep my
development going.

- Justin
On Thu, Jul 19, 2018 at 2:09 PM Bryan Drewery  wrote:
>
> Did this issue get resolved?
>
> On 6/8/2018 11:37 AM, Konstantin Belousov wrote:
> > On Fri, Jun 08, 2018 at 02:30:10PM -0400, Mark Johnston wrote:
> >> On Fri, Jun 08, 2018 at 08:37:55PM +0300, Konstantin Belousov wrote:
> >>> On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
>  On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> > Checking it without any locks is perfectly valid in this case. It is 
> > done
> > after v_holdcnt gets bumped from a non-zero value. So at that time it
> > is at least two. Of course that result is stale as an arbitrary number 
> > of
> > other threads could have bumped and dropped the ref past that point.
> > The minimum value is 1 since we hold the ref. But this means the
> > vnode must not be on the free list and that's what the assertion is
> > verifying.
> >
> > The problem is indeed lack of ordering against the code clearing the
> > flag for the case where 2 threads to vhold and one does the 0->1
> > transition.
> >
> > That said, the fence is required for the assertion to work.
> >
> 
>  Yeah, I agree with this logic.  What I mean is that reordering between
>  v_holdcnt 0->1 and v_iflag is normally settled by the release and
>  acquisition of the vnode interlock, which we are supposed to hold for
>  v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
>  are not asserts do hold the vnode interlock.  So, I'm just saying that I
>  don't think the possible reordering affects them.
> >>> But do we know that only VI_FREE checks are affected ?
> >>>
> >>> My concern is that users of _vhold() rely on seeing up to date state of 
> >>> the
> >>> vnode, and VI_FREE is only an example of the problem.  Most likely, the
> >>> code which fetched the vnode pointer before _vhold() call, should 
> >>> guarantee
> >>> visibility.
> >>
> >> Wouldn't this be a problem only if we permit lockless accesses of vnode
> >> state outside of _vhold() and other vnode subroutines? The current
> >> protocol requires that the interlock be held, and this synchronizes with
> >> code which performs 0->1 and 1->0 transitions of the hold count. If this
> >> requirement is relaxed in the future, then fences would indeed be
> >> needed.
> >
> > I do not claim that my concern is a real problem. I stated it as a
> > thing to look at when deciding whether the fences should be added
> > (unconditionally ?).
> >
> > If you argument is that the only current lock-less protocol for the
> > struct vnode state is the v_holdcnt transitions for > 1, then I can
> > agree with it.
> >
>
>
> --
> Regards,
> Bryan Drewery
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Bryan Drewery]

Did this issue get resolved?

On 6/8/2018 11:37 AM, Konstantin Belousov wrote:
> On Fri, Jun 08, 2018 at 02:30:10PM -0400, Mark Johnston wrote:
>> On Fri, Jun 08, 2018 at 08:37:55PM +0300, Konstantin Belousov wrote:
>>> On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
 On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> Checking it without any locks is perfectly valid in this case. It is done
> after v_holdcnt gets bumped from a non-zero value. So at that time it
> is at least two. Of course that result is stale as an arbitrary number of
> other threads could have bumped and dropped the ref past that point.
> The minimum value is 1 since we hold the ref. But this means the
> vnode must not be on the free list and that's what the assertion is
> verifying.
>
> The problem is indeed lack of ordering against the code clearing the
> flag for the case where 2 threads to vhold and one does the 0->1
> transition.
>
> That said, the fence is required for the assertion to work.
>

 Yeah, I agree with this logic.  What I mean is that reordering between
 v_holdcnt 0->1 and v_iflag is normally settled by the release and
 acquisition of the vnode interlock, which we are supposed to hold for
 v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
 are not asserts do hold the vnode interlock.  So, I'm just saying that I
 don't think the possible reordering affects them.
>>> But do we know that only VI_FREE checks are affected ?
>>>
>>> My concern is that users of _vhold() rely on seeing up to date state of the
>>> vnode, and VI_FREE is only an example of the problem.  Most likely, the
>>> code which fetched the vnode pointer before _vhold() call, should guarantee
>>> visibility.
>>
>> Wouldn't this be a problem only if we permit lockless accesses of vnode
>> state outside of _vhold() and other vnode subroutines? The current
>> protocol requires that the interlock be held, and this synchronizes with
>> code which performs 0->1 and 1->0 transitions of the hold count. If this
>> requirement is relaxed in the future, then fences would indeed be
>> needed.
> 
> I do not claim that my concern is a real problem. I stated it as a
> thing to look at when deciding whether the fences should be added
> (unconditionally ?).
> 
> If you argument is that the only current lock-less protocol for the
> struct vnode state is the v_holdcnt transitions for > 1, then I can
> agree with it.
> 


-- 
Regards,
Bryan Drewery



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r334708 - head/sys/kern

[Konstantin Belousov]

On Fri, Jun 08, 2018 at 02:30:10PM -0400, Mark Johnston wrote:
> On Fri, Jun 08, 2018 at 08:37:55PM +0300, Konstantin Belousov wrote:
> > On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
> > > On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> > > > Checking it without any locks is perfectly valid in this case. It is 
> > > > done
> > > > after v_holdcnt gets bumped from a non-zero value. So at that time it
> > > > is at least two. Of course that result is stale as an arbitrary number 
> > > > of
> > > > other threads could have bumped and dropped the ref past that point.
> > > > The minimum value is 1 since we hold the ref. But this means the
> > > > vnode must not be on the free list and that's what the assertion is
> > > > verifying.
> > > >
> > > > The problem is indeed lack of ordering against the code clearing the
> > > > flag for the case where 2 threads to vhold and one does the 0->1
> > > > transition.
> > > >
> > > > That said, the fence is required for the assertion to work.
> > > >
> > > 
> > > Yeah, I agree with this logic.  What I mean is that reordering between
> > > v_holdcnt 0->1 and v_iflag is normally settled by the release and
> > > acquisition of the vnode interlock, which we are supposed to hold for
> > > v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
> > > are not asserts do hold the vnode interlock.  So, I'm just saying that I
> > > don't think the possible reordering affects them.
> > But do we know that only VI_FREE checks are affected ?
> > 
> > My concern is that users of _vhold() rely on seeing up to date state of the
> > vnode, and VI_FREE is only an example of the problem.  Most likely, the
> > code which fetched the vnode pointer before _vhold() call, should guarantee
> > visibility.
> 
> Wouldn't this be a problem only if we permit lockless accesses of vnode
> state outside of _vhold() and other vnode subroutines? The current
> protocol requires that the interlock be held, and this synchronizes with
> code which performs 0->1 and 1->0 transitions of the hold count. If this
> requirement is relaxed in the future, then fences would indeed be
> needed.

I do not claim that my concern is a real problem. I stated it as a
thing to look at when deciding whether the fences should be added
(unconditionally ?).

If you argument is that the only current lock-less protocol for the
struct vnode state is the v_holdcnt transitions for > 1, then I can
agree with it.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Mark Johnston]

On Fri, Jun 08, 2018 at 08:37:55PM +0300, Konstantin Belousov wrote:
> On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
> > On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> > > Checking it without any locks is perfectly valid in this case. It is done
> > > after v_holdcnt gets bumped from a non-zero value. So at that time it
> > > is at least two. Of course that result is stale as an arbitrary number of
> > > other threads could have bumped and dropped the ref past that point.
> > > The minimum value is 1 since we hold the ref. But this means the
> > > vnode must not be on the free list and that's what the assertion is
> > > verifying.
> > >
> > > The problem is indeed lack of ordering against the code clearing the
> > > flag for the case where 2 threads to vhold and one does the 0->1
> > > transition.
> > >
> > > That said, the fence is required for the assertion to work.
> > >
> > 
> > Yeah, I agree with this logic.  What I mean is that reordering between
> > v_holdcnt 0->1 and v_iflag is normally settled by the release and
> > acquisition of the vnode interlock, which we are supposed to hold for
> > v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
> > are not asserts do hold the vnode interlock.  So, I'm just saying that I
> > don't think the possible reordering affects them.
> But do we know that only VI_FREE checks are affected ?
> 
> My concern is that users of _vhold() rely on seeing up to date state of the
> vnode, and VI_FREE is only an example of the problem.  Most likely, the
> code which fetched the vnode pointer before _vhold() call, should guarantee
> visibility.

Wouldn't this be a problem only if we permit lockless accesses of vnode
state outside of _vhold() and other vnode subroutines? The current
protocol requires that the interlock be held, and this synchronizes with
code which performs 0->1 and 1->0 transitions of the hold count. If this
requirement is relaxed in the future, then fences would indeed be
needed.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Konstantin Belousov]

On Thu, Jun 07, 2018 at 11:02:29PM -0700, Ryan Libby wrote:
> On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> > On Fri, Jun 8, 2018 at 6:29 AM, Ryan Libby  wrote:
> >>
> >> 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 +, 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.cWed Jun  6 10:46:24 2018
> >> >> > (r334707)
> >> >> > +++ head/sys/kern/vfs_subr.cWed 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(>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.
> >
> >
> > Checking it without any locks is perfectly valid in this case. It is done
> > after v_holdcnt gets bumped from a non-zero value. So at that time it
> > is at least two. Of course that result is stale as an arbitrary number of
> > other threads could have bumped and dropped the ref past that point.
> > The minimum value is 1 since we hold the ref. But this means the
> > vnode must not be on the free list and that's what the assertion is
> > verifying.
> >
> > The problem is indeed lack of ordering against the code clearing the
> > flag for the case where 2 threads to vhold and one does the 0->1
> > transition.
> >
> > That said, the fence is required for the assertion to work.
> >
> 
> Yeah, I agree with this logic.  What I mean is that reordering between
> v_holdcnt 0->1 and v_iflag is normally settled by the release and
> acquisition of the vnode interlock, which we are supposed to hold for
> v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
> are not asserts do hold the vnode interlock.  So, I'm just saying that I
> don't think the possible reordering affects them.
But do we know that only VI_FREE checks are affected ?

My concern is that users of _vhold() rely on seeing up to date state of the
vnode, and VI_FREE is only an example of the problem.  Most likely, the
code which fetched the vnode pointer before _vhold() call, should guarantee
visibility.

> 
> >>
> >>
> >> It might just be that unlocked assertions about v_iflag actually need
> >> additional synchronization (although it would be a little unfortunate to
> >> add 

Re: svn commit: r334708 - head/sys/kern

[Ryan Libby]

On Thu, Jun 7, 2018 at 10:03 PM, Mateusz Guzik  wrote:
> On Fri, Jun 8, 2018 at 6:29 AM, Ryan Libby  wrote:
>>
>> 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 +, 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.cWed Jun  6 10:46:24 2018
>> >> > (r334707)
>> >> > +++ head/sys/kern/vfs_subr.cWed 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(>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.
>
>
> Checking it without any locks is perfectly valid in this case. It is done
> after v_holdcnt gets bumped from a non-zero value. So at that time it
> is at least two. Of course that result is stale as an arbitrary number of
> other threads could have bumped and dropped the ref past that point.
> The minimum value is 1 since we hold the ref. But this means the
> vnode must not be on the free list and that's what the assertion is
> verifying.
>
> The problem is indeed lack of ordering against the code clearing the
> flag for the case where 2 threads to vhold and one does the 0->1
> transition.
>
> That said, the fence is required for the assertion to work.
>

Yeah, I agree with this logic.  What I mean is that reordering between
v_holdcnt 0->1 and v_iflag is normally settled by the release and
acquisition of the vnode interlock, which we are supposed to hold for
v_*i*flag.  A quick scan seems to show all of the checks of VI_FREE that
are not asserts do hold the vnode interlock.  So, I'm just saying that I
don't think the possible reordering affects them.

>>
>>
>> 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;

Re: svn commit: r334708 - head/sys/kern

[Konstantin Belousov]

On Fri, Jun 08, 2018 at 07:03:04AM +0200, Mateusz Guzik wrote:
> Part of the problem is lack of primitives like READ_ONCE/WRITE_ONCE as
> seen in the linux kernel, someone should hack up equivalents.
Only replying to this statement right now.  What is the semantic of
the _ONCE operations ?  Isn't it the same as atomic_load/store (without
fences) ?
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Matthew Macy]

> The main codepath which runs into this (... -> cache_lookup -> vhold) most
> definitely does not need the fence for production operation.
>
> What is unclear without audit is whether there are vhold users which need
> one. I think the patch is fine to go in if the other VI_FREE place gets a
> comment about a fence stemming from mtx_unlock and there is another one
> prior to the assertion explaining that this orders against v_iflag tests and
> may
> or may not be needed for other consumers.
>
> In general the code is *full* of data races and accidental reliance of
> ordering
> provided by amd64. Weeding this all out will be a painful exercise.
>
> Part of the problem is lack of primitives like READ_ONCE/WRITE_ONCE as
> seen in the linux kernel, someone should hack up equivalents.

CK of course has these. We can create a wrapper around them if we
don't want to use them directly.
-M
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Mateusz Guzik]

On Fri, Jun 8, 2018 at 6:29 AM, Ryan Libby  wrote:

> 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 +, 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.cWed Jun  6 10:46:24 2018
> (r334707)
> >> > +++ head/sys/kern/vfs_subr.cWed 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(>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.
>

Checking it without any locks is perfectly valid in this case. It is done
after v_holdcnt gets bumped from a non-zero value. So at that time it
is at least two. Of course that result is stale as an arbitrary number of
other threads could have bumped and dropped the ref past that point.
The minimum value is 1 since we hold the ref. But this means the
vnode must not be on the free list and that's what the assertion is
verifying.

The problem is indeed lack of ordering against the code clearing the
flag for the case where 2 threads to vhold and one does the 0->1
transition.

That said, the fence is required for the assertion to work.


>
> 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();
>

This probably has no added value for non-debug kernels.


> > refcount_acquire(>v_holdcnt);
> >
> > mtx_unlock(_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(>v_holdcnt)) {
> > -#if !defined(__amd64__) && !defined(__i386__)
> > -   mb();
> > -#endif
> > +   atomic_thread_fence_acq();
>

Same as above.


> >

Re: svn commit: r334708 - head/sys/kern

[Ryan Libby]

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 +, 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.cWed Jun  6 10:46:24 2018
>> > (r334707)
>> > +++ head/sys/kern/vfs_subr.cWed 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(>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(>v_holdcnt);
>
> mtx_unlock(_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(>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;
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Mark Johnston]

On Wed, Jun 06, 2018 at 05:03:11PM +0300, Konstantin Belousov wrote:
> On Wed, Jun 06, 2018 at 12:57:12PM +, 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.cWed Jun  6 10:46:24 2018
> > (r334707)
> > +++ head/sys/kern/vfs_subr.cWed 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(>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(>v_holdcnt);
 
mtx_unlock(_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(>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;

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Konstantin Belousov]

On Wed, Jun 06, 2018 at 12:57:12PM +, 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(>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.

Does the fence needed for the non-invariants case ? 

Fourth, doesn't v_usecount has the same issues WRT inactivation ?

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Andriy Gapon]

On 06/06/2018 16:02, Warner Losh wrote:
> 
> 
> On Wed, Jun 6, 2018 at 8:57 AM, 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(>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?

This is the wrong "acquire", "acquire" in a sense of "get" or "increment", not
in a sense of a memory barrier.



-- 
Andriy Gapon
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Justin Hibbits]

On Wed, Jun 6, 2018 at 9:02 AM, Warner Losh  wrote:
>
>
> On Wed, Jun 6, 2018 at 8:57 AM, 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.cWed Jun  6 10:46:24 2018(r334707)
>> +++ head/sys/kern/vfs_subr.cWed 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(>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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r334708 - head/sys/kern

[Warner Losh]

On Wed, Jun 6, 2018 at 8:57 AM, 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.cWed Jun  6 10:46:24 2018(r334707)
> +++ head/sys/kern/vfs_subr.cWed 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(>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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r334708 - head/sys/kern

[Justin Hibbits]

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.cWed Jun  6 10:46:24 2018(r334707)
+++ head/sys/kern/vfs_subr.cWed 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(>v_holdcnt)) {
+#if !defined(__amd64__) && !defined(__i386__)
+   mb();
+#endif
VNASSERT((vp->v_iflag & VI_FREE) == 0, vp,
("_vhold: vnode with holdcnt is free"));
return;
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"