tracing: make signal tracepoints more useful

Obviously this is the user visible change. But personally I do agree with Seiji who requested this feature. Currently trace_signal_generate() just records the fact that __send_signal() was called, you can't know whether the signal was actually sent or not.

Obviously this is the user visible change. But personally I do agree with Seiji who requested this feature. Currently trace_signal_generate() just records the fact that __send_signal() was called, you can't know whether the signal was actually sent or not.

Hello,

Please pull from

git://github.com/utrace/linux sigtrace

Another (4th) attempt to push these simple changes, now in the form of a pull request (yes, github, I still can't restore my korg account).

2/2 looks like a bugfix to me, but 1/2 changes the output from trace_signal_generate() and removes trace_signal_overflow_fail. In essence the change is:

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

where
- grp=0/1 means private or shared

- res is enum { TRACE_SIGNAL_DELIVERED, TRACE_SIGNAL_IGNORED, TRACE_SIGNAL_ALREADY_PENDING, TRACE_SIGNAL_OVERFLOW_FAIL, TRACE_SIGNAL_LOSE_INFO, };

Obviously this is the user visible change. But personally I do agree with Seiji who requested this feature. Currently trace_signal_generate() just records the fact that __send_signal() was called, you can't know whether the signal was actually sent or not.

Steven Rostedt says:

Adding more to a tracepoint is never an issue, even without a library to parse the data correctly (which we still need in the distros). Thus this change should have no issues.

Oleg Nesterov (2):
tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info
tracing: send_sigqueue() needs trace_signal_generate() too

 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   28 +++++++++----
 2 files changed, 41 insertions(+), 72 deletions(-)

Quoted 2 lines · Oleg Nesterov

Hello,

ping ;)

Quoted 47 lines

Please pull from

git://github.com/utrace/linux sigtrace

Another (4th) attempt to push these simple changes, now in the form of a pull request (yes, github, I still can't restore my korg account).

2/2 looks like a bugfix to me, but 1/2 changes the output from trace_signal_generate() and removes trace_signal_overflow_fail. In essence the change is:

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

where
- grp=0/1 means private or shared

- res is enum { TRACE_SIGNAL_DELIVERED, TRACE_SIGNAL_IGNORED, TRACE_SIGNAL_ALREADY_PENDING, TRACE_SIGNAL_OVERFLOW_FAIL, TRACE_SIGNAL_LOSE_INFO, };

Obviously this is the user visible change. But personally I do agree with Seiji who requested this feature. Currently trace_signal_generate() just records the fact that __send_signal() was called, you can't know whether the signal was actually sent or not.

Steven Rostedt says:

Adding more to a tracepoint is never an issue, even without a library to parse the data correctly (which we still need in the distros). Thus this change should have no issues.

Oleg Nesterov (2):
tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info
tracing: send_sigqueue() needs trace_signal_generate() too

 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   28 +++++++++----
 2 files changed, 41 insertions(+), 72 deletions(-)

Quoted 2 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

Looks good to me at a first (quick) sight, except this bit which changes the ABI:

Quoted 2 lines

Quoted 2 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

That's not how we change tracepoints generally - we add a new one and eventually phase out the old one. Which apps/tools rely on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

Quoted 6 lines · Ingo Molnar

Looks good to me at a first (quick) sight, except this bit which changes the ABI:

Quoted 2 lines

Quoted 2 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

Quoted 4 lines

That's not how we change tracepoints generally - we add a new one and eventually phase out the old one. Which apps/tools rely on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

But this tracepoint wasn't changed, it was added on to. There's a difference. Any tool that uses this (including something like powertop) should be able to handle it. [...]

Quoted 8 lines · Steven Rostedt

Quoted 6 lines · Ingo Molnar

Looks good to me at a first (quick) sight, except this bit which changes the ABI:

Quoted 2 lines

Quoted 2 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

Quoted 8 lines

Quoted 4 lines

That's not how we change tracepoints generally - we add a new one and eventually phase out the old one. Which apps/tools rely on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

But this tracepoint wasn't changed, it was added on to. There's a difference. Any tool that uses this (including something like powertop) should be able to handle it. [...]

That's mostly true in theory - the question is, is it true in practice?

Say if an app relies on the smaller data structure, it sure might get surprised by the kernel writing a wider record ...

But do we really need to keep the old tracepoint? IOW, what if we simply rename it and add more info?

Quoted 3 lines · Oleg Nesterov

But do we really need to keep the old tracepoint? IOW, what if we simply rename it and add more info?

Quite frankly, unless somebody can point to something that breaks, I'd rather just change the existing one.

Nobody outside of a few special cases uses tracepoints. *nobody*. The only apps I have ever seen that matters to anybody ends up being latencytop and powertop. If those two have been tested and don't care, I don't think we should care.

[...] Which apps/tools rely on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

* Steven Rostedt <[email protected]> wrote:

Say if an app relies on the smaller data structure, it sure might get surprised by the kernel writing a wider record ...

Quoted 4 lines · Ingo Molnar

* Steven Rostedt <[email protected]> wrote:

Say if an app relies on the smaller data structure, it sure might get surprised by the kernel writing a wider record ...

Ingo,

The kernel does this all the time. We have syscalls that may extend the data structure. [...]

That is not true *AT ALL* in such an unqualified manner. Steve, stop being stupid.

The kernel syscall ABI may indeed sometimes expand *INPUT* structures (if via some mechanism it's possible to make sure that old ABI uses don't cause the kernel to read undefined data), but the trace events are *OUTPUT* structures.

Quoted 7 lines · Ingo Molnar

That is not true *AT ALL* in such an unqualified manner. Steve, stop being stupid.

The kernel syscall ABI may indeed sometimes expand *INPUT* structures (if via some mechanism it's possible to make sure that old ABI uses don't cause the kernel to read undefined data), but the trace events are *OUTPUT* structures.

The difference between syscalls and tracepoints is that a tracepoint always reports the size of the structure that was read, where a syscall does not. So I do consider this similar to reading the /proc/stat file as the user can see how much was read. The backwards compatibility should be easy to write. Old tools should not break, because it wont be reading the new fields, and new tools can determine which tracepoint is there because it is trivial to see which version of the tracepoint is there because of the size read.

Any tool that requests the signal trace event, and copies the full (and now larger) record it got in the ring-buffer, without expanding the target record's size accordingly will *BREAK*.

I do not claim that tools will break in practice - i'm raising the *possibility* out of caution and i'm frustrated that you *STILL* don't understand how ABIs are maintained in Linux.

Quoted 10 lines · Ingo Molnar

Any tool that requests the signal trace event, and copies the full (and now larger) record it got in the ring-buffer, without expanding the target record's size accordingly will *BREAK*.

I do not claim that tools will break in practice - i'm raising the *possibility* out of caution and i'm frustrated that you *STILL* don't understand how ABIs are maintained in Linux.

OK, but what if we rename the tracepoint?

IOW, add the new tracepoint and remove the old one. Of course, this can confuse the users of the current "signal_generate", but this is safe. b413d48a does this...

Or this is not allowed too?

* Steven Rostedt <[email protected]> wrote:

Any tool that requests the signal trace event, and copies the full (and now larger) record it got in the ring-buffer, without expanding the target record's size accordingly will *BREAK*.

I do not claim that tools will break in practice - i'm raising the *possibility* out of caution and i'm frustrated that you *STILL* don't understand how ABIs are maintained in Linux.

You arguing about defined semantics is *MEANINGLESS*. What matters is what the apps do in practice.

If the apps we know
about do it robustly and adapt (or don't care) about the expansion, and if no-one reports a regression in tools we don't know about, then it's probably fine.

But your argument that expansion is somehow part of the ABI is patently false and misses the point. Seeing your arguments make me *very* nervous about applying any ABI affecting patch from you.

People are adding tracepoints all over the kernel without much thought.

We take much more care when we add a system call. By making tracepoints have as strict a ABI as system calls will cause a maintenance nightmare in the future. I guarantee that!

Quoted 5 lines · Ingo Molnar

* Steven Rostedt <[email protected]> wrote:

Any tool that requests the signal trace event, and copies the full (and now larger) record it got in the ring-buffer, without expanding the target record's size accordingly will *BREAK*.

I'm curious to where it gets the size?

This is not like the kernel writing to a pointer in userspace memory, where it can indeed break code by writing too much. This is the userspace program writing from a shared memory location.

Quoted 4 lines

I do not claim that tools will break in practice - i'm raising the *possibility* out of caution and i'm frustrated that you *STILL* don't understand how ABIs are maintained in Linux.

You are defending code that would do:

	size = read_size(ring_buffer_event);
	memcpy(data, buffer, size);

over code that would most likely do:

	memcpy(data, buffer, sizeof(*data));

???

According to this logic, we should never increase the size of /proc/stat, because someone might do:

	i = 0;
	fd = open("/proc/stat", O_RDONLY);
	do {
		r = read(fd, buff+i, BUFSIZ);
		i += r;
	} while (r > 0);

Quoted 3 lines

You arguing about defined semantics is *MEANINGLESS*. What matters is what the apps do in practice.

Exactly, to depend on the ring buffer size to do all copies to fixed size data structures seems to be backwards to what would be done in practice.

Quoted 4 lines

If the apps we know
about do it robustly and adapt (or don't care) about the expansion, and if no-one reports a regression in tools we don't know about, then it's probably fine.

It's not about robustness, it's about the easy way to copy.

	memcpy(data, buffer, sizeof(*data));

wont break.

Quoted 4 lines

But your argument that expansion is somehow part of the ABI is patently false and misses the point. Seeing your arguments make me *very* nervous about applying any ABI affecting patch from you.

Well you already think I'm stupid, I wont change the ABI anymore. Obviously I know nothing, because I created a flexible interface that's not used by anything except perf and trace-cmd, but because there's no library, we are stuck with fixed tracepoints, which will come to haunt us in the not so distant future.

This will bloat the kernel. Tracepoints are not free. They bloat the kernel's text section. Every tracepoint still adds a bit of code in the "unlikely" part inlined where they are called. So they do have an affect on icache, as well as the code to process the tracepoint (around 5k per tracepoint).

Quoted 3 lines

The kernel does this all the time. We have syscalls that may extend the data structure. [...]

That is not true *AT ALL* in such an unqualified manner. Steve, stop being stupid.

Quoted 4 lines · Oleg Nesterov

Quoted 4 lines

2/2 looks like a bugfix to me, but 1/2 changes the output from trace_signal_generate() and removes trace_signal_overflow_fail. In essence the change is:

Quoted 3 lines

Quoted 3 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

I've also Cc:-ed Masami-san who appears to have introduced most of this trace information.

Looks good to me at a first (quick) sight, except this bit which changes the ABI:

Quoted 2 lines

Quoted 2 lines

- TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+ TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d grp=%d res=%d",

That's not how we change tracepoints generally - we add a new one and eventually phase out the old one.

Which apps/tools rely
on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

Note, it might make sense to send these as two patches to lkml with me Cc:-ed to avoid any github trust issues, i can apply them and push them to Linus.

OK. So we should add the new tracepoint. Looks a bit ugly, but I understand your concerns.

Say, trace_send_signal(sig, info, t, group, result), OK?

Seiji, please double check this is all you need, it won't be simply to change this tracepoint again. While we are adding the new one, we can add/change something in TP_STRUCT__entry if you think this is needed.

Quoted 3 lines

Which apps/tools rely
on the old tracepoint? If it's exactly zero apps then we might be able to change it, but this needs to be investigated.

If only I knew. How can we investigate this? Hopefully nothing relies on the old tracepoint, but who knows.

OK. So we should add the new tracepoint. Looks a bit ugly, but I understand your concerns.

Hello,

Please pull from

git://github.com/utrace/linux sigtrace

Oleg Nesterov (2):
tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info
tracing: send_sigqueue() needs trace_signal_generate() too

 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   28 +++++++++----
 2 files changed, 41 insertions(+), 72 deletions(-)