* Q: tracing: can we change trace_signal_generate() signature?
@ 2011-11-21 19:19 Oleg Nesterov
2011-11-21 20:03 ` Steven Rostedt
0 siblings, 1 reply; 16+ messages in thread
From: Oleg Nesterov @ 2011-11-21 19:19 UTC (permalink / raw)
To: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu,
Seiji Aguchi, Steven Rostedt
Cc: linux-kernel
Hello,
Is it possible to change trace_signal_generate()'s args or this
is the part of the kernel ABI?
We have the "feature request". The customer wants to know was the
signal delivered or not, and why. We could add another trace_()
into __send_signal() but this looks ugly to me.
So. Can't we add
enum {
TRACE_SIGNAL_DELIVERED,
TRACE_SIGNAL_IGNORED_OR_BLOCKED,
TRACE_SIGNAL_ALREADY_PENDING,
}
and move trace_signal_generate() to the end of __send_signal()
with the additional argument(s) to avoid the new tracepoint?
If yes, then can't we also kill trace_signal_overflow_fail()
and trace_signal_lose_info()? We can simply add more
TRACE_SIGNAL_'s instead, this certainly looks better imho.
IOW. Ignoring the changes in include/trace/events/signal.h,
can the patch below work or the changes like this are not
allowed?
See also https://bugzilla.redhat.com/show_bug.cgi?id=738720
Thanks,
Oleg.
--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -1019,19 +1019,27 @@ static inline int legacy_queue(struct sigpending *signals, int sig)
return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
}
+enum {
+ TRACE_SIGNAL_DELIVERED,
+ TRACE_SIGNAL_IGNORED_OR_BLOCKED,
+ TRACE_SIGNAL_ALREADY_PENDING,
+ TRACE_SIGNAL_OVERFLOW_FAIL,
+ TRACE_SIGNAL_LOSE_INFO,
+};
+
static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
int group, int from_ancestor_ns)
{
struct sigpending *pending;
struct sigqueue *q;
int override_rlimit;
-
- trace_signal_generate(sig, info, t);
+ int ret = 0, result;
assert_spin_locked(&t->sighand->siglock);
+ result = TRACE_SIGNAL_IGNORED_OR_BLOCKED;
if (!prepare_signal(sig, t, from_ancestor_ns))
- return 0;
+ goto ret;
pending = group ? &t->signal->shared_pending : &t->pending;
/*
@@ -1039,8 +1047,11 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
* exactly one non-rt signal, so that we can get more
* detailed information about the cause of the signal.
*/
+ result = TRACE_SIGNAL_ALREADY_PENDING;
if (legacy_queue(pending, sig))
- return 0;
+ goto ret;
+
+ result = TRACE_SIGNAL_DELIVERED;
/*
* fast-pathed signals for kernel-internal things like SIGSTOP
* or SIGKILL.
@@ -1095,14 +1106,15 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
* signal was rt and sent by user using something
* other than kill().
*/
- trace_signal_overflow_fail(sig, group, info);
- return -EAGAIN;
+ result = TRACE_SIGNAL_OVERFLOW_FAIL;
+ ret = -EAGAIN;
+ goto ret;
} else {
/*
* This is a silent loss of information. We still
* send the signal, but the *info bits are lost.
*/
- trace_signal_lose_info(sig, group, info);
+ result = TRACE_SIGNAL_LOSE_INFO;
}
}
@@ -1110,7 +1122,9 @@ out_set:
signalfd_notify(t, sig);
sigaddset(&pending->signal, sig);
complete_signal(sig, t, group);
- return 0;
+ret:
+ trace_signal_generate(sig, info, t, group, result);
+ return ret;
}
static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: Q: tracing: can we change trace_signal_generate() signature? 2011-11-21 19:19 Q: tracing: can we change trace_signal_generate() signature? Oleg Nesterov @ 2011-11-21 20:03 ` Steven Rostedt 2011-11-21 20:21 ` Oleg Nesterov 0 siblings, 1 reply; 16+ messages in thread From: Steven Rostedt @ 2011-11-21 20:03 UTC (permalink / raw) To: Oleg Nesterov Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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, ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Q: tracing: can we change trace_signal_generate() signature? 2011-11-21 20:03 ` Steven Rostedt @ 2011-11-21 20:21 ` Oleg Nesterov 2011-11-21 21:52 ` Steven Rostedt 0 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2011-11-21 20:21 UTC (permalink / raw) To: Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Q: tracing: can we change trace_signal_generate() signature? 2011-11-21 20:21 ` Oleg Nesterov @ 2011-11-21 21:52 ` Steven Rostedt 2011-11-22 20:52 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Oleg Nesterov 0 siblings, 1 reply; 16+ messages in thread From: Steven Rostedt @ 2011-11-21 21:52 UTC (permalink / raw) To: Oleg Nesterov Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) 2011-11-21 21:52 ` Steven Rostedt @ 2011-11-22 20:52 ` Oleg Nesterov 2011-11-22 20:52 ` [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Oleg Nesterov @ 2011-11-22 20:52 UTC (permalink / raw) To: Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info 2011-11-22 20:52 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Oleg Nesterov @ 2011-11-22 20:52 ` Oleg Nesterov 2011-11-23 1:43 ` Li Zefan 2011-11-22 20:53 ` [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov 2011-12-02 17:53 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Steven Rostedt 2 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2011-11-22 20:52 UTC (permalink / raw) To: Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info 2011-11-22 20:52 ` [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov @ 2011-11-23 1:43 ` Li Zefan 2011-11-23 17:37 ` Oleg Nesterov 0 siblings, 1 reply; 16+ messages in thread From: Li Zefan @ 2011-11-23 1:43 UTC (permalink / raw) To: Oleg Nesterov Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel > /** > * 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 ;) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info 2011-11-23 1:43 ` Li Zefan @ 2011-11-23 17:37 ` Oleg Nesterov 2011-11-30 16:24 ` Seiji Aguchi 0 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2011-11-23 17:37 UTC (permalink / raw) To: Li Zefan Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info 2011-11-23 17:37 ` Oleg Nesterov @ 2011-11-30 16:24 ` Seiji Aguchi 0 siblings, 0 replies; 16+ messages in thread From: Seiji Aguchi @ 2011-11-30 16:24 UTC (permalink / raw) To: Oleg Nesterov, Li Zefan Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Jiri Olsa, linux-kernel, 'Masami.hiramatsu.pt@hitachi.com' >------------------------------------------------------------------------------ >[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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too 2011-11-22 20:52 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Oleg Nesterov 2011-11-22 20:52 ` [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov @ 2011-11-22 20:53 ` Oleg Nesterov 2011-11-30 16:24 ` Seiji Aguchi 2011-12-02 17:53 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Steven Rostedt 2 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2011-11-22 20:53 UTC (permalink / raw) To: Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too 2011-11-22 20:53 ` [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov @ 2011-11-30 16:24 ` Seiji Aguchi 0 siblings, 0 replies; 16+ messages in thread From: Seiji Aguchi @ 2011-11-30 16:24 UTC (permalink / raw) To: Oleg Nesterov, Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, linux-kernel, 'Masami.hiramatsu.pt@hitachi.com' >-----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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) 2011-11-22 20:52 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Oleg Nesterov 2011-11-22 20:52 ` [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov 2011-11-22 20:53 ` [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov @ 2011-12-02 17:53 ` Steven Rostedt 2011-12-19 17:04 ` [PATCH RESEND 0/2] tracing: signal tracepoints Oleg Nesterov 2 siblings, 1 reply; 16+ messages in thread From: Steven Rostedt @ 2011-12-02 17:53 UTC (permalink / raw) To: Oleg Nesterov Cc: Frederic Weisbecker, Ingo Molnar, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH RESEND 0/2] tracing: signal tracepoints 2011-12-02 17:53 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Steven Rostedt @ 2011-12-19 17:04 ` Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Oleg Nesterov @ 2011-12-19 17:04 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Andrew Morton Cc: Frederic Weisbecker, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH RESEND 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info 2011-12-19 17:04 ` [PATCH RESEND 0/2] tracing: signal tracepoints Oleg Nesterov @ 2011-12-19 17:05 ` Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov 2011-12-19 17:28 ` [PATCH RESEND 0/2] tracing: signal tracepoints Seiji Aguchi 2 siblings, 0 replies; 16+ messages in thread From: Oleg Nesterov @ 2011-12-19 17:05 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Andrew Morton Cc: Frederic Weisbecker, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel __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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH RESEND 2/2] tracing: send_sigqueue() needs trace_signal_generate() too 2011-12-19 17:04 ` [PATCH RESEND 0/2] tracing: signal tracepoints Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov @ 2011-12-19 17:05 ` Oleg Nesterov 2011-12-19 17:28 ` [PATCH RESEND 0/2] tracing: signal tracepoints Seiji Aguchi 2 siblings, 0 replies; 16+ messages in thread From: Oleg Nesterov @ 2011-12-19 17:05 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Andrew Morton Cc: Frederic Weisbecker, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH RESEND 0/2] tracing: signal tracepoints 2011-12-19 17:04 ` [PATCH RESEND 0/2] tracing: signal tracepoints Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov @ 2011-12-19 17:28 ` Seiji Aguchi 2 siblings, 0 replies; 16+ messages in thread From: Seiji Aguchi @ 2011-12-19 17:28 UTC (permalink / raw) To: Oleg Nesterov, Steven Rostedt, Ingo Molnar, Andrew Morton Cc: Frederic Weisbecker, Jiri Olsa, Masami Hiramatsu, Seiji Aguchi, linux-kernel 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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-12-19 17:28 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-11-21 19:19 Q: tracing: can we change trace_signal_generate() signature? Oleg Nesterov 2011-11-21 20:03 ` Steven Rostedt 2011-11-21 20:21 ` Oleg Nesterov 2011-11-21 21:52 ` Steven Rostedt 2011-11-22 20:52 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Oleg Nesterov 2011-11-22 20:52 ` [PATCH 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov 2011-11-23 1:43 ` Li Zefan 2011-11-23 17:37 ` Oleg Nesterov 2011-11-30 16:24 ` Seiji Aguchi 2011-11-22 20:53 ` [PATCH 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov 2011-11-30 16:24 ` Seiji Aguchi 2011-12-02 17:53 ` [PATCH 0/2] (Was: Q: tracing: can we change trace_signal_generate() signature?) Steven Rostedt 2011-12-19 17:04 ` [PATCH RESEND 0/2] tracing: signal tracepoints Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 1/2] tracing: let trace_signal_generate() report more info, kill overflow_fail/lose_info Oleg Nesterov 2011-12-19 17:05 ` [PATCH RESEND 2/2] tracing: send_sigqueue() needs trace_signal_generate() too Oleg Nesterov 2011-12-19 17:28 ` [PATCH RESEND 0/2] tracing: signal tracepoints Seiji Aguchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).