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

* Q: tracing: can we change trace_signal_generate() signature?
@ 2011-11-21 19:19 Oleg Nesterov
  2011-11-21 20:03 ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Oleg Nesterov @ 2011-11-21 19:19 UTC (permalink / raw)
  To: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu,
	Seiji Aguchi, Steven Rostedt
  Cc: linux-kernel

Hello,

Is it possible to change trace_signal_generate()'s args or this
is the part of the kernel ABI?

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.

IOW. Ignoring the changes in include/trace/events/signal.h,
can the patch below work or the changes like this are not
allowed?

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;
 		}
 	}
 
@@ -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,


^ permalink raw reply	[flat|nested] 16+ messages in thread