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

PATCH In review Oleg Nesterov 4 participants 16 messages Nov 21, 2011 – Dec 19, 2011 Tracing

lore

Share & tools

lore

Oleg Nesterov

author

2011-11-21 19:24 UTC #0 lore raw

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?

Thanks,

Oleg.

kernel/signal.c +22 -8

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

Steven Rostedt

maintainer

2011-11-21 20:03 UTC #1 lore raw

Quoted 4 lines · Oleg Nesterov

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.

Quoted 13 lines

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

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 60 lines

Thanks,

Oleg.

kernel/signal.c +15 -6

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

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1117

Quoted 10 lines

kernel/signal.c +4 -1

+			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

Quoted 14 lines

kernel/signal.c +3 -1

 		}
 	}

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

Oleg Nesterov

author

2011-11-21 20:25 UTC #2 lore raw

Quoted 9 lines · Steven Rostedt

Quoted 4 lines · Oleg Nesterov
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.

I only I knew if it is used (and how) or not...

Quoted 5 lines

Quoted 3 lines
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.

Heh. How can I do this? The only thing I can do is: send the patch to the maintainer - you ;)

OK. I'll send the patch "officially" tomorrow, let's see who nacks it.

Quoted 3 lines

kernel/signal.c static inline int legacy_queue(struct sigpending *signals, int sig) · line 1024
Quoted 3 lines
kernel/signal.c +3 -0
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED_OR_BLOCKED,

(can't understand why I added _OR_BLOCKED, it should be
TRACE_SIGNAL_IGNORED)

Quoted 11 lines

kernel/signal.c +3 -2

@@ -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 {
 			/*

Quoted 8 lines

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1117
Quoted 5 lines
kernel/signal.c +1 -1
 			 * 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?

Not sure I understand...

With this patch trace_signal_generate() also reports "result" which allows to know was the signal actually delivered or not. And, if not, why it wasn't delivered.

TRACE_SIGNAL_OVERFLOW_FAIL and TRACE_SIGNAL_LOSE_INFO are not really needed, but this way we can kill trace_signal_overflow_fail() and trace_signal_lose_info() and simplify the code.

Oleg.

Steven Rostedt

maintainer

2011-11-21 21:52 UTC #3 lore raw

Quoted 10 lines · Oleg Nesterov

Quoted 5 lines
Quoted 3 lines
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.
Heh. How can I do this? The only thing I can do is: send the patch to the maintainer - you ;)
OK. I'll send the patch "officially" tomorrow, let's see who nacks it.

I only maintain the tracing infrastructure. The tracepoint users are maintained by the subsystem they are used in. Who's the signal maintainer? ;)

Quoted 7 lines

Quoted 4 lines
kernel/signal.c static inline int legacy_queue(struct sigpending *signals, int sig) · line 1024
Quoted 4 lines
kernel/signal.c +3 -0
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED_OR_BLOCKED,
(can't understand why I added _OR_BLOCKED, it should be
TRACE_SIGNAL_IGNORED)

quilt refresh?

Quoted 21 lines

Quoted 12 lines

kernel/signal.c +3 -2

@@ -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 {
 			/*

Quoted 8 lines

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1117
Quoted 5 lines
kernel/signal.c +1 -1
 			 * 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?

Quoted 1 line

Not sure I understand...

Is "result" used for anything but tracepoints? When tracing is disabled, the tracepoints should be just nops (when jump_label is enabled). Thus tracing is very light. But if we are constantly calculating "result", this is unused by those that don't use the tracing infrastructure, which is 99.99% of all users. This is what I meant.

-- Steve

Quoted 10 lines

With this patch trace_signal_generate() also reports "result" which allows to know was the signal actually delivered or not. And, if not, why it wasn't delivered.
TRACE_SIGNAL_OVERFLOW_FAIL and TRACE_SIGNAL_LOSE_INFO are not really needed, but this way we can kill trace_signal_overflow_fail() and trace_signal_lose_info() and simplify the code.
Oleg.

Oleg Nesterov

author

2011-11-22 20:57 UTC #4 lore raw

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

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · Oleg Nesterov
Heh. How can I do this? The only thing I can do is: send the patch to the maintainer - you ;)
OK. I'll send the patch "officially" tomorrow, let's see who nacks it.
I only maintain the tracing infrastructure. The tracepoint users are maintained by the subsystem they are used in. Who's the signal maintainer? ;)

You are trolling me ;)

Quoted 20 lines

Quoted 19 lines

Quoted 6 lines

kernel/signal.c +0 -2

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

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1117

Quoted 10 lines

kernel/signal.c +4 -1

+			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?

Quoted 7 lines

Quoted 1 line
Not sure I understand...
Is "result" used for anything but tracepoints? When tracing is disabled, the tracepoints should be just nops (when jump_label is enabled). Thus tracing is very light. But if we are constantly calculating "result", this is unused by those that don't use the tracing infrastructure, which is 99.99% of all users. This is what I meant.

Ah I see. I thought you dislike OVERFLOW_FAIL/LOSE_INFO namely.

Of course, you are right. OTOH, this patch shaves 1058 bytes from .text. And without CONFIG_TRACE* gcc doesn't generate the extra code.

Oh. I simply do not know what can I do. Obviously, I'd like to avoid the new tracepoints in __send_signal(), imho this would be ugly. But the users want more info.

OK. let me send the patch at least for review. May be someone will nack it authoritatively, in this case I can relax and forward the nack back to bugzilla ;)

However, at least 2/2 looks very reasonable to me. In fact it looks almost like the bug-fix.

Oleg.

Oleg Nesterov

author

2011-11-22 20:57 UTC #5 lore raw

PATCH 1/2 tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

Incompatible change.

__send_signal()->trace_signal_generate() doesn't report enough info. The users want to know was the signal actually delivered or not, and they also need the shared/private info.

The patch moves trace_signal_generate() at the end of __send_signal() and adds the 2 additional arguments.

This also allows us to kill trace_signal_overflow_fail/lose_info, we can simply add the appropriate TRACE_SIGNAL_ "result" codes.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

include/trace/events/signal.h +22 ~~-63~~

index 17df434..2de7208 100644
@@ -23,11 +23,23 @@
 		}						\
 	} while (0)
 
+#ifndef TRACE_HEADER_MULTI_READ
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED,
+	TRACE_SIGNAL_ALREADY_PENDING,
+	TRACE_SIGNAL_OVERFLOW_FAIL,
+	TRACE_SIGNAL_LOSE_INFO,
+};
+#endif
+
 /**
  * signal_generate - called when a signal is generated
  * @sig: signal number
  * @info: pointer to struct siginfo
  * @task: pointer to struct task_struct
+ * @group: shared or private
+ * @resutl: TRACE_SIGNAL_*
  *
  * Current process sends a 'sig' signal to 'task' process with
  * 'info' siginfo. If 'info' is SEND_SIG_NOINFO or SEND_SIG_PRIV,

@@ -37,9 +49,10 @@
  */
 TRACE_EVENT(signal_generate,
 
-	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task,
+			int group, int result),
 
-	TP_ARGS(sig, info, task),
+	TP_ARGS(sig, info, task, group, result),
 
 	TP_STRUCT__entry(
 		__field(	int,	sig			)

@@ -47,6 +60,8 @@ TRACE_EVENT(signal_generate,
 		__field(	int,	code			)
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
+		__field(	int,	group			)
+		__field(	int,	result			)
 	),
 
 	TP_fast_assign(

@@ -54,11 +69,14 @@ TRACE_EVENT(signal_generate,
 		TP_STORE_SIGINFO(__entry, info);
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
 		__entry->pid	= task->pid;
+		__entry->group	= group;
+		__entry->result	= result;
 	),
 
-	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",
 		  __entry->sig, __entry->errno, __entry->code,
-		  __entry->comm, __entry->pid)
+		  __entry->comm, __entry->pid, __entry->group,
+		  __entry->result)
 );
 
 /**

@@ -101,65 +119,6 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sa_handler, __entry->sa_flags)
 );
 
-DECLARE_EVENT_CLASS(signal_queue_overflow,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig	)
-		__field(	int,	group	)
-		__field(	int,	errno	)
-		__field(	int,	code	)
-	),
-
-	TP_fast_assign(
-		__entry->sig	= sig;
-		__entry->group	= group;
-		TP_STORE_SIGINFO(__entry, info);
-	),
-
-	TP_printk("sig=%d group=%d errno=%d code=%d",
-		  __entry->sig, __entry->group, __entry->errno, __entry->code)
-);
-
-/**
- * signal_overflow_fail - called when signal queue is overflow
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel fails to generate 'sig' signal with 'info' siginfo, because
- * siginfo queue is overflow, and the signal is dropped.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_overflow_fail,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
-/**
- * signal_lose_info - called when siginfo is lost
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel generates 'sig' signal but loses 'info' siginfo, because siginfo
- * queue is overflow.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of non-RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_lose_info,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */

kernel/signal.c +14 -8

index b3f78d0..5f130f6 100644
@@ -1025,13 +1025,13 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	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;
 	if (!prepare_signal(sig, t, from_ancestor_ns))
-		return 0;
+		goto ret;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	/*

@@ -1039,8 +1039,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 +1098,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 +1114,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,

Signature · 1 line

1.5.5.1

Li Zefan

2011-11-23 01:42 UTC #6 lore raw

Re: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

Quoted 8 lines

/**
* signal_generate - called when a signal is generated
* @sig: signal number
* @info: pointer to struct siginfo
* @task: pointer to struct task_struct
+ * @group: shared or private
+ * @resutl: TRACE_SIGNAL_*
*

s/resutl/result ;)

Oleg Nesterov

author

2011-11-23 17:42 UTC #7 lore raw

Re: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

Quoted 11 lines · Li Zefan

Quoted 9 lines
/**
* signal_generate - called when a signal is generated
* @sig: signal number
* @info: pointer to struct siginfo
* @task: pointer to struct task_struct
+ * @group: shared or private
+ * @resutl: TRACE_SIGNAL_*
*
s/resutl/result ;)

I'll send the bugreport to gcc.gnu.org. Surely -Wall should catch such a typo!

------------------------------------------------------------------------------
[PATCH v2 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

Incompatible change.

__send_signal()->trace_signal_generate() doesn't report enough info. The users want to know was the signal actually delivered or not, and they also need the shared/private info.

The patch moves trace_signal_generate() at the end of __send_signal() and adds the 2 additional arguments.

This also allows us to kill trace_signal_overflow_fail/lose_info, we can simply add the appropriate TRACE_SIGNAL_ "result" codes.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

include/trace/events/signal.h +22 ~~-63~~

index 17df434..2de7208 100644
@@ -23,11 +23,23 @@
 		}						\
 	} while (0)
 
+#ifndef TRACE_HEADER_MULTI_READ
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED,
+	TRACE_SIGNAL_ALREADY_PENDING,
+	TRACE_SIGNAL_OVERFLOW_FAIL,
+	TRACE_SIGNAL_LOSE_INFO,
+};
+#endif
+
 /**
  * signal_generate - called when a signal is generated
  * @sig: signal number
  * @info: pointer to struct siginfo
  * @task: pointer to struct task_struct
+ * @group: shared or private
+ * @result: TRACE_SIGNAL_*
  *
  * Current process sends a 'sig' signal to 'task' process with
  * 'info' siginfo. If 'info' is SEND_SIG_NOINFO or SEND_SIG_PRIV,

@@ -37,9 +49,10 @@
  */
 TRACE_EVENT(signal_generate,
 
-	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task,
+			int group, int result),
 
-	TP_ARGS(sig, info, task),
+	TP_ARGS(sig, info, task, group, result),
 
 	TP_STRUCT__entry(
 		__field(	int,	sig			)

@@ -47,6 +60,8 @@ TRACE_EVENT(signal_generate,
 		__field(	int,	code			)
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
+		__field(	int,	group			)
+		__field(	int,	result			)
 	),
 
 	TP_fast_assign(

@@ -54,11 +69,14 @@ TRACE_EVENT(signal_generate,
 		TP_STORE_SIGINFO(__entry, info);
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
 		__entry->pid	= task->pid;
+		__entry->group	= group;
+		__entry->result	= result;
 	),
 
-	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",
 		  __entry->sig, __entry->errno, __entry->code,
-		  __entry->comm, __entry->pid)
+		  __entry->comm, __entry->pid, __entry->group,
+		  __entry->result)
 );
 
 /**

@@ -101,65 +119,6 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sa_handler, __entry->sa_flags)
 );
 
-DECLARE_EVENT_CLASS(signal_queue_overflow,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig	)
-		__field(	int,	group	)
-		__field(	int,	errno	)
-		__field(	int,	code	)
-	),
-
-	TP_fast_assign(
-		__entry->sig	= sig;
-		__entry->group	= group;
-		TP_STORE_SIGINFO(__entry, info);
-	),
-
-	TP_printk("sig=%d group=%d errno=%d code=%d",
-		  __entry->sig, __entry->group, __entry->errno, __entry->code)
-);
-
-/**
- * signal_overflow_fail - called when signal queue is overflow
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel fails to generate 'sig' signal with 'info' siginfo, because
- * siginfo queue is overflow, and the signal is dropped.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_overflow_fail,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
-/**
- * signal_lose_info - called when siginfo is lost
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel generates 'sig' signal but loses 'info' siginfo, because siginfo
- * queue is overflow.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of non-RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_lose_info,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */

kernel/signal.c +14 -8

index b3f78d0..5f130f6 100644
@@ -1025,13 +1025,13 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	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;
 	if (!prepare_signal(sig, t, from_ancestor_ns))
-		return 0;
+		goto ret;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	/*

@@ -1039,8 +1039,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 +1098,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 +1114,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,

Signature · 1 line

1.5.5.1

Seiji Aguchi

Reviewed-by

2011-11-30 16:24 UTC #8 lore raw

RE: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

Quoted 9 lines

------------------------------------------------------------------------------
[PATCH v2 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info
Incompatible change.
__send_signal()->trace_signal_generate() doesn't report enough info. The users want to know was the signal actually delivered or not, and they also need the shared/private info.

Quoted 6 lines

The patch moves trace_signal_generate() at the end of __send_signal() and adds the 2 additional arguments.
This also allows us to kill trace_signal_overflow_fail/lose_info, we can simply add the appropriate TRACE_SIGNAL_ "result" codes.

Thanks. This looks good to me.

Reviewed-by Seiji Aguchi <[email protected]>

Quoted 212 lines

Signed-off-by Oleg Nesterov <[email protected]>

---

include/trace/events/signal.h |   85 +++++++++++------------------------------
kernel/signal.c               |   22 +++++++----
2 files changed, 36 insertions(+), 71 deletions(-)

include/trace/events/signal.h +22 ~~-63~~

index 17df434..2de7208 100644
@@ -23,11 +23,23 @@
 		}						\
 	} while (0)
 
+#ifndef TRACE_HEADER_MULTI_READ
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED,
+	TRACE_SIGNAL_ALREADY_PENDING,
+	TRACE_SIGNAL_OVERFLOW_FAIL,
+	TRACE_SIGNAL_LOSE_INFO,
+};
+#endif
+
 /**
 * signal_generate - called when a signal is generated
 * @sig: signal number
 * @info: pointer to struct siginfo
 * @task: pointer to struct task_struct
+ * @group: shared or private
+ * @result: TRACE_SIGNAL_*
 *
 * Current process sends a 'sig' signal to 'task' process with
 * 'info' siginfo. If 'info' is SEND_SIG_NOINFO or SEND_SIG_PRIV,

@@ -37,9 +49,10 @@
 */
 TRACE_EVENT(signal_generate,
 
-	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task,
+			int group, int result),
 
-	TP_ARGS(sig, info, task),
+	TP_ARGS(sig, info, task, group, result),
 
 	TP_STRUCT__entry(
 		__field(	int,	sig			)

@@ -47,6 +60,8 @@ TRACE_EVENT(signal_generate,
 		__field(	int,	code			)
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
+		__field(	int,	group			)
+		__field(	int,	result			)
 	),
 
 	TP_fast_assign(

@@ -54,11 +69,14 @@ TRACE_EVENT(signal_generate,
 		TP_STORE_SIGINFO(__entry, info);
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
 		__entry->pid	= task->pid;
+		__entry->group	= group;
+		__entry->result	= result;
 	),
 
-	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",
 		  __entry->sig, __entry->errno, __entry->code,
-		  __entry->comm, __entry->pid)
+		  __entry->comm, __entry->pid, __entry->group,
+		  __entry->result)
 );
 
 /**

@@ -101,65 +119,6 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sa_handler, __entry->sa_flags)
 );
 
-DECLARE_EVENT_CLASS(signal_queue_overflow,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig	)
-		__field(	int,	group	)
-		__field(	int,	errno	)
-		__field(	int,	code	)
-	),
-
-	TP_fast_assign(
-		__entry->sig	= sig;
-		__entry->group	= group;
-		TP_STORE_SIGINFO(__entry, info);
-	),
-
-	TP_printk("sig=%d group=%d errno=%d code=%d",
-		  __entry->sig, __entry->group, __entry->errno, __entry->code)
-);
-
-/**
- * signal_overflow_fail - called when signal queue is overflow
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel fails to generate 'sig' signal with 'info' siginfo, because
- * siginfo queue is overflow, and the signal is dropped.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_overflow_fail,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
-/**
- * signal_lose_info - called when siginfo is lost
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel generates 'sig' signal but loses 'info' siginfo, because siginfo
- * queue is overflow.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of non-RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_lose_info,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */

kernel/signal.c +14 -9

index b3f78d0..5f130f6 100644
@@ -1025,13 +1025,13 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	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;
 	if (!prepare_signal(sig, t, from_ancestor_ns))
-		return 0;
+		goto ret;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	/*

@@ -1039,8 +1039,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 +1098,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 +1114,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,
--

1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Oleg Nesterov

author

2011-11-22 20:58 UTC #9 lore raw

PATCH 2/2 tracing: send_sigqueue() needs trace_signal_generate() too

In reply to Oleg Nesterov

Add trace_signal_generate() into send_sigqueue().

send_sigqueue() is very similar to __send_signal(), just it uses the preallocated info. It should do the same wrt tracing.

Reported-by Seiji Aguchi <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/signal.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

kernel/signal.c +5 -1

index 5f130f6..7366f68 100644
@@ -1559,7 +1559,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
 	unsigned long flags;
-	int ret;
+	int ret, result;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));

@@ -1568,6 +1568,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		goto ret;
 
 	ret = 1; /* the signal is ignored */
+	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, 0))
 		goto out;

@@ -1579,6 +1580,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		 */
 		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
+		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 	q->info.si_overrun = 0;

@@ -1588,7 +1590,9 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	list_add_tail(&q->list, &pending->list);
 	sigaddset(&pending->signal, sig);
 	complete_signal(sig, t, group);
+	result = TRACE_SIGNAL_DELIVERED;
 out:
+	trace_signal_generate(sig, &q->info, t, group, result);
 	unlock_task_sighand(t, &flags);
 ret:
 	return ret;

Signature · 1 line

1.5.5.1

Seiji Aguchi

Reviewed-by on v1 2/2

2011-11-30 16:24 UTC #10 lore raw

RE: [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too

Quoted 7 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Oleg Nesterov
Sent: Tuesday, November 22, 2011 3:53 PM
To: Steven Rostedt
Cc: Frederic Weisbecker; Ingo Molnar; Jiri Olsa; Masami Hiramatsu; Seiji Aguchi; [email protected]
Subject: [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too

Quoted 5 lines

Add trace_signal_generate() into send_sigqueue().
send_sigqueue() is very similar to __send_signal(), just it uses the preallocated info. It should do the same wrt tracing.

Looks good.

Reviewed-by Seiji Aguchi <[email protected]>

Quoted 54 lines

Reported-by Seiji Aguchi <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
kernel/signal.c | 6 +++++-

1 files changed, 5 insertions(+), 1 deletions(-)

kernel/signal.c +4 -1

index 5f130f6..7366f68 100644
@@ -1559,7 +1559,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
 	unsigned long flags;
-	int ret;
+	int ret, result;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));

@@ -1568,6 +1568,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		goto ret;
 
 	ret = 1; /* the signal is ignored */
+	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, 0))
 		goto out;

@@ -1579,6 +1580,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		 */
 		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
+		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 	q->info.si_overrun = 0;

@@ -1588,7 +1590,9 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	list_add_tail(&q->list, &pending->list);
 	sigaddset(&pending->signal, sig);
 	complete_signal(sig, t, group);
+	result = TRACE_SIGNAL_DELIVERED;

out:

kernel/signal.c +1 -0

+	trace_signal_generate(sig, &q->info, t, group, result);

	unlock_task_sighand(t, &flags);
ret:
	return ret;

--
1.5.5.1

Steven Rostedt

maintainer Acked-by on the v1 series

2011-12-02 17:53 UTC #11 lore raw

Re: [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?)

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

O
Quoted 5 lines
Is "result" used for anything but tracepoints? When tracing is disabled, the tracepoints should be just nops (when jump_label is enabled). Thus tracing is very light. But if we are constantly calculating "result", this is unused by those that don't use the tracing infrastructure, which is 99.99% of all users. This is what I meant.
Ah I see. I thought you dislike OVERFLOW_FAIL/LOSE_INFO namely.
Of course, you are right. OTOH, this patch shaves 1058 bytes from .text. And without CONFIG_TRACE* gcc doesn't generate the extra code.

I was just noting that when tracing is disabled (CONFIG_TRACE* is set, like it is on distros, but tracing is not happening), that we have extra code. We usually strive to have tracing configured into the kernel, but produces no (actually as little as possible) overhead when not actively tracing.

That said, you know this code much more than I do. If this isn't a fast path, and spinning a few more CPU cycles and perhaps dirtying a few cache lines floats your boat. I'm OK with this change.

Quoted 10 lines

Oh. I simply do not know what can I do. Obviously, I'd like to avoid the new tracepoints in __send_signal(), imho this would be ugly. But the users want more info.
OK. let me send the patch at least for review. May be someone will nack it authoritatively, in this case I can relax and forward the nack back to bugzilla ;)

Again, if you don't think adding very slight overhead to this path is an issue. Go ahead and add it.

Quoted 3 lines

However, at least 2/2 looks very reasonable to me. In fact it looks almost like the bug-fix.

2/2 looks to have the extra overhead to. Is the bug fix just with the trace point.

Again, if you don't mind the overhead, then here:

Acked-by Steven Rostedt <[email protected]>

-- Steve

Oleg Nesterov

author

2011-12-19 17:10 UTC #12 lore raw

cover tracing: signal tracepoints

Steven, sorry for delay...

Quoted 11 lines · Steven Rostedt

Quoted 9 lines · Oleg Nesterov
Quoted 6 lines
Is "result" used for anything but tracepoints? When tracing is disabled, the tracepoints should be just nops (when jump_label is enabled). Thus tracing is very light. But if we are constantly calculating "result", this is unused by those that don't use the tracing infrastructure, which is 99.99% of all users. This is what I meant.
Ah I see. I thought you dislike OVERFLOW_FAIL/LOSE_INFO namely.

Quoted 8 lines

Quoted 2 lines
Of course, you are right. OTOH, this patch shaves 1058 bytes from .text. And without CONFIG_TRACE* gcc doesn't generate the extra code.
I was just noting that when tracing is disabled (CONFIG_TRACE* is set, like it is on distros, but tracing is not happening), that we have extra code. We usually strive to have tracing configured into the kernel, but produces no (actually as little as possible) overhead when not actively tracing.

Yes, yes, I see. But I do not see any alternative. Of course, instead of adding "int result" we could add more trace_signal_generate's into the code, but imho this is too ugly. And in fact I am not sure this means less overhead with CONFIG_TRACE* even if this code is nop'ed.

Quoted 3 lines

That said, you know this code much more than I do. If this isn't a fast path, and spinning a few more CPU cycles and perhaps dirtying a few cache lines floats your boat. I'm OK with this change.

I simply do not know. I _think_ that the overhead is negligible, the extra calculating just adds a couple of "mov CONSTANT, REGISTER" insns.

Quoted 10 lines

Quoted 7 lines
Oh. I simply do not know what can I do. Obviously, I'd like to avoid the new tracepoints in __send_signal(), imho this would be ugly. But the users want more info.
OK. let me send the patch at least for review. May be someone will nack it authoritatively, in this case I can relax and forward the nack back to bugzilla ;)
Again, if you don't think adding very slight overhead to this path is an issue. Go ahead and add it.

OK, thanks.

The next question is, how can I add it ;) May be Ingo or Andrew could take these patches? Original signal tracepoints were routed via tip-tree...

Add them both to TO:, lets see who is kinder.

Quoted 9 lines

Quoted 2 lines
However, at least 2/2 looks very reasonable to me. In fact it looks almost like the bug-fix.
2/2 looks to have the extra overhead to. Is the bug fix just with the trace point.
Again, if you don't mind the overhead, then here:
Acked-by Steven Rostedt <[email protected]>

Thanks, included.

Oleg.

Oleg Nesterov

author

2011-12-19 17:10 UTC #13 lore raw

PATCH RESEND 1/2 tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info

__send_signal()->trace_signal_generate() doesn't report enough info. The users want to know was the signal actually delivered or not, and they also need the shared/private info.

The patch moves trace_signal_generate() at the end of __send_signal() and adds the 2 additional arguments.

This also allows us to kill trace_signal_overflow_fail/lose_info, we can simply add the appropriate TRACE_SIGNAL_ "result" codes.

Reported-by Seiji Aguchi <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Seiji Aguchi <[email protected]>

Acked-by Steven Rostedt <[email protected]>

---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

include/trace/events/signal.h +22 ~~-63~~

index 17df434..39a8a43 100644
@@ -23,11 +23,23 @@
 		}						\
 	} while (0)
 
+#ifndef TRACE_HEADER_MULTI_READ
+enum {
+	TRACE_SIGNAL_DELIVERED,
+	TRACE_SIGNAL_IGNORED,
+	TRACE_SIGNAL_ALREADY_PENDING,
+	TRACE_SIGNAL_OVERFLOW_FAIL,
+	TRACE_SIGNAL_LOSE_INFO,
+};
+#endif
+
 /**
  * signal_generate - called when a signal is generated
  * @sig: signal number
  * @info: pointer to struct siginfo
  * @task: pointer to struct task_struct
+ * @group: shared or private
+ * @result: TRACE_SIGNAL_*
  *
  * Current process sends a 'sig' signal to 'task' process with
  * 'info' siginfo. If 'info' is SEND_SIG_NOINFO or SEND_SIG_PRIV,

@@ -37,9 +49,10 @@
  */
 TRACE_EVENT(signal_generate,
 
-	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task,
+			int group, int result),
 
-	TP_ARGS(sig, info, task),
+	TP_ARGS(sig, info, task, group, result),
 
 	TP_STRUCT__entry(
 		__field(	int,	sig			)

@@ -47,6 +60,8 @@ TRACE_EVENT(signal_generate,
 		__field(	int,	code			)
 		__array(	char,	comm,	TASK_COMM_LEN	)
 		__field(	pid_t,	pid			)
+		__field(	int,	group			)
+		__field(	int,	result			)
 	),
 
 	TP_fast_assign(

@@ -54,11 +69,14 @@ TRACE_EVENT(signal_generate,
 		TP_STORE_SIGINFO(__entry, info);
 		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
 		__entry->pid	= task->pid;
+		__entry->group	= group;
+		__entry->result	= result;
 	),
 
-	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",
 		  __entry->sig, __entry->errno, __entry->code,
-		  __entry->comm, __entry->pid)
+		  __entry->comm, __entry->pid, __entry->group,
+		  __entry->result)
 );
 
 /**

@@ -101,65 +119,6 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sa_handler, __entry->sa_flags)
 );
 
-DECLARE_EVENT_CLASS(signal_queue_overflow,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig	)
-		__field(	int,	group	)
-		__field(	int,	errno	)
-		__field(	int,	code	)
-	),
-
-	TP_fast_assign(
-		__entry->sig	= sig;
-		__entry->group	= group;
-		TP_STORE_SIGINFO(__entry, info);
-	),
-
-	TP_printk("sig=%d group=%d errno=%d code=%d",
-		  __entry->sig, __entry->group, __entry->errno, __entry->code)
-);
-
-/**
- * signal_overflow_fail - called when signal queue is overflow
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel fails to generate 'sig' signal with 'info' siginfo, because
- * siginfo queue is overflow, and the signal is dropped.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_overflow_fail,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
-/**
- * signal_lose_info - called when siginfo is lost
- * @sig: signal number
- * @group: signal to process group or not (bool)
- * @info: pointer to struct siginfo
- *
- * Kernel generates 'sig' signal but loses 'info' siginfo, because siginfo
- * queue is overflow.
- * 'group' is not 0 if the signal will be sent to a process group.
- * 'sig' is always one of non-RT signals.
- */
-DEFINE_EVENT(signal_queue_overflow, signal_lose_info,
-
-	TP_PROTO(int sig, int group, struct siginfo *info),
-
-	TP_ARGS(sig, group, info)
-);
-
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */

kernel/signal.c +14 -8

index b3f78d0..5f130f6 100644
@@ -1025,13 +1025,13 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	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;
 	if (!prepare_signal(sig, t, from_ancestor_ns))
-		return 0;
+		goto ret;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	/*

@@ -1039,8 +1039,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 +1098,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 +1114,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,

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2011-12-19 17:10 UTC #14 lore raw

PATCH RESEND 2/2 tracing: send_sigqueue() needs trace_signal_generate() too

In reply to Oleg Nesterov

Add trace_signal_generate() into send_sigqueue().

send_sigqueue() is very similar to __send_signal(), just it uses the preallocated info. It should do the same wrt tracing.

Reported-by Seiji Aguchi <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Seiji Aguchi <[email protected]>

Acked-by Steven Rostedt <[email protected]>

---
 kernel/signal.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

kernel/signal.c +5 -1

index 5f130f6..7366f68 100644
@@ -1559,7 +1559,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
 	unsigned long flags;
-	int ret;
+	int ret, result;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));

@@ -1568,6 +1568,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		goto ret;
 
 	ret = 1; /* the signal is ignored */
+	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, 0))
 		goto out;

@@ -1579,6 +1580,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		 */
 		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
+		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 	q->info.si_overrun = 0;

@@ -1588,7 +1590,9 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 	list_add_tail(&q->list, &pending->list);
 	sigaddset(&pending->signal, sig);
 	complete_signal(sig, t, group);
+	result = TRACE_SIGNAL_DELIVERED;
 out:
+	trace_signal_generate(sig, &q->info, t, group, result);
 	unlock_task_sighand(t, &flags);
 ret:
 	return ret;

Signature · 1 line

1.5.5.1

Seiji Aguchi

2011-12-19 17:28 UTC #15 lore raw

RE: [PATCH RESEND 0/2] tracing: signal tracepoints

In reply to Oleg Nesterov

Hi,

Thanks, Oleg.

This patchset is helpful for me.

When some applications fail to send signal, our customers sometimes ask its reason to us. With this patchset, we can know behavior of signal event in detail and explain it to them.

Seiji

Quoted 80 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Oleg Nesterov
Sent: Monday, December 19, 2011 12:05 PM
To: Steven Rostedt; Ingo Molnar; Andrew Morton
Cc: Frederic Weisbecker; Jiri Olsa; Masami Hiramatsu; Seiji Aguchi; [email protected]
Subject: [PATCH RESEND 0/2] tracing: signal tracepoints
Steven, sorry for delay...
Quoted 11 lines · Steven Rostedt
Quoted 9 lines · Oleg Nesterov
Quoted 6 lines
Is "result" used for anything but tracepoints? When tracing is disabled, the tracepoints should be just nops (when jump_label is enabled). Thus tracing is very light. But if we are constantly calculating "result", this is unused by those that don't use the tracing infrastructure, which is 99.99% of all users. This is what I meant.
Ah I see. I thought you dislike OVERFLOW_FAIL/LOSE_INFO namely.
Quoted 8 lines
Quoted 2 lines
Of course, you are right. OTOH, this patch shaves 1058 bytes from .text. And without CONFIG_TRACE* gcc doesn't generate the extra code.
I was just noting that when tracing is disabled (CONFIG_TRACE* is set, like it is on distros, but tracing is not happening), that we have extra code. We usually strive to have tracing configured into the kernel, but produces no (actually as little as possible) overhead when not actively tracing.
Yes, yes, I see. But I do not see any alternative. Of course, instead of adding "int result" we could add more trace_signal_generate's into the code, but imho this is too ugly. And in fact I am not sure this means less overhead with CONFIG_TRACE* even if this code is nop'ed.
Quoted 3 lines
That said, you know this code much more than I do. If this isn't a fast path, and spinning a few more CPU cycles and perhaps dirtying a few cache lines floats your boat. I'm OK with this change.
I simply do not know. I _think_ that the overhead is negligible, the extra calculating just adds a couple of "mov CONSTANT, REGISTER" insns.
Quoted 10 lines
Quoted 7 lines
Oh. I simply do not know what can I do. Obviously, I'd like to avoid the new tracepoints in __send_signal(), imho this would be ugly. But the users want more info.
OK. let me send the patch at least for review. May be someone will nack it authoritatively, in this case I can relax and forward the nack back to bugzilla ;)
Again, if you don't think adding very slight overhead to this path is an issue. Go ahead and add it.
OK, thanks.
The next question is, how can I add it ;) May be Ingo or Andrew could take these patches? Original signal tracepoints were routed via tip-tree...
Add them both to TO:, lets see who is kinder.
Quoted 9 lines
Quoted 2 lines
However, at least 2/2 looks very reasonable to me. In fact it looks almost like the bug-fix.
2/2 looks to have the extra overhead to. Is the bug fix just with the trace point.
Again, if you don't mind the overhead, then here:
Acked-by Steven Rostedt <[email protected]>
Thanks, included.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/