Re: Q: tracing: can we change trace_signal_generate() signature?

Steven Rostedt
2011-11-21T20:03:25Z

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,