Re: [PATCH 1/3] perf: add PERF_RECORD_NAMESPACES to include namespaces related info

[Hari Bathini]

On Thursday 10 November 2016 06:49 PM, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 05:08:06PM +0530, Hari Bathini wrote:
>
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index c66a485..575aed6 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/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 */
> > @@ -862,6 +863,24 @@ enum perf_event_type {
> >   	 */
> >   	PERF_RECORD_SWITCH_CPU_WIDE		= 15,
> >   
> > +	/*
> > +	 * struct {
> > +	 *	struct perf_event_header	header;
> > +	 *
> > +	 *	u32				pid, tid;
> > +	 *	u64				time;
> > +	 *	u32				uts_ns_inum;
> > +	 *	u32				ipc_ns_inum;
> > +	 *	u32				mnt_ns_inum;
> > +	 *	u32				pid_ns_inum;
> > +	 *	u32				net_ns_inum;
> > +	 *	u32				cgroup_ns_inum;
> > +	 *	u32				user_ns_inum;
> > +	 * 	struct sample_id		sample_id;
> > +	 * };
> > +	 */
> > +	PERF_RECORD_NAMESPACES			= 16,
> So this format is not extensible, that is, if someone adds yet another
> namespace, we'll need to introduce PERF_RECORD_NAMESPACES2.
>
> Is there a 'natural' and exposed namespace index that we can use to
> change it like:
>
> 	u32	nr_nss;
> 	u32	namespace[nr_nss];
>
> ?

Hi Peter,

Nothing of that sort exists, currently.
Maybe, time to introduce with this patch-set..?

> > +static void perf_event_namespaces_output(struct perf_event *event,
> > +					 void *data)
> > +{
> > +	struct perf_namespaces_event *namespaces_event = data;
> > +	struct perf_output_handle handle;
> > +	struct perf_sample_data sample;
> > +	int size = namespaces_event->event_id.header.size;
> > +	struct nsproxy *nsproxy;
> > +	int ret;
> > +
> > +	if (!perf_event_namespaces_match(event))
> > +		return;
> > +
> > +	perf_event_header__init_id(&namespaces_event->event_id.header,
> > +				   &sample, event);
> > +	ret = perf_output_begin(&handle, event,
> > +				namespaces_event->event_id.header.size);
> > +
> > +	if (ret)
> > +		goto out;
> If you were to introduce:
>
> 	struct ns_event_id *ei = &namespace_event->event_id;
>
> > +
> > +	namespaces_event->event_id.pid = perf_event_pid(event,
> > +							namespaces_event->task);
> > +	namespaces_event->event_id.tid = perf_event_tid(event,
> > +							namespaces_event->task);
> > +
> > +	if (namespaces_event->task != current)
> > +		task_lock(namespaces_event->task);
> > +
> > +	nsproxy = namespaces_event->task->nsproxy;
> > +	if (nsproxy != NULL) {
> > +		namespaces_event->event_id.uts_ns_inum =
> > +					nsproxy->uts_ns->ns.inum;
> > +#ifdef CONFIG_IPC_NS
> > +		namespaces_event->event_id.ipc_ns_inum =
> > +					nsproxy->ipc_ns->ns.inum;
> > +#endif
> > +		namespaces_event->event_id.mnt_ns_inum =
> > +					nsproxy->mnt_ns->ns.inum;
> > +		namespaces_event->event_id.pid_ns_inum =
> > +					nsproxy->pid_ns_for_children->ns.inum;
> > +#ifdef CONFIG_NET
> > +		namespaces_event->event_id.net_ns_inum =
> > +					nsproxy->net_ns->ns.inum;
> > +#endif
> > +#ifdef CONFIG_CGROUPS
> > +		namespaces_event->event_id.cgroup_ns_inum =
> > +					nsproxy->cgroup_ns->ns.inum;
> > +#endif
> > +	}
> > +
> > +	namespaces_event->event_id.user_ns_inum =
> > +			__task_cred(namespaces_event->task)->user_ns->ns.inum;
> You can do s/namespace_event->event_id./ei->/ which is tons shorter and
> would result in less wrapping of lines and generally improve
> readability.

True.

> > +
> > +	if (namespaces_event->task != current)
> > +		task_unlock(namespaces_event->task);
> > +
> > +	namespaces_event->event_id.time = perf_event_clock(event);
> > +
> > +	perf_output_put(&handle, namespaces_event->event_id);
> > +
> > +	perf_event__output_id_sample(event, &handle, &sample);
> > +
> > +	perf_output_end(&handle);
> > +out:
> > +	namespaces_event->event_id.header.size = size;
> > +}
> > +
> > +void perf_event_namespaces(struct task_struct *task)
> > +{
> > +	struct perf_namespaces_event namespaces_event;
> > +
> > +	if (!atomic_read(&nr_namespaces_events))
> > +		return;
> > +
> > +	namespaces_event = (struct perf_namespaces_event){
> > +		.task	= task,
> > +		.event_id  = {
> > +			.header = {
> > +				.type = PERF_RECORD_NAMESPACES,
> > +				.misc = 0,
> > +				.size = sizeof(namespaces_event.event_id),
> > +			},
> > +			/* .pid */
> > +			/* .tid */
> > +			/* .time */
> > +			/* .uts_ns_inum */
> > +			/* .ipc_ns_inum */
> > +			/* .mnt_ns_inum */
> > +			/* .pid_ns_inum */
> > +			/* .net_ns_inum */
> > +			/* .cgroup_ns_inum */
> > +			/* .user_ns_inum */
> > +		},
> > +	};
> > +
> > +	perf_iterate_sb(perf_event_namespaces_output,
> > +			&namespaces_event,
> > +			NULL);
> > +}
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 997ac1d..3faca3d 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1818,6 +1818,7 @@ static __latent_entropy struct task_struct *copy_process(
> >   	cgroup_post_fork(p);
> >   	threadgroup_change_end(current);
> >   	perf_event_fork(p);
> > +	perf_event_namespaces(p);
> I would much prefer calling perf_event_namespace() from
> perf_event_fork() and reduce the external interface.

OK. Will update..

Thanks
Hari