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

[Steven Rostedt <rostedt@goodmis.org>]

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,