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

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

[Oleg Nesterov]

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,

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

[Steven Rostedt]

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,

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

[Oleg Nesterov]

On 11/21, Steven Rostedt wrote:
>
> 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.

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

> > 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.

> > +enum {
> > +	TRACE_SIGNAL_DELIVERED,
> > +	TRACE_SIGNAL_IGNORED_OR_BLOCKED,

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

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

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.

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

[Steven Rostedt]

On Mon, 2011-11-21 at 21:21 +0100, Oleg Nesterov wrote:

> > > 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? ;)

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

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

> 
> 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.

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

[Oleg Nesterov]

On 11/21, Steven Rostedt wrote:
>
> On Mon, 2011-11-21 at 21:21 +0100, Oleg Nesterov wrote:
>
> > 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 ;)

> > > > @@ -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?
> >
> > 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.

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

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 17df434..2de7208 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -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 */
diff --git a/kernel/signal.c b/kernel/signal.c
index b3f78d0..5f130f6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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

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

[Li Zefan]

>  /**
>   * 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 ;)

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

[Oleg Nesterov]

On 11/23, Li Zefan wrote:
>
> >  /**
> >   * 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 <oleg@redhat.com>
---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 17df434..2de7208 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -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 */
diff --git a/kernel/signal.c b/kernel/signal.c
index b3f78d0..5f130f6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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

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

[Seiji Aguchi]

>------------------------------------------------------------------------------
>[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.
>

Thanks. This looks good to me.
Reviewed-by: Seiji Aguchi <seiji.aguchi@hds.com>

>Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>---
> include/trace/events/signal.h |   85 +++++++++++------------------------------
> kernel/signal.c               |   22 +++++++----
> 2 files changed, 36 insertions(+), 71 deletions(-)
>
>diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
>index 17df434..2de7208 100644
>--- a/include/trace/events/signal.h
>+++ b/include/trace/events/signal.h
>@@ -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 */
>diff --git a/kernel/signal.c b/kernel/signal.c
>index b3f78d0..5f130f6 100644
>--- a/kernel/signal.c
>+++ b/kernel/signal.c
>@@ -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 majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

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

[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 <saguchi@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 5f130f6..7366f68 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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;
-- 
1.5.5.1

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

[Seiji Aguchi]

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] 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; linux-kernel@vger.kernel.org
>Subject: [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too
>
>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 <seiji.aguchi@hds.com>

>Reported-by: Seiji Aguchi <saguchi@redhat.com>
>Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>---
> kernel/signal.c |    6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
>diff --git a/kernel/signal.c b/kernel/signal.c
>index 5f130f6..7366f68 100644
>--- a/kernel/signal.c
>+++ b/kernel/signal.c
>@@ -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;
>--
>1.5.5.1
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

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

[Steven Rostedt]

On Tue, 2011-11-22 at 21:52 +0100, Oleg Nesterov wrote:
> O
> > 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.

> 
> 
> 
> 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.

> 
> 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 <rostedt@goodmis.org>

-- Steve

[PATCH RESEND 0/2] tracing: signal tracepoints

[Oleg Nesterov]

Steven, sorry for delay...

On 12/02, Steven Rostedt wrote:
>
> On Tue, 2011-11-22 at 21:52 +0100, Oleg Nesterov wrote:
> >
> > > 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.

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.

> 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.

> > 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.


> > 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 <rostedt@goodmis.org>

Thanks, included.

Oleg.

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

[Oleg Nesterov]

__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 <saguchi@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Seiji Aguchi <seiji.aguchi@hds.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 17df434..39a8a43 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -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 */
diff --git a/kernel/signal.c b/kernel/signal.c
index b3f78d0..5f130f6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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

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

[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 <saguchi@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Seiji Aguchi <seiji.aguchi@hds.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/signal.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 5f130f6..7366f68 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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;
-- 
1.5.5.1

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

[Seiji Aguchi]

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

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] 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; linux-kernel@vger.kernel.org
>Subject: [PATCH RESEND 0/2] tracing: signal tracepoints
>
>Steven, sorry for delay...
>
>On 12/02, Steven Rostedt wrote:
>>
>> On Tue, 2011-11-22 at 21:52 +0100, Oleg Nesterov wrote:
>> >
>> > > 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.
>
>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.
>
>> 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.
>
>> > 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.
>
>
>> > 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 <rostedt@goodmis.org>
>
>Thanks, included.
>
>Oleg.
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] tracing: make signal tracepoints more useful

[Oleg Nesterov]

Hello.

Linus, I am asking you to review (and hopefully apply) these
changes, I do not know who else can do this. I do not mean the
implementation, the patches are simple. Just the behavioural
change.

2/2 looks like a bugfix to me, but 1/2 changes the output from
trace_signal_generate() and removes trace_signal_overflow_fail.
In essence the change is:

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

where
	- grp=0/1 means private or shared

	- res is enum {
			TRACE_SIGNAL_DELIVERED,
			TRACE_SIGNAL_IGNORED,
			TRACE_SIGNAL_ALREADY_PENDING,
			TRACE_SIGNAL_OVERFLOW_FAIL,
			TRACE_SIGNAL_LOSE_INFO,
		};

Obviously this is the user visible change. But personally I do
agree with Seiji who requested this feature. Currently
trace_signal_generate() just records the fact that __send_signal()
was called, you can't know whether the signal was actually sent
or not.

 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   28 +++++++++----
 2 files changed, 41 insertions(+), 72 deletions(-)

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

[Oleg Nesterov]

__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 <saguchi@redhat.com>
Reviewed-by: Seiji Aguchi <seiji.aguchi@hds.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/trace/events/signal.h |   85 +++++++++++------------------------------
 kernel/signal.c               |   22 +++++++----
 2 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index 17df434..39a8a43 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -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 */
diff --git a/kernel/signal.c b/kernel/signal.c
index bb0efa5..4515114 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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