From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752002AbcHGTKe (ORCPT <rfc822;w@1wt.eu>);
	Sun, 7 Aug 2016 15:10:34 -0400
Received: from mail-io0-f195.google.com ([209.85.223.195]:34625 "EHLO
	mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751712AbcHGTKd (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 7 Aug 2016 15:10:33 -0400
Subject: Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
To: David Carrillo-Cisneros <davidcc@google.com>, linux-kernel@vger.kernel.org
References: <1470539577-102094-1-git-send-email-davidcc@google.com>
 <1470539577-102094-4-git-send-email-davidcc@google.com>
Cc: "x86@kernel.org" <x86@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, Andi Kleen <ak@linux.intel.com>,
        Kan Liang <kan.liang@intel.com>, Peter Zijlstra <peterz@infradead.org>,
        Vegard Nossum <vegard.nossum@gmail.com>, Paul Turner <pjt@google.com>,
        Stephane Eranian <eranian@google.com>
From: Nilay Vaish <nilayvaish@gmail.com>
Message-ID: <9b29b625-d019-6671-5676-3c58a31a6d3f@gmail.com>
Date: Sun, 7 Aug 2016 14:10:31 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.2
MIME-Version: 1.0
In-Reply-To: <1470539577-102094-4-git-send-email-davidcc@google.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/06/16 22:12, David Carrillo-Cisneros wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 34049cc..77f1bd3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3333,6 +3333,26 @@ struct perf_read_data {
>  	int ret;
>  };
>
> +static int find_cpu_to_read(struct perf_event *event)
> +{
> +	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
> +	int event_cpu = event->oncpu, local_cpu = smp_processor_id();
> +	u16 local_pkg, event_pkg;
> +
> +	if (!active)
> +		return -1;
> +
> +	if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
> +		event_pkg =  topology_physical_package_id(event_cpu);
> +		local_pkg =  topology_physical_package_id(local_cpu);
> +
> +		if (event_pkg == local_pkg)
> +			return local_cpu;
> +	}
> +
> +	return event_cpu;
> +}
> +
>  /*
>   * Cross CPU call to read the hardware event
>   */
> @@ -3454,19 +3474,17 @@ u64 perf_event_read_local(struct perf_event *event)
>
>  static int perf_event_read(struct perf_event *event, bool group)
>  {
> -	int ret = 0;
> +	int ret = 0, cpu_to_read;
>
> -	/*
> -	 * If event is enabled and currently active on a CPU, update the
> -	 * value in the event structure:
> -	 */
> -	if (event->state == PERF_EVENT_STATE_ACTIVE) {
> +	cpu_to_read = find_cpu_to_read(event);
> +
> +	if (cpu_to_read >= 0) {
>  		struct perf_read_data data = {
>  			.event = event,
>  			.group = group,
>  			.ret = 0,
>  		};
> -		ret = smp_call_function_single(event->oncpu,
> +		ret = smp_call_function_single(cpu_to_read,
>  					       __perf_event_read, &data, 1);
>  		ret = ret ? : data.ret;
>  	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>

I would like to suggest a small change to this patch.  I think the check 
on PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read() 
function.  The new function should assume that the event is active.  I 
find this more readable since the next check in function 
perf_event_read() is on PERF_EVENT_STATE_INACTIVE.


Diff below:

+static int find_cpu_to_read(struct perf_event *event)
+{
+    int event_cpu = event->oncpu, local_cpu = smp_processor_id();
+    u16 local_pkg, event_pkg;
+
+    if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+        event_pkg = topology_physical_package_id(event_cpu);
+        local_pkg = topology_physical_package_id(local_cpu);
+
+        if (event_pkg == local_pkg)
+            return local_cpu;
+    }
+
+    return event_cpu;
+}
+
  /*
   * Cross CPU call to read the hardware event
   */
@@ -3477,17 +3493,15 @@ static int perf_event_read(struct perf_event 
*event, bool group)
  {
         int ret = 0;

-       /*
-        * If event is enabled and currently active on a CPU, update the
-        * value in the event structure:
-        */
-       if (event->state == PERF_EVENT_STATE_ACTIVE) {
+    	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+        	int cpu_to_read = find_cpu_to_read(event);
+
                 struct perf_read_data data = {
                         .event = event,
                         .group = group,
                         .ret = 0,
                 };
-               ret = smp_call_function_single(event->oncpu,
+               ret = smp_call_function_single(cpu_to_read,
                                                __perf_event_read, 
&data, 1);
                 ret = ret ? : data.ret;
         } else if (event->state == PERF_EVENT_STATE_INACTIVE) {