[illumos-Developer] Reaping enablings on defunct providers

[Bryan Cantrill]

Hey Adam,

> This is great stuff, and -- as you say -- something that we've all
> wanted for a long long time. The code looks great. The only addition
> I'd ask for is if you considered having a case with bufpolicy=ring
> and/or anonymous tracing.

Supporting a ring buffer policy is a tad brutal, because it would
require processing the entire ring to verify that the ECB isn't in it.
 There are ways we could optimize that, but they're ugly; unless
someone is burning with this use case, I'd rather punt on it.
Anonymous tracing will just work; once the enabling is claimed, the
buffer will be switched, and it will become reapable.

> I had a few questions:
>
> Can you explain the purpose of dtrace_unregister_defunct_reap? Is the
> idea to try for providers that were recently defunct and then give up
> after a minute?

Yes; you don't want to keep trying in perpetuity because you may be
blocked by either a ring buffer policy or speculative tracing or
something else that is managing to cache ECB state.  So after a while,
you want to give up.

> Should the fasttrap cleanup stuff use a taskq rather than its timeout
> stuff (not asking you to do it of course)?

Eh, it's fine. ;)  I don't think it would be a problem to reimplement
that as a task queue, of course, but I would wait until there was some
compelling reason to do so.

> Can you talk a little about the approach you took? Obviously if you're
> using speculative tracing or bufpolicy=ring or anonymous tracing it
> limits the efficacy of what you've done. Would it have been possible
> to disable the ECBs without destroying them? I assume that's horribly
> facile, but I'd be interested to understand.

It's probably possible (as Wolfgang Thaler was fond of saying, "it's
just software, man"), but it would be additional complexity for cases
that are pretty borderline.  The speculative tracing one is hard to
get excited about because I don't think such enablings would be
long-lived; ring-buffer enablings are longer lived, of course, but I
don't think they tend to include USDT probes.  (Though I'm happy to
tackle this as follow-on work if someone has actually encountered this
case.)  And as I mentioned above, anonymous tracing will just work.
(Though long-lived anonymous enablings of USDT probes must be really
unusual!)

> dtrace_buffer_t structures should be cache-size aligned since they're
> per-CPU structures, yes? Would that be worth noting explicitly? And
> why did you elect to have padding in two places rather than just 7
> 64-bit values at the end?

It's just to reduce false sharing. I debated not padding it out; I
think it's unlikely to be a problem in practice as a given line can
only be shared by at most two CPUs, and that's generally not enough
sharing to get deeply pathological -- but the extra memory cost is
minimal, and there's value to maintaining as much CPU orthogonality as
possible with respect to probe evaluation.  As for padding it out in
two places, the first pad is only there for 32-bit whereas the pad
that I added is bitness-neutral.

> Thanks; again, this is really great.

Thanks for taking a look!  Now on to my other wads. ;)

        - Bryan