svn commit: r334708 - head/sys/kern

[Konstantin Belousov]

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.

Does the fence needed for the non-invariants case ? 

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