svn commit: r319720 - head/sys/dev/vt

[Konstantin Belousov]

On Thu, Jun 08, 2017 at 02:49:43PM -0700, John Baldwin wrote:
> On Thursday, June 08, 2017 08:47:18 PM Jonathan T. Looney wrote:
> > Author: jtl
> > Date: Thu Jun  8 20:47:18 2017
> > New Revision: 319720
> > URL: https://svnweb.freebsd.org/changeset/base/319720
> > 
> > Log:
> >   With EARLY_AP_STARTUP enabled, we are seeing crashes in softclock_call_cc()
> >   during bootup. Debugging information shows that softclock_call_cc() is
> >   trying to execute the vt_consdev.vd_timer callout, and the callout
> >   structure contains a NULL c_func.
> >   
> >   This appears to be due to a race between vt_upgrade() running
> >   callout_reset() and vt_resume_flush_timer() calling callout_schedule().
> >   
> >   Fix the race by ensuring that vd_timer_armed is always set before
> >   attempting to (re)schedule the callout.
> >   
> >   Discussed with:	emaste
> >   MFC after:	2 weeks
> >   Sponsored by:	Netflix
> >   Differential Revision:	https://reviews.freebsd.org/D9828
> 
> This should probably be using atomic_thread_fence_foo() in conjunction with
> a simple 'vd->vd_timer_armed = 1' assignment instead of abusing
> atomic_add_acq_int().  Unfortunately atomic_thread_fence_*() aren't yet
> documented in atomic(9). :(  The commit message that added them is below
> though:
> 
> ------------------------------------------------------------------------
> r285283 | kib | 2015-07-08 11:12:24 -0700 (Wed, 08 Jul 2015) | 22 lines
> 
> Add the atomic_thread_fence() family of functions with intent to
> provide a semantic defined by the C11 fences with corresponding
> memory_order.
> 
> atomic_thread_fence_acq() gives r | r, w, where r and w are read and
> write accesses, and | denotes the fence itself.
> 
> atomic_thread_fence_rel() is r, w | w.
> 
> atomic_thread_fence_acq_rel() is the combination of the acquire and
> release in single operation.  Note that reads after the acq+rel fence
> could be made visible before writes preceeding the fence.
> 
> atomic_thread_fence_seq_cst() orders all accesses before/after the
> fence, and the fence itself is globally ordered against other
> sequentially consistent atomic operations.
> 
> Reviewed by:    alc
> Discussed with: bde
> Sponsored by:   The FreeBSD Foundation
> MFC after:      3 weeks
> 
> ------------------------------------------------------------------------
> 
> That said, it is hard to see how a bare acquire barrier is really
> sufficient for anything.  Acquire barriers generally must be paired with
> a release barrier in order to provide sychronization.

More, it seems that the acquire barrier on this side is not what you want.
Acquire is only effective for load, so in the atomic_add_acq() op there,
the store action does not provide any guarantees.

>From the comment, it looks like you want a typical publish/consume scheme,
where vd_timer_armed is the flag, and VDF_ASYNC is the data.  That is, in
vt_upgrade():
		vd->vd_flags |= VDF_ASYNC;
		atomic_add_rel_int(&vd->vd_timer_armed, 1);
and in vt_resume_flush_timer():
	if (!atomic_cmpset_acq_int(&vd->vd_timer_armed, 0, 1) ||
	    (vd->vd_flags & VDF_ASYNC) == 0)
		return;

But this raises another question: aren't vd_timer_armed and VDF_ASYNC
designate the same condition ?