[PATCH -tip v3 0/3] tracepoint: Add signal events

[PATCH -tip v3 0/3] tracepoint: Add signal events

[Masami Hiramatsu]

Hi,

These patches add signal related tracepoints including
signal generation, delivery, and loss. First patch also
moves signal-sending tracepoint from events/sched.h to
events/signal.h.

Changes in v3
- Add Docbook style comments

Changes in v2
- Add siginfo arguments

Thank you,

---

Masami Hiramatsu (3):
      tracepoint: Add signal loss events
      tracepoint: Add signal deliver event
      tracepoint: Move signal sending tracepoint to events/signal.h


 Documentation/DocBook/tracepoint.tmpl |    5 +
 include/trace/events/sched.h          |   25 -----
 include/trace/events/signal.h         |  173 +++++++++++++++++++++++++++++++++
 kernel/signal.c                       |   27 ++++-
 4 files changed, 198 insertions(+), 32 deletions(-)
 create mode 100644 include/trace/events/signal.h

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhiramat@redhat.com

[PATCH -tip v3 1/3] tracepoint: Move signal sending tracepoint to events/signal.h

[Masami Hiramatsu]

Move signal sending event to events/signal.h. This patch also renames
sched_signal_send event to signal_generate.

Changes in v3:
 - Add docbook style comments

Changes in v2:
 - Add siginfo argument
 - Add siginfo storing macro

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Roland McGrath <roland@redhat.com>
Cc: Jason Baron <jbaron@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
---

 Documentation/DocBook/tracepoint.tmpl |    5 +++
 include/trace/events/sched.h          |   25 -------------
 include/trace/events/signal.h         |   66 +++++++++++++++++++++++++++++++++
 kernel/signal.c                       |    5 ++-
 4 files changed, 74 insertions(+), 27 deletions(-)
 create mode 100644 include/trace/events/signal.h

diff --git a/Documentation/DocBook/tracepoint.tmpl b/Documentation/DocBook/tracepoint.tmpl
index b0756d0..8bca1d5 100644
--- a/Documentation/DocBook/tracepoint.tmpl
+++ b/Documentation/DocBook/tracepoint.tmpl
@@ -86,4 +86,9 @@
 !Iinclude/trace/events/irq.h
   </chapter>
 
+  <chapter id="signal">
+   <title>SIGNAL</title>
+!Iinclude/trace/events/signal.h
+  </chapter>
+
 </book>
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index b50b985..b221bb3 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -320,31 +320,6 @@ TRACE_EVENT(sched_process_fork,
 );
 
 /*
- * Tracepoint for sending a signal:
- */
-TRACE_EVENT(sched_signal_send,
-
-	TP_PROTO(int sig, struct task_struct *p),
-
-	TP_ARGS(sig, p),
-
-	TP_STRUCT__entry(
-		__field(	int,	sig			)
-		__array(	char,	comm,	TASK_COMM_LEN	)
-		__field(	pid_t,	pid			)
-	),
-
-	TP_fast_assign(
-		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
-		__entry->pid	= p->pid;
-		__entry->sig	= sig;
-	),
-
-	TP_printk("sig=%d comm=%s pid=%d",
-		  __entry->sig, __entry->comm, __entry->pid)
-);
-
-/*
  * XXX the below sched_stat tracepoints only apply to SCHED_OTHER/BATCH/IDLE
  *     adding sched_stat support to SCHED_FIFO/RR would be welcome.
  */
diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
new file mode 100644
index 0000000..af2e6b1
--- /dev/null
+++ b/include/trace/events/signal.h
@@ -0,0 +1,66 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM signal
+
+#if !defined(_TRACE_SIGNAL_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SIGNAL_H
+
+#include <linux/signal.h>
+#include <linux/sched.h>
+#include <linux/tracepoint.h>
+
+#define TP_STORE_SIGINFO(__entry, info)				\
+	do {							\
+		if (info == SEND_SIG_NOINFO) {			\
+			__entry->errno	= 0;			\
+			__entry->code	= SI_USER;		\
+		} else if (info == SEND_SIG_PRIV) {		\
+			__entry->errno	= 0;			\
+			__entry->code	= SI_KERNEL;		\
+		} else {					\
+			__entry->errno	= info->si_errno;	\
+			__entry->code	= info->si_code;	\
+		}						\
+	} while (0)
+
+/**
+ * signal_generate - called when a signal is generated
+ * @sig: signal number
+ * @info: pointer to struct siginfo
+ * @task: pointer to struct task_struct
+ *
+ * Current process sends a 'sig' signal to 'task' process with
+ * 'info' siginfo. If 'info' is SEND_SIG_NOINFO or SEND_SIG_PRIV,
+ * 'info' is not a pointer and you can't access its field. Instead,
+ * SEND_SIG_NOINFO means that si_code is SI_USER, and SEND_SIG_PRIV
+ * means that si_code is SI_KERNEL.
+ */
+TRACE_EVENT(signal_generate,
+
+	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
+
+	TP_ARGS(sig, info, p),
+
+	TP_STRUCT__entry(
+		__field(	int,	sig			)
+		__field(	int,	errno			)
+		__field(	int,	code			)
+		__array(	char,	comm,	TASK_COMM_LEN	)
+		__field(	pid_t,	pid			)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		TP_STORE_SIGINFO(__entry, info);
+		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->pid	= task->pid;
+	),
+
+	TP_printk("sig=%d errno=%d code=%d comm=%s pid=%d",
+		  __entry->sig, __entry->errno, __entry->code,
+		  __entry->comm, __entry->pid)
+);
+
+#endif /* _TRACE_SIGNAL_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/kernel/signal.c b/kernel/signal.c
index fe08008..54ac4c5 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -28,7 +28,8 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
-#include <trace/events/sched.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/signal.h>
 
 #include <asm/param.h>
 #include <asm/uaccess.h>
@@ -856,7 +857,7 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	struct sigqueue *q;
 	int override_rlimit;
 
-	trace_sched_signal_send(sig, t);
+	trace_signal_generate(sig, info, t);
 
 	assert_spin_locked(&t->sighand->siglock);
 


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

[PATCH -tip v3 2/3] tracepoint: Add signal deliver event

[Masami Hiramatsu]

Add a tracepoint where a process gets a signal. This tracepoint
shows signal-number, sa-handler and sa-flag.

Changes in v3:
 - Add docbook style comments

Changes in v2:
 - Add siginfo argument
 - Fix comment

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Roland McGrath <roland@redhat.com>
Cc: Jason Baron <jbaron@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
---

 include/trace/events/signal.h |   39 +++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |    3 +++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index af2e6b1..ddf89c9 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -60,6 +60,45 @@ TRACE_EVENT(signal_generate,
 		  __entry->comm, __entry->pid)
 );
 
+/**
+ * signal_deliver - called when a signal is delivered
+ * @sig: signal number
+ * @info: pointer to struct siginfo
+ * @ka: pointer to struct k_sigaction
+ *
+ * A 'sig' signal is delivered to current process with 'info' siginfo,
+ * and it will be handled by 'ka'. ka->sa.sa_handler can be SIG_IGN or
+ * SIG_DFL.
+ * Note that some signals reported by signal_generate tracepoint can be
+ * lost, ignored or modified (by debugger) before hitting this tracepoint.
+ * This means, this can show which signals are actually delivered, but
+ * matching generated signals and delivered signals may not be correct.
+ */
+TRACE_EVENT(signal_deliver,
+
+	TP_PROTO(int sig, struct siginfo *info, struct k_sigaction *ka),
+
+	TP_ARGS(sig, info, ka),
+
+	TP_STRUCT__entry(
+		__field(	int,		sig		)
+		__field(	int,		errno		)
+		__field(	int,		code		)
+		__field(	unsigned long,	sa_handler	)
+		__field(	unsigned long,	sa_flags	)
+	),
+
+	TP_fast_assign(
+		__entry->sig	= sig;
+		TP_STORE_SIGINFO(__entry, info);
+		__entry->sa_handler	= (unsigned long)ka->sa.sa_handler;
+		__entry->sa_flags	= ka->sa.sa_flags;
+	),
+
+	TP_printk("sig=%d errno=%d code=%d sa_handler=%lx sa_flags=%lx",
+		  __entry->sig, __entry->errno, __entry->code,
+		  __entry->sa_handler, __entry->sa_flags)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index 54ac4c5..d518984 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1860,6 +1860,9 @@ relock:
 			ka = &sighand->action[signr-1];
 		}
 
+		/* Trace actually delivered signals. */
+		trace_signal_deliver(signr, info, ka);
+
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
 		if (ka->sa.sa_handler != SIG_DFL) {


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

[PATCH -tip v3 3/3] tracepoint: Add signal loss events

[Masami Hiramatsu]

Add signal_overflow_fail and signal_lose_info tracepoints
for signal-lost events.

Changes in v3:
 - Add docbook style comments

Changes in v2:
 - Use siginfo string macro

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Suggested-by: Roland McGrath <roland@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jason Baron <jbaron@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
---

 include/trace/events/signal.h |   68 +++++++++++++++++++++++++++++++++++++++++
 kernel/signal.c               |   19 ++++++++---
 2 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
index ddf89c9..8404d8b 100644
--- a/include/trace/events/signal.h
+++ b/include/trace/events/signal.h
@@ -99,6 +99,74 @@ TRACE_EVENT(signal_deliver,
 		  __entry->sig, __entry->errno, __entry->code,
 		  __entry->sa_handler, __entry->sa_flags)
 );
+
+/**
+ * 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.
+ */
+TRACE_EVENT(signal_overflow_fail,
+
+	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_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.
+ */
+TRACE_EVENT(signal_lose_info,
+
+	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)
+);
 #endif /* _TRACE_SIGNAL_H */
 
 /* This part must be outside protection */
diff --git a/kernel/signal.c b/kernel/signal.c
index d518984..6b982f2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -919,12 +919,21 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			break;
 		}
 	} else if (!is_si_special(info)) {
-		if (sig >= SIGRTMIN && info->si_code != SI_USER)
-		/*
-		 * Queue overflow, abort.  We may abort if the signal was rt
-		 * and sent by user using something other than kill().
-		 */
+		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
+			/*
+			 * Queue overflow, abort.  We may abort if the
+			 * signal was rt and sent by user using something
+			 * other than kill().
+			 */
+			trace_signal_overflow_fail(sig, group, info);
 			return -EAGAIN;
+		} 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);
+		}
 	}
 
 out_set:


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Hi,
> 
> These patches add signal related tracepoints including
> signal generation, delivery, and loss. First patch also
> moves signal-sending tracepoint from events/sched.h to
> events/signal.h.
> 
> Changes in v3
> - Add Docbook style comments
> 
> Changes in v2
> - Add siginfo arguments
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       tracepoint: Add signal loss events
>       tracepoint: Add signal deliver event
>       tracepoint: Move signal sending tracepoint to events/signal.h
> 
> 
>  Documentation/DocBook/tracepoint.tmpl |    5 +
>  include/trace/events/sched.h          |   25 -----
>  include/trace/events/signal.h         |  173 +++++++++++++++++++++++++++++++++
>  kernel/signal.c                       |   27 ++++-
>  4 files changed, 198 insertions(+), 32 deletions(-)
>  create mode 100644 include/trace/events/signal.h

Would be nice to have Roland's and Oleg's Acked-by tags in the patches - 
to show that this is a representative and useful looking set of signal 
events.

	Ingo

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Jason Baron]

On Fri, Nov 20, 2009 at 04:31:08PM -0500, Masami Hiramatsu wrote:
> Hi,
> 
> These patches add signal related tracepoints including
> signal generation, delivery, and loss. First patch also
> moves signal-sending tracepoint from events/sched.h to
> events/signal.h.
> 
> Changes in v3
> - Add Docbook style comments
> 

Documentation bits look good. thanks for adding them.

Reviewed-by: Jason Baron <jbaron@redhat.com>

Re: [PATCH -tip v3 1/3] tracepoint: Move signal sending tracepoint to events/signal.h

[Oleg Nesterov]

On 11/20, Masami Hiramatsu wrote:
>
> +TRACE_EVENT(signal_generate,
> +
> +	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
> +
> +	TP_ARGS(sig, info, p),
                          ^^^
s/p/task/ ?

Oleg.

Re: [PATCH -tip v3 1/3] tracepoint: Move signal sending tracepoint to events/signal.h

[Masami Hiramatsu]

Oleg Nesterov wrote:
> On 11/20, Masami Hiramatsu wrote:
>>
>> +TRACE_EVENT(signal_generate,
>> +
>> +	TP_PROTO(int sig, struct siginfo *info, struct task_struct *task),
>> +
>> +	TP_ARGS(sig, info, p),
>                            ^^^
> s/p/task/ ?

Oops, right.

Thank you for reviewing!

>
> Oleg.
>

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Oleg Nesterov]

On 11/23, Ingo Molnar wrote:
>
> * Masami Hiramatsu <mhiramat@redhat.com> wrote:
>
> > Hi,
> >
> > These patches add signal related tracepoints including
> > signal generation, delivery, and loss. First patch also
> > moves signal-sending tracepoint from events/sched.h to
> > events/signal.h.
> >
> > Changes in v3
> > - Add Docbook style comments
> >
> > Changes in v2
> > - Add siginfo arguments
> >
> > Thank you,
> >
> > ---
> >
> > Masami Hiramatsu (3):
> >       tracepoint: Add signal loss events
> >       tracepoint: Add signal deliver event
> >       tracepoint: Move signal sending tracepoint to events/signal.h
> >
> >
> >  Documentation/DocBook/tracepoint.tmpl |    5 +
> >  include/trace/events/sched.h          |   25 -----
> >  include/trace/events/signal.h         |  173 +++++++++++++++++++++++++++++++++
> >  kernel/signal.c                       |   27 ++++-
> >  4 files changed, 198 insertions(+), 32 deletions(-)
> >  create mode 100644 include/trace/events/signal.h
>
> Would be nice to have Roland's and Oleg's Acked-by tags in the patches -
> to show that this is a representative and useful looking set of signal
> events.

Sorry, I can't really comment these patches.

I mean, I do not know which info is useful and which is not.
For example, I am a bit surprized we report trace_signal_lose_info()
but please do not consider this as if I think we shouldn't. Just I
do not know.

OTOH, we do not report if __send_signal() fails just because the
legacy signal is already queued. We do not report who sends the signal,
we do not report if it was private or shared. zap_process, complete_signal
can "send" SIGKILL via sigaddset, this won't be noticed. But again, it is
not that I think this should be reported.

In short: I think any info may be useful, and these patches can help.
But I do not understand what exactly should be reported to userspace.

Oleg.

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/23, Ingo Molnar wrote:
> >
> > * Masami Hiramatsu <mhiramat@redhat.com> wrote:
> >
> > > Hi,
> > >
> > > These patches add signal related tracepoints including
> > > signal generation, delivery, and loss. First patch also
> > > moves signal-sending tracepoint from events/sched.h to
> > > events/signal.h.
> > >
> > > Changes in v3
> > > - Add Docbook style comments
> > >
> > > Changes in v2
> > > - Add siginfo arguments
> > >
> > > Thank you,
> > >
> > > ---
> > >
> > > Masami Hiramatsu (3):
> > >       tracepoint: Add signal loss events
> > >       tracepoint: Add signal deliver event
> > >       tracepoint: Move signal sending tracepoint to events/signal.h
> > >
> > >
> > >  Documentation/DocBook/tracepoint.tmpl |    5 +
> > >  include/trace/events/sched.h          |   25 -----
> > >  include/trace/events/signal.h         |  173 +++++++++++++++++++++++++++++++++
> > >  kernel/signal.c                       |   27 ++++-
> > >  4 files changed, 198 insertions(+), 32 deletions(-)
> > >  create mode 100644 include/trace/events/signal.h
> >
> > Would be nice to have Roland's and Oleg's Acked-by tags in the patches -
> > to show that this is a representative and useful looking set of signal
> > events.
> 
> Sorry, I can't really comment these patches.
> 
> I mean, I do not know which info is useful and which is not. For 
> example, I am a bit surprized we report trace_signal_lose_info() but 
> please do not consider this as if I think we shouldn't. Just I do not 
> know.

well, there we lose information, so it's basically an exception/anomaly 
that a person doing analysis might be interested in.

> OTOH, we do not report if __send_signal() fails just because the 
> legacy signal is already queued. [...]

We could do that (beyond the queued signals full event), but i think 
it's rather common to see signal overlap in the legacy case, right?

> [...] We do not report who sends the signal, [...]

The PID of any task generating an event can be sampled, so that's 
implicit.

> [...] we do not report if it was private or shared. zap_process, 
> complete_signal can "send" SIGKILL via sigaddset, this won't be 
> noticed. But again, it is not that I think this should be reported.
> 
> In short: I think any info may be useful, and these patches can help. 
> But I do not understand what exactly should be reported to userspace.

The principe is this: there's two extremes:

 A- report no event

 B- report every event precisely, that allows all signal state and
    actions to be reconstructed in hindsight.

And there's a continuum between the two extremes. Just a random state 
between A) and B) makes little sense - but certain subsets (say an 
'overview' of major signal events) might make sense from an analysis 
POV.

But the thing is, by my reading of these patches we are pretty close to 
B) right now and the tracepoints still look sane - so we might as well 
implement your suggestions and achieve B)? That's a well-defined target 
to achieve. It would mean we need events of sigmask manipulations as 
well, and handler setting events. Plus the missing events you pointed 
out. (plus other stuff i might have forgotten about)

	Ingo

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Masami Hiramatsu]

Oleg Nesterov wrote:
> On 11/23, Ingo Molnar wrote:
>>
>> * Masami Hiramatsu<mhiramat@redhat.com>  wrote:
>>
>>> Hi,
>>>
>>> These patches add signal related tracepoints including
>>> signal generation, delivery, and loss. First patch also
>>> moves signal-sending tracepoint from events/sched.h to
>>> events/signal.h.
>>>
>>> Changes in v3
>>> - Add Docbook style comments
>>>
>>> Changes in v2
>>> - Add siginfo arguments
>>>
>>> Thank you,
>>>
>>> ---
>>>
>>> Masami Hiramatsu (3):
>>>        tracepoint: Add signal loss events
>>>        tracepoint: Add signal deliver event
>>>        tracepoint: Move signal sending tracepoint to events/signal.h
>>>
>>>
>>>   Documentation/DocBook/tracepoint.tmpl |    5 +
>>>   include/trace/events/sched.h          |   25 -----
>>>   include/trace/events/signal.h         |  173 +++++++++++++++++++++++++++++++++
>>>   kernel/signal.c                       |   27 ++++-
>>>   4 files changed, 198 insertions(+), 32 deletions(-)
>>>   create mode 100644 include/trace/events/signal.h
>>
>> Would be nice to have Roland's and Oleg's Acked-by tags in the patches -
>> to show that this is a representative and useful looking set of signal
>> events.
>
> Sorry, I can't really comment these patches.
>
> I mean, I do not know which info is useful and which is not.
> For example, I am a bit surprized we report trace_signal_lose_info()
> but please do not consider this as if I think we shouldn't. Just I
> do not know.
>
> OTOH, we do not report if __send_signal() fails just because the
> legacy signal is already queued. We do not report who sends the signal,
> we do not report if it was private or shared. zap_process, complete_signal
> can "send" SIGKILL via sigaddset, this won't be noticed. But again, it is
> not that I think this should be reported.
>
> In short: I think any info may be useful, and these patches can help.
> But I do not understand what exactly should be reported to userspace.

Yeah, any comments are welcome:-) IMHO, these tracepoints are just for
providing options for users who care about who sent the signal, etc.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH -tip v3 0/3] tracepoint: Add signal events

[Oleg Nesterov]

On 11/24, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Sorry, I can't really comment these patches.
> >
> > I mean, I do not know which info is useful and which is not. For
> > example, I am a bit surprized we report trace_signal_lose_info() but
> > please do not consider this as if I think we shouldn't. Just I do not
> > know.
>
> well, there we lose information, so it's basically an exception/anomaly
> that a person doing analysis might be interested in.
>
> > OTOH, we do not report if __send_signal() fails just because the
> > legacy signal is already queued. [...]
>
> We could do that (beyond the queued signals full event), but i think
> it's rather common to see signal overlap in the legacy case, right?

Yes, right. My point was, we do not know if people want to know
about "lost" signal in this case. Perhaps some application forgot
to unblock the signal, or the sender shouldn't send it, or the
reciever didn't react to the previous one.

But once again, I do not argue. I think the patches are nice and
useful. All I wanted to say is: I trust Masami and I have no idea
whether we need more or less info, and which events are more
"interesting".

> > [...] We do not report who sends the signal, [...]
>
> The PID of any task generating an event can be sampled, so that's
> implicit.

Yes, I missed this. If current != sender (timers, SIGIO) one can
look at entry->code = si_code.

> The principe is this: there's two extremes:
>
>  A- report no event
>
>  B- report every event precisely, that allows all signal state and
>     actions to be reconstructed in hindsight.
>
> And there's a continuum between the two extremes. Just a random state
> between A) and B) makes little sense - but certain subsets (say an
> 'overview' of major signal events) might make sense from an analysis
> POV.
>
> But the thing is, by my reading of these patches we are pretty close to
> B) right now and the tracepoints still look sane - so we might as well
> implement your suggestions and achieve B)? That's a well-defined target
> to achieve. It would mean we need events of sigmask manipulations as
> well, and handler setting events. Plus the missing events you pointed
> out. (plus other stuff i might have forgotten about)

Fortunately, Roland has already replied:

	> If we
	> need to change them, that will become clear from the experiences of people
	> actually using these.

In fact, the above is very close to what I meant but failed to explain ;)

Oleg.