Fix a panic during boot while executing the vt_timer() callout

[jtl]

Summary

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.

Test Plan

I modified the code to add a delay in order to make it more likely we would hit the race condition. With that code and without this proposed patch, I saw crashes. With that code and with this proposed patch, I did not see crashes.

[ed]

By the time I see multiple atomics operations stacked like this, I start to wonder: isn't there a way to guarantee this by simply using locking APIs? We already call VT_LOCK()/VT_UNLOCK(), it seems. Maybe this is just missing in one of the other places?

[imp]

sys/dev/vt/vt_core.c

2623 ↗

(On Diff #25766)

What are the races? Might not be a bad idea to point to the other side of the race.

[jtl]

In D9828#202716, @ed wrote:

By the time I see multiple atomics operations stacked like this, I start to wonder: isn't there a way to guarantee this by simply using locking APIs? We already call VT_LOCK()/VT_UNLOCK(), it seems. Maybe this is just missing in one of the other places?

I just opened bug 217408 for this. I have more analysis there.

In short, the other end of the race may hold a spin lock when calling vt_resume_flush_timer(), so it can't acquire the vd lock (a regular mutex) at that point.

We don't actually need atomics here. But, we do care about order of operations, so I used them for the memory barriers they provide. We need to ensure that vd_timer_armed is 1 before we set VDF_ASYNC. We may be able to do away with the second atomic. I guess it isn't really critical VDF_ASYNC is set before we call vt_timer(). (And, even though it might be somewhat unusual, vt_timer() may well fire before callout_reset() returns in the case the thread gets pre-empted. But, as I said, it may actually not be critical that VDF_ASYNC is set before vt_timer() runs. So, maybe that was an overreaction on my part.)

sys/dev/vt/vt_core.c

2623 ↗

(On Diff #25766)

ACK. I'll try to improve the comment.

[jtl]

Enhance the comment. Move an atomic operation to non-atomic.

[jtl]

I had a long discussion with @ed in the bug. You can see that for context. I think the summary is that I still think we should commit this code, even if @ed thinks a more robust fix requires a larger change. I would like to do that, but do also support @ed making a further bug fix in the future.

[emaste]

sys/dev/vt/vt_core.c

2627 ↗

(On Diff #25772)

perhaps add a reference to the PR?

[jtl]

sys/dev/vt/vt_core.c

2627 ↗

(On Diff #25772)

As we discussed today the dev summit, I think this change is still necessary even in the single-core case. So, it would still be necessary even with the change @ed proposed in the PR. Therefore, since this change shouldn't be temporary, it is not necessary to reference the PR in the comment.