should_yield problem [Was: svn commit: r251322 - head/sys/kern]

[Andriy Gapon]

on 03/06/2013 20:36 Konstantin Belousov said the following:
> Author: kib
> Date: Mon Jun  3 17:36:43 2013
> New Revision: 251322
> URL: http://svnweb.freebsd.org/changeset/base/251322
> 
> Log:
>   Be more generous when donating the current thread time to the owner of
>   the vnode lock while iterating over the free vnode list.  Instead of
>   yielding, pause for 1 tick.  The change is reported to help in some
>   virtualized environments.

Kostik,

I've just run into a problem where word "generous" does not seem to be
applicable at all unless I am mistaken about how the code works.

There is a thread in vdropl() call that has a vnode interlock and want to take
vnode_free_list_mtx.
Then there is a thread that keeps cycling on the locked interlock while holding
vnode_free_list_mtx:

#0  cpustop_handler () at /usr/src/sys/amd64/amd64/mp_machdep.c:1391
#1  0xffffffff80cc8e1d in ipi_nmi_handler () at
/usr/src/sys/amd64/amd64/mp_machdep.c:1373
#2  0xffffffff80cd6269 in trap (frame=0xffffff80002baf30) at
/usr/src/sys/amd64/amd64/trap.c:211
#3  0xffffffff80cbf86f in nmi_calltrap () at
/usr/src/sys/amd64/amd64/exception.S:501
#4  0xffffffff80924f22 in _mtx_trylock (m=0xfffffe00236f4c98, opts=0,
file=<value optimized out>, line=0) at atomic.h:160
#5  0xffffffff809d3e5a in mnt_vnode_next_active (mvp=0xffffff86c95e4958,
mp=0xfffffe00118039a8) at /usr/src/sys/kern/vfs_subr.c:4813
#6  0xffffffff809daa86 in vfs_msync (mp=0xfffffe00118039a8, flags=2) at
/usr/src/sys/kern/vfs_subr.c:3542
#7  0xffffffff809e06ef in sys_sync (td=<value optimized out>, uap=<value
optimized out>) at /usr/src/sys/kern/vfs_syscalls.c:149
#8  0xffffffff80cd515a in amd64_syscall (td=0xfffffe019d966490, traced=0) at
subr_syscall.c:135
#9  0xffffffff80cbf717 in Xfast_syscall () at
/usr/src/sys/amd64/amd64/exception.S:387

Now the curious part:
(kgdb) p td->td_swvoltick
$4 = 0
(kgdb) p td->td_ru.ru_nvcsw
$7 = 0

ticks happen to be in negative territory:
(kgdb) p ticks
$2 = -1946084020

Give this definition:
int
should_yield(void)
{

        return (ticks - curthread->td_swvoltick >= hogticks);
}

We see that should_yield() is going to return false until ticks roll over to
become positive again.  And obviously with the thread spinning on VI_TRYLOCK()
it's not going to get its td_swvoltick updated.

I am not sure if the originally reported problem was also caused by
should_yield() or if it was something else.  But in either case I think that we
should fix should_yield.  Perhaps (ticks - curthread->td_swvoltick) should be
cast to unsigned before comparison?

>   Submitted by:	Roger Pau Monn? <roger.pau at citrix.com>
>   Discussed with:	jilles
>   Tested by:	pho
>   MFC after:	2 weeks
> 
> Modified:
>   head/sys/kern/vfs_subr.c
> 
> Modified: head/sys/kern/vfs_subr.c
> ==============================================================================
> --- head/sys/kern/vfs_subr.c	Mon Jun  3 17:36:26 2013	(r251321)
> +++ head/sys/kern/vfs_subr.c	Mon Jun  3 17:36:43 2013	(r251322)
> @@ -4693,7 +4693,7 @@ restart:
>  			if (mp_ncpus == 1 || should_yield()) {
>  				TAILQ_INSERT_BEFORE(vp, *mvp, v_actfreelist);
>  				mtx_unlock(&vnode_free_list_mtx);
> -				kern_yield(PRI_USER);
> +				pause("vnacti", 1);
>  				mtx_lock(&vnode_free_list_mtx);
>  				goto restart;
>  			}
> 


-- 
Andriy Gapon