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

[Konstantin Belousov]

On Fri, Jun 09, 2017 at 04:56:03PM -0700, Jonathan Looney wrote:
> Hi John, Konstantin,
> 
> This crash occurs during system startup when we are trying to switch from
> having each write to the vt device do an immediate flush to using a
> callout-based asynchronous flushing mechanism.
> 
> It appears the crash was caused by having the VDF_ASYNC flag set while the
> vd_timer_armed flag was 0. The fix is to make sure that vd_timer_armed is 1
> before the VDF_ASYNC flag is set. It is my understanding that the acquire
> semantics in the atomic_add_acq_int() call should ensure that the write to
> vd_timer_armed occurs before the load, bitwise-or, and store associated
> with `vd->vd_flags |= VDF_ASYNC`. Ensuring that ordering (or, at least the
> store ordering) is all that is really necessary to stop the crash from
> occurring.
No, acquire is only specified for loads, and release for stores.  In other
words, on some hypothetical ll/sc architecture, the atomic_add_acq()
could be implemented as follows, in asm-pseudocode
atomic_add_acq(int x):
	ll	x, r1
	acq	x
	add	1, r
	sc	r1, x
Your use of the atomic does not prevent stores reordering.

And equally important, _acq is useless without dual _rel.

> 
> (A more thorough analysis is available in the PR [217408], which I forgot
> to include in the commit metadata.)
> 
> To answer Konstantin's question, the VDF_ASYNC and vd_timer_armed flags are
> different. The VDF_ASYNC flag indicates that we want to use async flushing.
> The vd_timer_armed flag indicates that the callout is actually armed to
> flush at some point soon, so a thread that writes to the vt device doesn't
> need to worry about scheduling the callout.
Ok.

> 
> I'm not claiming that this fixes all bugs in this area. (In fact, I
> specifically disclaim this.) But, it does stop the crash from occurring.
> 
> If you still feel there are better mechanisms to achieve the desired
> ordering, please let me know and I'll be happy to fix and/or improve this.
See the pseudocode I posted in my original reply, which uses acq/rel pair.

> 
> Jonathan
> 
> On Thu, Jun 8, 2017 at 2:49 PM, John Baldwin <jhb at freebsd.org> 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.
> >
> > --
> > John Baldwin
> >