Re: [PATCH 1/1] perf/core: find auxiliary events in running pmus list

[Alexander Shishkin]

kan.liang@xxxxxxxxx writes:

> From: Kan Liang <kan.liang@xxxxxxxxx>
>
> perf_event_aux funciton goes through pmus list to find proper auxiliary
> events to output. The pmus list consists of all possible pmus in the
> system, that may or may not be running at the moment, while the
> auxiliary events must be from the running pmus. Therefore searching
> non-running pmus is unnecessary and expensive especially when there are
> many non-running pmus on the list.
>
> For example, the brk test case in lkp triggers many mmap operations,
> at the time, perf with cycles:pp is also running on the system. As a
> result, many perf_event_aux are invoked, and each would search the whole
> pmus list. If we enable the uncore support (even when uncore event are
> not really used), dozens of uncore pmus will be added into pmus list,
> which can significantly decrease brk_test's ops_per_sec. Based on our
> test, the ops_per_sec without uncore patch is 2647573, while the
> ops_per_sec with uncore patch is only 1768444, which is a 33.2%
> reduction.

What does this ops_per_sec measure, exactly? Just curious.
You'll probably also observe the same effect if you simply create a
bunch of disabled events before you measure the time that it takes
perf_event_aux() to notify everybody. Even worse, because you can have
way more events than pmus. Question is, is this really a problem.

> This patch introduces a running_pmus list which only tracks the running
> pmus in the system. The perf_event_aux uses running_pmus list instead of
> pmus list to find auxiliary events.

This patch also adds a global mutex that serializes *all* event
creation/freeing. Including the fork and exit paths.

I mean:

> @@ -7740,6 +7770,29 @@ static void account_event_cpu(struct perf_event *event, int cpu)
>  		atomic_inc(&per_cpu(perf_cgroup_events, cpu));
>  }
>  
> +static void account_running_pmu(struct perf_event *event)
> +{
> +	struct running_pmu *pmu;
> +
> +	mutex_lock(&running_pmus_lock);
> +
> +	list_for_each_entry(pmu, &running_pmus, entry) {
> +		if (pmu->pmu == event->pmu) {
> +			pmu->nr_event++;
> +			goto out;
> +		}
> +	}
> +
> +	pmu = kzalloc(sizeof(struct running_pmu), GFP_KERNEL);
> +	if (pmu != NULL) {
> +		pmu->nr_event++;
> +		pmu->pmu = event->pmu;
> +		list_add_rcu(&pmu->entry, &running_pmus);
> +	}
> +out:
> +	mutex_unlock(&running_pmus_lock);
> +}
> +
>  static void account_event(struct perf_event *event)
>  {
>  	bool inc = false;
> @@ -7772,6 +7825,8 @@ static void account_event(struct perf_event *event)
>  		static_key_slow_inc(&perf_sched_events.key);
>  
>  	account_event_cpu(event, event->cpu);
> +
> +	account_running_pmu(event);

doesn't look justified.

Regards,
--
Alex