epoch: support non-preemptible epochs checking in_epoch()

[kevans]

Summary

Twice I've tried using epoch and managed to forget that in_epoch() only operates on preemptible epochs, so here's a first cut at rectifying this.

For default epochs, it's easy enough to verify that we're in the given epoch (AFAICT): if we're in a critical section and our record for the given epoch is active, then we're in it.

This patch also adds some additional INVARIANTS bookkeeping. Notably, we set and check the recorded thread in epoch_enter/epoch_exit to try and catch some edge-cases for the caller. It also checks upon freeing that none of the records had a thread in the the epoch, which may make it a little easier to diagnose some improper use if epoch_free() took place while some other thread was inside.

[markj]

sys/kern/subr_epoch.c

82 ↗

(On Diff #79196)

To be honest I think the comment is a bit verbose. /* Used to verify record ownership for non-preemptible epochs. */ would be sufficient IMO.

83 ↗

(On Diff #79196)

Did you consider using er_tdlist instead of adding a new field?

468 ↗

(On Diff #79196)

The ifdef is not needed.

897 ↗

(On Diff #79196)

I don't think this printf is very useful as it is, the debugger will print the same information.

899 ↗

(On Diff #79196)

Shouldn't it be MPASS(entered && er->er_td == td)?

[kevans]

sys/kern/subr_epoch.c

83 ↗

(On Diff #79196)

I'm open to reconsidering it, but I had kinda ruled it out because it seemed a little clunky -- er_tdlist is a tailq of epoch_trackers, that we don't use for non-preemptible epochs. I hadn't really put any more thought into it then that, to be honest.

899 ↗

(On Diff #79196)

I'll make it so after removing the useless printf above -- right now all exits go through here, so !entered was perfectly valid; if we're not printing anything on failure (I can't imagine what we would print) then the previous cases will just return immediately.

[kib]

sys/kern/subr_epoch.c

83 ↗

(On Diff #79196)

It would be somewhat nice to not change struct size depending on the kernel config.

[markj]

sys/kern/subr_epoch.c

83 ↗

(On Diff #79196)

Ah, I had it in my head that er_tdlist is a list of struct thread. I think it's ok then.

I was going to write the same thing about conditionally defined structure members, but in this case it's private to subr_epoch.c so not really a problem.

[kevans]

Addressed most of the commentary so far; er_td remains there for the time being, but notably epoch_record is internal and not part of KBI that would fluctuate with INVARIANTS.

[markj]

Looks good.

[kib]

sys/kern/subr_epoch.c

869 ↗

(On Diff #79224)

Is it useful to enter crit section and assert that er_td != curthread ?

[kevans]

sys/kern/subr_epoch.c

869 ↗

(On Diff #79224)

I'm inclined to agree, yes.

[kevans]

sys/kern/subr_epoch.c

869 ↗

(On Diff #79224)

On second thought, I guess we have to iterate over all of the cpus and check their records on this epoch to make sure that none of them have er_td == curthread since we weren't in a critical section, but that seems pretty reasonable. The current thread has reason to believe it might be in the epoch, we could catch here if something bogus happens.

[kevans]

Additional assert if we're not in a critical section; presumably we could have migrated, so check if any cpu has this thread erroneously recorded. We're perhaps also pretty likely to hit the epoch_exit() assertion that er->er_td == curthread or the follow-up critical_exit() assert if the caller bungled up critical section handling somewhere and migrated between enter/exit, but they could have also lucked out so it's perhaps best not to rely on that.

[kib]

sys/kern/subr_epoch.c

877 ↗

(On Diff #79251)

BTW I would change MPASS to KASSERT which can provide more information in the message. For instance, the cpu index where the situation occurs is immediately useful.

[kevans]

sys/kern/subr_epoch.c

877 ↗

(On Diff #79251)

Sure, I will do this pre-commit, maybe:

"exited critical section in epoch %s, cpu %d"