linux-kernel - Re: [PATCH v8] 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]

Date:   Thu, 12 Dec 2019 14:49:51 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Song Liu <songliubraving@...com>
Cc:     linux-kernel@...r.kernel.org, kernel-team@...com,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v8] perf: Sharing PMU counters across compatible events

On Fri, Dec 06, 2019 at 04:24:47PM -0800, Song Liu wrote:

> @@ -750,6 +752,16 @@ struct perf_event {
>  	void *security;
>  #endif
>  	struct list_head		sb_list;
> +
> +	/* for PMU sharing */
> +	struct perf_event		*dup_master;
> +	/* check event_sync_dup_count() for the use of dup_base_* */
> +	u64				dup_base_count;
> +	u64				dup_base_child_count;
> +	/* when this event is master,  read from master*count */
> +	local64_t			master_count;
> +	atomic64_t			master_child_count;
> +	int				dup_active_count;
>  #endif /* CONFIG_PERF_EVENTS */
>  };

> +/* 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 *master;
> +	int ret;
> +
> +	/* no sharing, just do event->pmu->add() */
> +	if (!event->dup_master)
> +		return event->pmu->add(event, PERF_EF_START);

Possibly we should look at the location of perf_event::dup_master to be
in a hot cacheline. Because I'm thinking you just added a guaranteed
miss here.

> +
> +	master = event->dup_master;
> +
> +	if (!master->dup_active_count) {
> +		ret = event->pmu->add(master, PERF_EF_START);
> +		if (ret)
> +			return ret;
> +
> +		if (master != event)
> +			perf_event_set_state(master, PERF_EVENT_STATE_ENABLED);
> +	}
> +
> +	master->dup_active_count++;
> +	master->pmu->read(master);
> +	event->dup_base_count = local64_read(&master->count);
> +	event->dup_base_child_count = atomic64_read(&master->child_count);
> +	return 0;
> +}

> +/* PMU sharing aware version of event->pmu->del() */
> +static void event_pmu_del(struct perf_event *event,
> +			  struct perf_event_context *ctx)
> +{
> +	struct perf_event *master;
> +
> +	if (event->dup_master == NULL) {
> +		event->pmu->del(event, 0);
> +		return;
> +	}

How about you write it exactly like the add version:

	if (!event->dup_master)
		return event->pmu->del(event, 0);

?

> +
> +	master = event->dup_master;
> +	event_sync_dup_count(event, master);
> +	if (--master->dup_active_count == 0) {
> +		event->pmu->del(master, 0);
> +		perf_event_set_state(master, PERF_EVENT_STATE_INACTIVE);
> +	} else if (master == event) {
> +		perf_event_set_state(master, PERF_EVENT_STATE_ENABLED);
> +	}
> +}
> +
> +/* PMU sharing aware version of event->pmu->read() */
> +static void event_pmu_read(struct perf_event *event)
> +{
> +	if (event->dup_master == NULL) {
> +		event->pmu->read(event);
> +		return;
> +	}

And here too.

> +	event_sync_dup_count(event, event->dup_master);
> +}