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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 25 Feb 2016 15:29:24 +0200
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	kan.liang@...el.com, peterz@...radead.org,
	linux-kernel@...r.kernel.org
Cc:	ak@...ux.intel.com, eranian@...gle.com, vincent.weaver@...ne.edu,
	tglx@...utronix.de, mingo@...nel.org, acme@...hat.com,
	jolsa@...hat.com, ying.huang@...ux.intel.com,
	Kan Liang <kan.liang@...el.com>
Subject: Re: [PATCH 1/1] perf/core: find auxiliary events in running pmus list

kan.liang@...el.com writes:

> From: Kan Liang <kan.liang@...el.com>
>
> 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