Messages in this thread | | | Subject | Re: Q: tracing: can we change trace_signal_generate() signature? | From | Steven Rostedt <> | Date | Mon, 21 Nov 2011 15:03:19 -0500 |
| |
On Mon, 2011-11-21 at 20:19 +0100, Oleg Nesterov wrote: > Hello, > > Is it possible to change trace_signal_generate()'s args or this > is the part of the kernel ABI?
As Linus said. It's only part of the ABI if a tool is using it. If you change it and no one complains, then it should be good to go.
> > We have the "feature request". The customer wants to know was the > signal delivered or not, and why. We could add another trace_() > into __send_signal() but this looks ugly to me. > > So. Can't we add > > enum { > TRACE_SIGNAL_DELIVERED, > TRACE_SIGNAL_IGNORED_OR_BLOCKED, > TRACE_SIGNAL_ALREADY_PENDING, > } > > and move trace_signal_generate() to the end of __send_signal() > with the additional argument(s) to avoid the new tracepoint? > > If yes, then can't we also kill trace_signal_overflow_fail() > and trace_signal_lose_info()? We can simply add more > TRACE_SIGNAL_'s instead, this certainly looks better imho.
Again, if no tool relies on it, it should be fine.
If we were finally able to get a library for tools to read tracepoints, then we could add and move them around with no issue.
> > IOW. Ignoring the changes in include/trace/events/signal.h, > can the patch below work or the changes like this are not > allowed?
I say change it and see who screams.
> > See also https://bugzilla.redhat.com/show_bug.cgi?id=738720 > > Thanks, > > Oleg. > > > --- x/kernel/signal.c > +++ x/kernel/signal.c > @@ -1019,19 +1019,27 @@ static inline int legacy_queue(struct sigpending *signals, int sig) > return (sig < SIGRTMIN) && sigismember(&signals->signal, sig); > } > > +enum { > + TRACE_SIGNAL_DELIVERED, > + TRACE_SIGNAL_IGNORED_OR_BLOCKED, > + TRACE_SIGNAL_ALREADY_PENDING, > + TRACE_SIGNAL_OVERFLOW_FAIL, > + TRACE_SIGNAL_LOSE_INFO, > +}; > + > static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, > int group, int from_ancestor_ns) > { > struct sigpending *pending; > struct sigqueue *q; > int override_rlimit; > - > - trace_signal_generate(sig, info, t); > + int ret = 0, result; > > assert_spin_locked(&t->sighand->siglock); > > + result = TRACE_SIGNAL_IGNORED_OR_BLOCKED; > if (!prepare_signal(sig, t, from_ancestor_ns)) > - return 0; > + goto ret; > > pending = group ? &t->signal->shared_pending : &t->pending; > /* > @@ -1039,8 +1047,11 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, > * exactly one non-rt signal, so that we can get more > * detailed information about the cause of the signal. > */ > + result = TRACE_SIGNAL_ALREADY_PENDING; > if (legacy_queue(pending, sig)) > - return 0; > + goto ret; > + > + result = TRACE_SIGNAL_DELIVERED; > /* > * fast-pathed signals for kernel-internal things like SIGSTOP > * or SIGKILL. > @@ -1095,14 +1106,15 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, > * signal was rt and sent by user using something > * other than kill(). > */ > - trace_signal_overflow_fail(sig, group, info); > - return -EAGAIN; > + result = TRACE_SIGNAL_OVERFLOW_FAIL; > + ret = -EAGAIN; > + goto ret; > } else { > /* > * This is a silent loss of information. We still > * send the signal, but the *info bits are lost. > */ > - trace_signal_lose_info(sig, group, info); > + result = TRACE_SIGNAL_LOSE_INFO;
Hmm, all this result manipulation added for tracing that doesn't occur in 99.99% of all machines?
-- Steve
> } > } > > @@ -1110,7 +1122,9 @@ out_set: > signalfd_notify(t, sig); > sigaddset(&pending->signal, sig); > complete_signal(sig, t, group); > - return 0; > +ret: > + trace_signal_generate(sig, info, t, group, result); > + return ret; > } > > static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
| |