Re: [PATCH v7 2/8] perf tool: add PERF_RECORD_NAMESPACES to include namespaces related info

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

Hi Arnaldo,


Thanks for the review.


On Thursday 02 March 2017 02:32 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 21, 2017 at 07:31:30PM +0530, Hari Bathini escreveu:
>> Update perf tool to examine PERF_RECORD_NAMESPACES events emitted by
>>> the kernel when fork, clone, setns or unshare are invoked.
> You forgot to update tools/perf/Documentation/ for all the options you
> added, see more comments below.

Will fold the next patch into this one.

>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>> ---
>>   tools/include/uapi/linux/perf_event.h |   32 +++++++++++++++++++++++-
>>   tools/perf/builtin-annotate.c         |    1 +
>>   tools/perf/builtin-diff.c             |    1 +
>>   tools/perf/builtin-inject.c           |   14 +++++++++++
>>   tools/perf/builtin-kmem.c             |    1 +
>>   tools/perf/builtin-kvm.c              |    2 ++
>>   tools/perf/builtin-lock.c             |    1 +
>>   tools/perf/builtin-mem.c              |    1 +
>>   tools/perf/builtin-record.c           |    6 +++++
>>   tools/perf/builtin-report.c           |    1 +
>>   tools/perf/builtin-sched.c            |    1 +
>>   tools/perf/builtin-script.c           |    1 +
>>   tools/perf/builtin-trace.c            |    3 ++
>>   tools/perf/perf.h                     |    1 +
>>   tools/perf/util/Build                 |    1 +
>>   tools/perf/util/data-convert-bt.c     |    1 +
>>   tools/perf/util/event.c               |    9 +++++++
>>   tools/perf/util/event.h               |   14 +++++++++++
>>   tools/perf/util/evsel.c               |    3 ++
>>   tools/perf/util/machine.c             |   31 +++++++++++++++++++++++
>>   tools/perf/util/machine.h             |    3 ++
>>   tools/perf/util/namespaces.c          |   35 ++++++++++++++++++++++++++
>>   tools/perf/util/namespaces.h          |   26 ++++++++++++++++++++
>>   tools/perf/util/session.c             |    7 +++++
>>   tools/perf/util/thread.c              |   44 ++++++++++++++++++++++++++++++++-
>>   tools/perf/util/thread.h              |    6 +++++
>>   tools/perf/util/tool.h                |    2 ++
>>   27 files changed, 244 insertions(+), 4 deletions(-)
>>   create mode 100644 tools/perf/util/namespaces.c
>>   create mode 100644 tools/perf/util/namespaces.h
>>
>> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
>> index c66a485..bec0aad 100644
>> --- a/tools/include/uapi/linux/perf_event.h
>> +++ b/tools/include/uapi/linux/perf_event.h
>> @@ -344,7 +344,8 @@ struct perf_event_attr {
>>   				use_clockid    :  1, /* use @clockid for time fields */
>>   				context_switch :  1, /* context switch data */
>>   				write_backward :  1, /* Write ring buffer from end to beginning */
>> -				__reserved_1   : 36;
>> +				namespaces     :  1, /* include namespaces data */
>> +				__reserved_1   : 35;
>>   
>>   	union {
>>   		__u32		wakeup_events;	  /* wakeup every n events */
>> @@ -610,6 +611,23 @@ struct perf_event_header {
>>   	__u16	size;
>>   };
>>   
>> +struct perf_ns_link_info {
>> +	__u64	dev;
>> +	__u64	ino;
>> +};
>> +
>> +enum {
>> +	NET_NS_INDEX		= 0,
>> +	UTS_NS_INDEX		= 1,
>> +	IPC_NS_INDEX		= 2,
>> +	PID_NS_INDEX		= 3,
>> +	USER_NS_INDEX		= 4,
>> +	MNT_NS_INDEX		= 5,
>> +	CGROUP_NS_INDEX		= 6,
>> +
>> +	NR_NAMESPACES,		/* number of available namespaces */
>> +};
>> +
>>   enum perf_event_type {
>>   
>>   	/*
>> @@ -862,6 +880,18 @@ enum perf_event_type {
>>   	 */
>>   	PERF_RECORD_SWITCH_CPU_WIDE		= 15,
>>   
>> +	/*
>> +	 * struct {
>> +	 *	struct perf_event_header	header;
>> +	 *	u32				pid;
>> +	 *	u32				tid;
>> +	 *	u64				nr_namespaces;
>> +	 *	{ u64				dev, inode; } [nr_namespaces];
>> +	 *	struct sample_id		sample_id;
>> +	 * };
>> +	 */
>> +	PERF_RECORD_NAMESPACES			= 16,
>> +
>>   	PERF_RECORD_MAX,			/* non-ABI */
>>   };
>>   
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index ebb6283..1b63dc4 100644
>> --- a/tools/perf/builtin-annotate.c
>> +++ b/tools/perf/builtin-annotate.c
>> @@ -393,6 +393,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>>   			.comm	= perf_event__process_comm,
>>   			.exit	= perf_event__process_exit,
>>   			.fork	= perf_event__process_fork,
>> +			.namespaces = perf_event__process_namespaces,
>>   			.ordered_events = true,
>>   			.ordering_requires_timestamps = true,
>>   		},
>> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>> index 70a2893..4b821cf 100644
>> --- a/tools/perf/builtin-diff.c
>> +++ b/tools/perf/builtin-diff.c
>> @@ -364,6 +364,7 @@ static struct perf_tool tool = {
>>   	.exit	= perf_event__process_exit,
>>   	.fork	= perf_event__process_fork,
>>   	.lost	= perf_event__process_lost,
>> +	.namespaces = perf_event__process_namespaces,
>>   	.ordered_events = true,
>>   	.ordering_requires_timestamps = true,
>>   };
>> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
>> index b9bc7e3..c5ddc73 100644
>> --- a/tools/perf/builtin-inject.c
>> +++ b/tools/perf/builtin-inject.c
>> @@ -333,6 +333,19 @@ static int perf_event__repipe_comm(struct perf_tool *tool,
>>   	return err;
>>   }
>>   
>> +static int perf_event__repipe_namespaces(struct perf_tool *tool,
>> +					 union perf_event *event,
>> +					 struct perf_sample *sample,
>> +					 struct machine *machine)
>> +{
>> +	int err;
>> +
>> +	err = perf_event__process_namespaces(tool, event, sample, machine);
> Minor, but since changes are needed anyway: combine the previous three
> lines into one.

Sure.

>> +	perf_event__repipe(tool, event, sample, machine);
>> +
>> +	return err;
>> +}
>> +
>>   static int perf_event__repipe_exit(struct perf_tool *tool,
>>   				   union perf_event *event,
>>   				   struct perf_sample *sample,
>> @@ -660,6 +673,7 @@ static int __cmd_inject(struct perf_inject *inject)
>>   		session->itrace_synth_opts = &inject->itrace_synth_opts;
>>   		inject->itrace_synth_opts.inject = true;
>>   		inject->tool.comm	    = perf_event__repipe_comm;
>> +		inject->tool.namespaces	    = perf_event__repipe_namespaces;
>>   		inject->tool.exit	    = perf_event__repipe_exit;
>>   		inject->tool.id_index	    = perf_event__repipe_id_index;
>>   		inject->tool.auxtrace_info  = perf_event__process_auxtrace_info;
>> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
>> index 6da8d08..d509e74 100644
>> --- a/tools/perf/builtin-kmem.c
>> +++ b/tools/perf/builtin-kmem.c
>> @@ -964,6 +964,7 @@ static struct perf_tool perf_kmem = {
>>   	.comm		 = perf_event__process_comm,
>>   	.mmap		 = perf_event__process_mmap,
>>   	.mmap2		 = perf_event__process_mmap2,
>> +	.namespaces	 = perf_event__process_namespaces,
>>   	.ordered_events	 = true,
>>   };
>>   
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index 08fa88f..18e6c38 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -1044,6 +1044,7 @@ static int read_events(struct perf_kvm_stat *kvm)
>>   	struct perf_tool eops = {
>>   		.sample			= process_sample_event,
>>   		.comm			= perf_event__process_comm,
>> +		.namespaces		= perf_event__process_namespaces,
>>   		.ordered_events		= true,
>>   	};
>>   	struct perf_data_file file = {
>> @@ -1348,6 +1349,7 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
>>   	kvm->tool.exit   = perf_event__process_exit;
>>   	kvm->tool.fork   = perf_event__process_fork;
>>   	kvm->tool.lost   = process_lost_event;
>> +	kvm->tool.namespaces  = perf_event__process_namespaces;
>>   	kvm->tool.ordered_events = true;
>>   	perf_tool__fill_defaults(&kvm->tool);
>>   
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index ce3bfb4..d750cca 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -858,6 +858,7 @@ static int __cmd_report(bool display_info)
>>   	struct perf_tool eops = {
>>   		.sample		 = process_sample_event,
>>   		.comm		 = perf_event__process_comm,
>> +		.namespaces	 = perf_event__process_namespaces,
>>   		.ordered_events	 = true,
>>   	};
>>   	struct perf_data_file file = {
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index cd7bc4d..430656c 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -342,6 +342,7 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>>   			.lost		= perf_event__process_lost,
>>   			.fork		= perf_event__process_fork,
>>   			.build_id	= perf_event__process_build_id,
>> +			.namespaces	= perf_event__process_namespaces,
>>   			.ordered_events	= true,
>>   		},
>>   		.input_name		 = "perf.data",
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 6cd6776..a8b9a78 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -876,6 +876,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>>   	signal(SIGTERM, sig_handler);
>>   	signal(SIGSEGV, sigsegv_handler);
>>   
>> +	if (rec->opts.record_namespaces)
>> +		tool->namespace_events = true;
>> +
>>   	if (rec->opts.auxtrace_snapshot_mode || rec->switch_output.enabled) {
>>   		signal(SIGUSR2, snapshot_sig_handler);
>>   		if (rec->opts.auxtrace_snapshot_mode)
>> @@ -1497,6 +1500,7 @@ static struct record record = {
>>   		.fork		= perf_event__process_fork,
>>   		.exit		= perf_event__process_exit,
>>   		.comm		= perf_event__process_comm,
>> +		.namespaces	= perf_event__process_namespaces,
>>   		.mmap		= perf_event__process_mmap,
>>   		.mmap2		= perf_event__process_mmap2,
>>   		.ordered_events	= true,
>> @@ -1611,6 +1615,8 @@ static struct option __record_options[] = {
>>   			  "opts", "AUX area tracing Snapshot Mode", ""),
>>   	OPT_UINTEGER(0, "proc-map-timeout", &record.opts.proc_map_timeout,
>>   			"per thread proc mmap processing timeout in ms"),
>> +	OPT_BOOLEAN(0, "namespaces", &record.opts.record_namespaces,
>> +		    "Record namespaces events"),
>>   	OPT_BOOLEAN(0, "switch-events", &record.opts.record_switch_events,
>>   		    "Record context switch events"),
>>   	OPT_BOOLEAN_FLAG(0, "all-kernel", &record.opts.all_kernel,
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index dbd7fa0..5c92c75 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -694,6 +694,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>>   			.mmap		 = perf_event__process_mmap,
>>   			.mmap2		 = perf_event__process_mmap2,
>>   			.comm		 = perf_event__process_comm,
>> +			.namespaces	 = perf_event__process_namespaces,
>>   			.exit		 = perf_event__process_exit,
>>   			.fork		 = perf_event__process_fork,
>>   			.lost		 = perf_event__process_lost,
>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>> index 270eb2d..e0ddd04 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3272,6 +3272,7 @@ int cmd_sched(int argc, const char **argv, const char *prefix __maybe_unused)
>>   		.tool = {
>>   			.sample		 = perf_sched__process_tracepoint_sample,
>>   			.comm		 = perf_event__process_comm,
>> +			.namespaces	 = perf_event__process_namespaces,
>>   			.lost		 = perf_event__process_lost,
>>   			.fork		 = perf_sched__process_fork_event,
>>   			.ordered_events = true,
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index c0783b4..f1ce806 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -2097,6 +2097,7 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
>>   			.mmap		 = perf_event__process_mmap,
>>   			.mmap2		 = perf_event__process_mmap2,
>>   			.comm		 = perf_event__process_comm,
>> +			.namespaces	 = perf_event__process_namespaces,
>>   			.exit		 = perf_event__process_exit,
>>   			.fork		 = perf_event__process_fork,
>>   			.attr		 = process_attr,
>> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
>> index 40ef9b2..0bcd32f 100644
>> --- a/tools/perf/builtin-trace.c
>> +++ b/tools/perf/builtin-trace.c
>> @@ -2415,8 +2415,9 @@ static int trace__replay(struct trace *trace)
>>   	trace->tool.exit	  = perf_event__process_exit;
>>   	trace->tool.fork	  = perf_event__process_fork;
>>   	trace->tool.attr	  = perf_event__process_attr;
>> -	trace->tool.tracing_data = perf_event__process_tracing_data;
>> +	trace->tool.tracing_data  = perf_event__process_tracing_data;
>>   	trace->tool.build_id	  = perf_event__process_build_id;
>> +	trace->tool.namespaces	  = perf_event__process_namespaces;
>>   
>>   	trace->tool.ordered_events = true;
>>   	trace->tool.ordering_requires_timestamps = true;
>> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
>> index 1c27d94..806c216 100644
>> --- a/tools/perf/perf.h
>> +++ b/tools/perf/perf.h
>> @@ -50,6 +50,7 @@ struct record_opts {
>>   	bool	     running_time;
>>   	bool	     full_auxtrace;
>>   	bool	     auxtrace_snapshot_mode;
>> +	bool	     record_namespaces;
>>   	bool	     record_switch_events;
>>   	bool	     all_kernel;
>>   	bool	     all_user;
>> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
>> index 5da376b..2ea5ee1 100644
>> --- a/tools/perf/util/Build
>> +++ b/tools/perf/util/Build
>> @@ -42,6 +42,7 @@ libperf-y += pstack.o
>>   libperf-y += session.o
>>   libperf-$(CONFIG_AUDIT) += syscalltbl.o
>>   libperf-y += ordered-events.o
>> +libperf-y += namespaces.o
>>   libperf-y += comm.o
>>   libperf-y += thread.o
>>   libperf-y += thread_map.o
>> diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
>> index 4e6cbc9..89ece24 100644
>> --- a/tools/perf/util/data-convert-bt.c
>> +++ b/tools/perf/util/data-convert-bt.c
>> @@ -1468,6 +1468,7 @@ int bt_convert__perf2ctf(const char *input, const char *path,
>>   			.lost            = perf_event__process_lost,
>>   			.tracing_data    = perf_event__process_tracing_data,
>>   			.build_id        = perf_event__process_build_id,
>> +			.namespaces      = perf_event__process_namespaces,
>>   			.ordered_events  = true,
>>   			.ordering_requires_timestamps = true,
>>   		},
>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
>> index 4ea7ce7..f118eac 100644
>> --- a/tools/perf/util/event.c
>> +++ b/tools/perf/util/event.c
>> @@ -31,6 +31,7 @@ static const char *perf_event__names[] = {
>>   	[PERF_RECORD_LOST_SAMPLES]		= "LOST_SAMPLES",
>>   	[PERF_RECORD_SWITCH]			= "SWITCH",
>>   	[PERF_RECORD_SWITCH_CPU_WIDE]		= "SWITCH_CPU_WIDE",
>> +	[PERF_RECORD_NAMESPACES]		= "NAMESPACES",
>>   	[PERF_RECORD_HEADER_ATTR]		= "ATTR",
>>   	[PERF_RECORD_HEADER_EVENT_TYPE]		= "EVENT_TYPE",
>>   	[PERF_RECORD_HEADER_TRACING_DATA]	= "TRACING_DATA",
>> @@ -1016,6 +1017,14 @@ int perf_event__process_comm(struct perf_tool *tool __maybe_unused,
>>   	return machine__process_comm_event(machine, event, sample);
>>   }
>>   
>> +int perf_event__process_namespaces(struct perf_tool *tool __maybe_unused,
>> +				   union perf_event *event,
>> +				   struct perf_sample *sample,
>> +				   struct machine *machine)
>> +{
>> +	return machine__process_namespaces_event(machine, event, sample);
>> +}
>> +
>>   int perf_event__process_lost(struct perf_tool *tool __maybe_unused,
>>   			     union perf_event *event,
>>   			     struct perf_sample *sample,
>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
>> index c735c53..4e90b09 100644
>> --- a/tools/perf/util/event.h
>> +++ b/tools/perf/util/event.h
>> @@ -39,6 +39,15 @@ struct comm_event {
>>   	char comm[16];
>>   };
>>   
>> +#define NAMESPACES_MAX			12
> Why have this limitation, does the kernel has it as well? Just read the

Right. A max limit here seems unnecessary. Will get rid of this.

> header, then allocate enough space, this way you don't need to have
> those checks about tools being incompatible with a kernel that supports
> more namespaces than the tool.
> And then how can this take place, i.e. are you truncating the extra
> namespaces coming from the kernel but continuing anyway, just after
> warning the user once about it?

Currently, NR_NAMESPACES is 7 & NAMESPACES_MAX is 12. I don't think we 
would go
beyond 12 namespaces in the kernel anytime soon. The magic number 12 is 
based
on that assumption.  So, truncating the extra namespaces coming from kernel
wasn't something I was concerned about. I will get rid of NAMESPACES_MAX 
magic
number as you suggested, thus not having to go by the assumption that the
kerenl may never have more than 12 namespaces.

But I think there is still a need to warn the user about different 
NR_NAMESPACES
value in kernel and perf because the warning was aimed at a scenario where
NR_NAMESPACES in the kernel is greater/less than the NR_NAMESPACES value
in perf.

> Wouldn't this message get lost in the logs and the user be left
> wondering why the namespaces it expects to be there to have vanished?

Hmmm.. Probably, while running perf-report or perf-script but not when 
data is
captured with perf-record. And that should be enough for the user to 
take a call
on where to post process or discard the data?

>> +	if (thread == NULL ||
>> +	    thread__set_namespaces(thread, sample->time, &event->namespaces)) {
>> +		dump_printf("problem processing PERF_RECORD_NAMESPACES, skipping event.\n");
>> +		err = -1;
>> +	}
>> +
>> +	thread__put(thread);
>> +
>> +	return err;
>> +}
>> +
>>   int machine__process_lost_event(struct machine *machine __maybe_unused,
>>   				union perf_event *event, struct perf_sample *sample __maybe_unused)
>>   {
>> @@ -1538,6 +1567,8 @@ int machine__process_event(struct machine *machine, union perf_event *event,
>>   		ret = machine__process_comm_event(machine, event, sample); break;
>>   	case PERF_RECORD_MMAP:
>>   		ret = machine__process_mmap_event(machine, event, sample); break;
>> +	case PERF_RECORD_NAMESPACES:
>> +		ret = machine__process_namespaces_event(machine, event, sample); break;
>>   	case PERF_RECORD_MMAP2:
>>   		ret = machine__process_mmap2_event(machine, event, sample); break;
>>   	case PERF_RECORD_FORK:
>> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
>> index a283050..3cdb134 100644
>> --- a/tools/perf/util/machine.h
>> +++ b/tools/perf/util/machine.h
>> @@ -97,6 +97,9 @@ int machine__process_itrace_start_event(struct machine *machine,
>>   					union perf_event *event);
>>   int machine__process_switch_event(struct machine *machine,
>>   				  union perf_event *event);
>> +int machine__process_namespaces_event(struct machine *machine,
>> +				      union perf_event *event,
>> +				      struct perf_sample *sample);
>>   int machine__process_mmap_event(struct machine *machine, union perf_event *event,
>>   				struct perf_sample *sample);
>>   int machine__process_mmap2_event(struct machine *machine, union perf_event *event,
>> diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
>> new file mode 100644
>> index 0000000..3134c00
>> --- /dev/null
>> +++ b/tools/perf/util/namespaces.c
>> @@ -0,0 +1,35 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * Copyright (C) 2017 Hari Bathini, IBM Corporation
>> + */
>> +
>> +#include "namespaces.h"
>> +#include "util.h"
>> +#include "event.h"
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +struct namespaces *namespaces__new(struct namespaces_event *event)
>> +{
>> +	struct namespaces *namespaces = zalloc(sizeof(*namespaces));
>> +
>> +	if (!namespaces)
>> +		return NULL;
>> +
>> +	namespaces->end_time = -1;
>> +
>> +	if (event) {
>> +		memcpy(namespaces->link_info, event->link_info,
>> +		       sizeof(namespaces->link_info));
>> +	}
> Please allocate just what came from the kernel, be it less or more than
> that magic number (12).

>> +struct namespaces_event;
>> +
>> +struct namespaces {
>> +	struct list_head list;
>> +	u64 end_time;
>> +	struct perf_ns_link_info link_info[NR_NAMESPACES];
> Here you could have it as a zero sized array and allocate it according
> to the number of namespaces that came from the kernel

Sure. Will change that.

Thanks
Hari