libstapsdt icon indicating copy to clipboard operation
libstapsdt copied to clipboard

Is support for semaphores useful?

Open dalehamel opened this issue 5 years ago • 5 comments

I'm struggling to understand how libstapsdt supports is-enabled.

It would appear that it relies on something, somewhere, overwriting the data at the address of the probe, but It's not clear to me where this is.

The only hint that i see is the magic number 0x90, which appears to be what systemtap depends on as a TRAP instruction. Is something further down in the kernel or in libbcc handling this?

It is clear to me that libbcc supports USDT probes that rely on a semaphore for their implementation of is-enabled, and that codepath makes sense.

Yet, when I run the python example of libstapsdt it appears to somehow correctly indicate to the userspace program when a probe is enabled - so perhaps there is no need or benefit to the semaphore at the moment.

One obvious benefit I can think of is that semaphores by their nature should allow for handling multiple observers attaching to a trace point, and correctly disabling it when the last one de-registers.

My assumption is that adding semaphore support would require allocating an unsigned short within the stubbed library, and putting the address of this value within the elf notes. I would need to verify exactly how this is done in conventional systemtap SDT macros, but it doesn't seem too difficult in theory.

@mmarchini when you have a moment, I would very much appreciate if you could shed a light on how is-enabled currently works, and weigh-in on any possible value of adding support for semaphores.

I asked about this in the iovisor irc, so i'll copy my questions and investigation from there to here:

i've been reading through mmarchini's work on libstapsdt, and I'm struggling to understand how it implements checking if a probe is enabled 
https://github.com/sthima/libstapsdt/blob/99911c5a44ea40fd14d99c52c5dacf4328aa463c/src/libstapsdt.c#L185-L193
in http://www.brendangregg.com/blog/2015-07-03/hacking-linux-usdt-ftrace.html, i understand how the code in bcc basically implements this same approach to increment the
semaphore, and so it makes sense that this is a way to signal to the process that it's enabled
when i read the ELF notes for the dynamic stub library produced by libstapsdt though, it clearly doesn't use a semaphore
checking the code in bcc, it's clear that this bypasses the semaphore code altogether
yet, it works somehow to check if a probe is enabled. It appears to be checking the address where the probe is defined
https://github.com/sthima/libstapsdt/blob/99911c5a44ea40fd14d99c52c5dacf4328aa463c/src/libstapsdt.c#L111-L119
is there a place somewhere that the probe address is modified when attached to, and disabled when detached? 
https://github.com/iovisor/bcc/blob/ba41501bb2ca89312061b31c08e570a11c092370/src/cc/usdt/usdt.cc#L121-L135 this would appear to nothing for these types of probes

dalehamel avatar Mar 20 '19 13:03 dalehamel

I'm struggling to understand how libstapsdt supports is-enabled.

It would appear that it relies on something, somewhere, overwriting the data at the address of the probe, but It's not clear to me where this is.

The only hint that i see is the magic number 0x90, which appears to be what systemtap depends on as a TRAP instruction. Is something further down in the kernel or in libbcc handling this?

Yes, libstapsdt is-enabled was implemented assuming the tracing application will insert a trap at the probe's location.

One obvious benefit I can think of is that semaphores by their nature should allow for handling multiple observers attaching to a trace point, and correctly disabling it when the last one de-registers.

My assumption is that adding semaphore support would require allocating an unsigned short within the stubbed library, and putting the address of this value within the elf notes. I would need to verify exactly how this is done in conventional systemtap SDT macros, but it doesn't seem too difficult in theory.

That makes sense and seems simple to implement, and there seem to be benefits of having semaphores on libstapsdt. If you want to give it a try go ahead, otherwise I should have some time to work on it next month.

mmarchini avatar Mar 20 '19 14:03 mmarchini

Thanks for the reply @mmarchini

Yes, libstapsdt is-enabled was implemented assuming the tracing application will insert a trap at the probe's location.

Do you happen to know how this works with BCC?

I’ll see if I can figure out how to implement the semaphores. I’m mostly curious if there is any performance benefit in using them.

dalehamel avatar Mar 20 '19 14:03 dalehamel

Ah I see that if I probe kprobe:is_trap_insn, I can see it being fired when the probe is enabled. So my theory is that this should be pretty cheap as long as you don't constantly enable/disable probes.

dalehamel avatar Mar 20 '19 15:03 dalehamel

Ah, I think I have my answer https://github.com/jav/systemtap/blob/master/runtime/uprobes/uprobes.txt - it's the uprobes underlying bcc's USDT support that do it, if i'm correct.

dalehamel avatar Mar 20 '19 19:03 dalehamel

I can think of one case where semaphores would be useful - if anything is still attached to a probe, we can more safely determine this before unloading a probe's provider

Right now, if you unload a probe that is still attached, the probe appears to remain attached to the old address as nothing de-registers it in the kernel.

If you iterate through each probe and verify they are not enabled through the kernel semaphore, you can be sure that unloading a provider is safe.

I actually don't know of any consequences of leaving the probe attached after the provider is unloaded. It might be that the breakpoint instruction would get overwritten if the memory is reused, and that the tracepoint would just be attached to something that cannot be possible fired.

Right now semaphore incrementing in bcc is done by writing to /dev/proc/mem directly, but this has been moved into the kernel. Once bcc supports doing this through the kernel API, I think we should add support for it here due to the advantages it may have around safer use of tracepoints.

dalehamel avatar Apr 24 '19 18:04 dalehamel