linux-kernel - Re: [PATCH v2 1/1] perf: Sharing PMU counters across compatible events

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]

Message-ID: <90BE3370-3006-426A-80DC-279F07192E70@fb.com>
Date:   Tue, 11 Sep 2018 13:29:32 +0000
From:   Song Liu <songliubraving@...com>
To:     Jiri Olsa <jolsa@...hat.com>
CC:     lkml <linux-kernel@...r.kernel.org>,
        kbuild test robot <lkp@...el.com>,
        Kernel Team <Kernel-team@...com>, Tejun Heo <tj@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>
Subject: Re: [PATCH v2 1/1] perf: Sharing PMU counters across compatible
 events



> On Sep 10, 2018, at 1:13 AM, Jiri Olsa <jolsa@...hat.com> wrote:
> 
> On Thu, Aug 30, 2018 at 06:35:37PM +0000, Song Liu wrote:
>> 
>> 
>>> On Aug 30, 2018, at 8:13 AM, Jiri Olsa <jolsa@...hat.com> wrote:
>>> 
>>> On Wed, Aug 15, 2018 at 10:03:13AM -0700, Song Liu wrote:
>>> 
>>> SNIP
>>> 
>>>> 
>>>> +	perf_event_remove_dup(event, ctx);
>>>> 	/*
>>>> 	 * We can have double detach due to exit/hot-unplug + close.
>>>> 	 */
>>>> @@ -1982,6 +2123,92 @@ event_filter_match(struct perf_event *event)
>>>> 	       perf_cgroup_match(event) && pmu_filter_match(event);
>>>> }
>>>> 
>>>> +/* PMU sharing aware version of event->pmu->add() */
>>>> +static int event_pmu_add(struct perf_event *event,
>>>> +			 struct perf_event_context *ctx)
>>>> +{
>>>> +	struct perf_event_dup *dup;
>>>> +	int ret;
>>>> +
>>>> +	/* no sharing, just do event->pmu->add() */
>>>> +	if (event->dup_id == -1)
>>>> +		return event->pmu->add(event, PERF_EF_START);
>>>> +
>>>> +	dup = &ctx->dup_events[event->dup_id];
>>>> +
>>>> +	if (dup->active_event_count) {
>>>> +		/* already enabled */
>>>> +		dup->active_event_count++;
>>>> +		dup->master->pmu->read(dup->master);
>>>> +		event->dup_base_count = dup_read_count(dup);
>>>> +		event->dup_base_child_count = dup_read_child_count(dup);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	/* try add master */
>>>> +	ret = event->pmu->add(dup->master, PERF_EF_START);
>>>> +
>>>> +	if (!ret) {
>>>> +		dup->active_event_count = 1;
>>>> +		event->pmu->read(dup->master);
>>>> +		event->dup_base_count = dup_read_count(dup);
>>>> +		event->dup_base_child_count = dup_read_child_count(dup);
>>> 
>>> should you read the base before calling pmu->add ?
>>> should be same for any dup event not just master
>>> 
>>> jirka
>> 
>> I am not sure I am following. The pmu is disabled when we call
>> event_pmu_add(). Why do we need to read before calling pmu->add()? 
>> And this is the first added dup event for this master, so we don't
>> need to worry about others. 
>> 
>> Does this make sense? 
> 
> I was just thinking since the pmu is disable we could
> we don't need to read the event on 2 places.. it's almost
> identic code

How about something like:


+/* PMU sharing aware version of event->pmu->add() */
+static int event_pmu_add(struct perf_event *event,
+			 struct perf_event_context *ctx)
+{
+	struct perf_event_dup *dup;
+	int ret;
+
+	/* no sharing, just do event->pmu->add() */
+	if (event->dup_id == -1)
+		return event->pmu->add(event, PERF_EF_START);
+
+	dup = &ctx->dup_events[event->dup_id];
+
+	if (dup->active_event_count = 0) {
+		/* try add master */
+		ret = event->pmu->add(dup->master, PERF_EF_START);
+		if (ret)
+			return ret;
+	}
+
+	dup->active_event_count++;
+	event->pmu->read(dup->master);
+	event->dup_base_count = dup_read_count(dup);
+	event->dup_base_child_count = dup_read_child_count(dup);
+	
+	return 0;
+}