Re: [PATCH v3 08/46] perf/x86/intel/cmt: add MONitored Resource (monr) initialization

[Thomas Gleixner]

On Sat, 29 Oct 2016, David Carrillo-Cisneros wrote:

> Events use monrs to monitor CMT events. Events that share their
> monitoring target (same thread or cgroup) share a monr.

That wants a proper explanation of the concept of monr before explaining
what is done with them.

> This patch introduces monrs and adds support for monr creation/destruction.

'This patch': We know already that this is a patch. See SubmittingPatches
 
> An event's associated monr is referenced by event->cmt_monr (introduced
> in previous patch).
> 
> monr->mon_events references the first event that uses that monr and events
> that share monr are appended to first event's cmt_list list head.
> 
> Hold all pkgd->mutex to modify monr->mon_events and event's data.

Please do not tell what the code does, tell why ad explain the concepts.

> Support for CPU and cgroups is added in future patches in this series.

There is cgroups and CPU and whatever stuff already in this one.
 
> More details in code's comments.

Please add proper descriptions what this is about instead of describing
what the code does. A changelog should give a reviewer a proper overview
about the functionality it introduces or changes. Sending a reviewer for
comment hunting is not a replacement for that.

> +static void monr_dealloc(struct monr *monr)
> +{
> +	kfree(monr);
> +}

Again your ordering of functions is interesting. Please group them together
as they are used. If you need one of them at an earlier point for an error
cleanup or such, then please use forward declarations.

> +static struct monr *monr_alloc(void)
> +{
> +	struct monr *monr;
> +
> +	lockdep_assert_held(&cmt_mutex);
> +
> +	monr = kzalloc(sizeof(*monr), GFP_KERNEL);
> +	if (!monr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return monr;
> +}
> +
> +static inline struct monr *monr_from_event(struct perf_event *event)
> +{
> +	return (struct monr *) READ_ONCE(event->hw.cmt_monr);

That type cast is required because READ_ONCE already returns the type of the
thing you read from, right? So type casting makes more sure of that.

This lacks an explanation why you need a read once at all and it would be
more obvious to put the READ_ONCE explicitely into the code where it is
used and required.

> +}
> +
> +static struct monr *monr_remove_event(struct perf_event *event)
> +{
> +	struct monr *monr = monr_from_event(event);

So what is the actual point of the READ_ONCE here?

> +
> +	lockdep_assert_held(&cmt_mutex);
> +	monr_hrchy_assert_held_mutexes();
> +
> +	if (list_empty(&monr->mon_events->hw.cmt_list)) {
> +		monr->mon_events = NULL;
> +		/* remove from cmt_event_monrs */
> +		list_del_init(&monr->entry);
> +	} else {
> +		if (monr->mon_events == event)
> +			monr->mon_events = list_next_entry(event, hw.cmt_list);
> +		list_del_init(&event->hw.cmt_list);
> +	}
> +
> +	WRITE_ONCE(event->hw.cmt_monr, NULL);

And for the WRITE_ONCE here? I'm must be missing something.

event->hw,cmt_monr cannot change here under your feet. So why forcing the
compiler to read it once? The write once is equally pointless. It does not
matter when it is set to NULL. 

READ_ONCE/WRITE_ONCE stand out in the code and makes the reader alert so he
starts to look for something which might change under the feet. Just there
is nothing. Annoying.

> +
> +	return monr;
> +}
> +
> +static int monr_append_event(struct monr *monr, struct perf_event *event)
> +{
> +	lockdep_assert_held(&cmt_mutex);
> +	monr_hrchy_assert_held_mutexes();

Again. I really appreciate defensive programming, but with all these
repetated asserts in all these callchains is that code actually making
progress when you have lockdep enabled?

> +	if (monr->mon_events) {
> +		list_add_tail(&event->hw.cmt_list,
> +			      &monr->mon_events->hw.cmt_list);
> +	} else {
> +		monr->mon_events = event;
> +		list_add_tail(&monr->entry, &cmt_event_monrs);
> +	}
> +
> +	WRITE_ONCE(event->hw.cmt_monr, monr);

See above.

> +
> +	return 0;
> +}
> +
> +static bool is_cgroup_event(struct perf_event *event)
> +{
> +	return false;
> +}
> +
> +static int monr_hrchy_attach_cgroup_event(struct perf_event *event)
> +{
> +	return -EPERM;
> +}
> +
> +static int monr_hrchy_attach_cpu_event(struct perf_event *event)
> +{
> +	return -EPERM;
> +}

So why is all this here despite the changelog claiming otherwise?

> +
> +static int monr_hrchy_attach_task_event(struct perf_event *event)
> +{
> +	struct monr *monr;
> +	int err;
> +
> +	monr = monr_alloc();
> +	if (IS_ERR(monr))
> +		return -ENOMEM;
> +
> +	err = monr_append_event(monr, event);
> +	if (err)
> +		monr_dealloc(monr);
> +	return err;
> +}
> +
> +/* Insert or create monr in appropriate position in hierarchy. */
> +static int monr_hrchy_attach_event(struct perf_event *event)
> +{
> +	int err = 0;

This initialization is pointless.

> +
> +	lockdep_assert_held(&cmt_mutex);
> +	monr_hrchy_acquire_mutexes();
> +
> +	if (!is_cgroup_event(event) &&
> +	    !(event->attach_state & PERF_ATTACH_TASK)) {

So while you have a faible for wrapping things which should be in the code,
here and else where you have these written out checks over and over.

> +		err = monr_hrchy_attach_cpu_event(event);
> +		goto exit;
> +	}
> +	if (is_cgroup_event(event)) {
> +		err = monr_hrchy_attach_cgroup_event(event);
> +		goto exit;
> +	}
> +	err = monr_hrchy_attach_task_event(event);

Can we please avoid these gotos?

	if (is_cpu_event(event))
		err = attach_cpu(event);
	else if (is_cgroup_event(event))
		err = attach_cgroup(event);
	else
		err = attach_task(event);

would be compact and readable code without gotos. Hmm?

> +exit:
> +	monr_hrchy_release_mutexes();
> +
> +	return err;
> +}
> +
> +/**
> + * __match_event() - Determine if @a and @b should share a rmid.

If you use kernel doc comments, then you have to fill out the argument
descriptors as well.

> + */
> +static bool __match_event(struct perf_event *a, struct perf_event *b)

Why underscores? We use underscores when we have something like this:

__bla(...)
{
	do stuff ....
}

bla(...)
{
	lock();
	__bla(...);
	unlock();
}

But not for standalone functions which have no particular counterpart which
does extra work like serialization.

> +{
> +	/* Cgroup/non-task per-cpu and task events don't mix */
> +	if ((a->attach_state & PERF_ATTACH_TASK) !=
> +	    (b->attach_state & PERF_ATTACH_TASK))
> +		return false;

So this would become a simple one liner with a proper wrapper as well

   	if (is_task_event(a) != is_task_event(b))
		return false;

> +
> +#ifdef CONFIG_CGROUP_PERF
> +	if (a->cgrp != b->cgrp)
> +		return false;
> +#endif
> +
> +	/* If not task event, it's a a cgroup or a non-task cpu event. */
> +	if (!(b->attach_state & PERF_ATTACH_TASK))
> +		return true;
> +
> +	/* Events that target same task are placed into the same group. */
> +	if (a->hw.target == b->hw.target)
> +		return true;
> +
> +	/* Are we a inherited event? */
> +	if (b->parent == a)
> +		return true;
> +
> +	return false;
> +}
> +
>  static struct pmu intel_cmt_pmu;

The placement of this in the middle of unrelated code does what? It's
really annoying having to search for stuff at random places.

Please be more careful about placement.

>  
> +/* Try to find a monr with same target, otherwise create new one. */
> +static int mon_group_setup_event(struct perf_event *event)
> +{
> +	struct monr *monr;
> +	int err;
> +
> +	lockdep_assert_held(&cmt_mutex);
> +
> +	list_for_each_entry(monr, &cmt_event_monrs, entry) {
> +		if (!__match_event(monr->mon_events, event))
> +			continue;
> +		monr_hrchy_acquire_mutexes();
> +		err = monr_append_event(monr, event);
> +		monr_hrchy_release_mutexes();
> +		return err;
> +	}
> +	/*
> +	 * Since no match was found, create a new monr and set this
> +	 * event as head of a mon_group. All events in this group
> +	 * will share the monr.
> +	 */
> +	return monr_hrchy_attach_event(event);
> +}
> +
>  static void intel_cmt_event_read(struct perf_event *event)
>  {
>  }
> @@ -68,6 +272,20 @@ static int intel_cmt_event_add(struct perf_event *event, int mode)
>  	return 0;
>  }
>  
> +static void intel_cmt_event_destroy(struct perf_event *event)

So I was looking at the fully applied series and could not find this
function anymore because it was renamed at some random point. Can you
please avoid this for new code? If you rework something existing it might
be necessary, but for new code it's just a pointless exercise.

> +{
> +	struct monr *monr;
> +
> +	mutex_lock(&cmt_mutex);
> +	monr_hrchy_acquire_mutexes();
> +
> +	/* monr is dettached from event. */

Please comment things which are not obvious. I already know that the event
is destroyed and therefore the monr has to be detached. Comments should
explain why and not what. 'What' we can see from the code and if that has
well chosen names, then it's self explanatory. But the 'Why' of something
complex does not spring into the readers brain right away. That's what
comments are for.

> +	monr = monr_remove_event(event);

Now, whats entirely non obvious is the assignement of the return value with
no further check or processing. I know you do that in some follow up patch,
but for now it's just adding a compiler warning.