While debugging some epoch related races at Netflix, we discoveredfew non fundamental, but annoying issues with epoch.First, the inlining makes it difficult to profile and trace epoch.At the same time, inlining doesn't effectively happens. In...

[glebius]

Summary

...files
that use epoch_enter/exit extensively, Clang would create non-inlined
functions. Resulting kernel will have several instances of
epoch_enter/exit.

Performance win for inlining is speculative. At the same time we can
speculate that inlining will bloat size of kernel text, resulting in
worse instruction cache hit ratio. Unless properly measured inlining
shouldn't be used by default.

Once uninlined, epoch can use regular thread KPI.

Second, the dualism between epoch_tracker and epoch_thread is fragile
and unnecessary. So, expose CK types to kernel and use a single normal
structure for epoch_tracker.

However, the type hiding structure for epoch_context_t seems inevitable,
for now, unless CK is exposed to userland, which is undesired.

There is one functional change in the review, decribed below.

Placing epoch_tracker in a structure is a very unsafe thing to do, so
for the sake of compat KPI like if_addr_rlock, use a thread private
tracker. This makes compat KPU functions re-entrable, and epoch_tracker
not exposed to userland.

The review isn't supposed to be committed as single commit. It is divided
into several commits, that can be viewed here:

https://github.com/glebius/FreeBSD/tree/epoch-clean

[bz]

So is it the last 5 commits on your github branch or is there anything in there from before that? Having this broken up in logical junks for review will make it a lot easier.

[mmacy]

sorry didn't mean to change the title - just add markj

[mmacy]

Please separate out the de-inlining in to a separate review. We can commit that as the major chunk and then the other changes will be easier to review.

[mmacy]

This needs to be multiple reviews.

[markj]

My inline comments are just style nitpicking.

I like most of the change. I think the td_et field is an improvement over the previous mechanism, but it cannot be used in nested contexts.

sys/kern/subr_epoch.c

64

Style: extra space after TAILQ_HEAD.

182

Style: missing newline.

227

Style: missing parens.

230

Should be:

#define INIT_CHECK(epoch) do {
        if (...)
        ...
} while (0)

sys/net/if.c

1768

I'm surprised by this - what prevented multiple readers from sharing the tracker?

1768

Nothing is preventing curthread->td_et from being reused in a nested context. We should panic if that happens, at the very least.

sys/sys/epoch.h

43

What's the purpose of this typedef?

69

Style: extra space after #ifdef.

[glebius]

Committed