Re: [PATCH V4 08/14] perf/x86/intel: Support per thread RDPMC TopDown metrics

["Liang, Kan" <kan.liang@linux.intel.com>]

On 9/30/2019 11:52 AM, Peter Zijlstra wrote:
> On Mon, Sep 16, 2019 at 06:41:22AM -0700, kan.liang@linux.intel.com wrote:
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 71f3086a8adc..7ec0f350d2ac 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -2262,6 +2262,11 @@ static int icl_set_topdown_event_period(struct perf_event *event)
>>   		local64_set(&hwc->period_left, 0);
>>   	}
>>   
>> +	if ((hwc->saved_slots) && is_first_topdown_event_in_group(event)) {
>> +		wrmsrl(MSR_CORE_PERF_FIXED_CTR3, hwc->saved_slots);
>> +		wrmsrl(MSR_PERF_METRICS, hwc->saved_metric);
>> +	}
> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 61448c19a132..c125068f2e16 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -133,6 +133,9 @@ struct hw_perf_event {
>>   
>>   			struct hw_perf_event_extra extra_reg;
>>   			struct hw_perf_event_extra branch_reg;
>> +
>> +			u64		saved_slots;
>> +			u64		saved_metric;
>>   		};
>>   		struct { /* software */
>>   			struct hrtimer	hrtimer;
> 
> Normal counters save their counter value in hwc->period_left, why does
> slots need a new word for that?
> 

We have two values which have to be stored. Only period_left is not enough.

> And since using METRIC means non-sampling, why can't we stick that
> saved_metric field in one of the unused sampling fields?
>

Yes, I think we can re-use last_period and period_left for saved_metric 
and saved_slots. I will change it in V5.


@@ -202,17 +199,26 @@ struct hw_perf_event {
  	 */
  	u64				sample_period;

-	/*
-	 * The period we started this sample with.
-	 */
-	u64				last_period;
+	union {
+		struct { /* Sampling */

-	/*
-	 * However much is left of the current period; note that this is
-	 * a full 64bit value and allows for generation of periods longer
-	 * than hardware might allow.
-	 */
-	local64_t			period_left;
+			/*
+			 * The period we started this sample with.
+			 */
+			u64				last_period;
+
+			/*
+			 * However much is left of the current period; note that this is
+			 * a full 64bit value and allows for generation of periods longer
+			 * than hardware might allow.
+			 */
+			local64_t			period_left;
+		};
+		struct { /* Topdown events counting for context switch*/
+			u64				saved_metric;
+			u64				saved_slots;
+		};
+	};


Thanks,
Kan

> ISTR asking this before...
>