Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG

[Nilay Vaish <nilayvaish@gmail.com>]

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) {