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

[Liang, Kan]

On 9/30/2019 11:52 AM, Peter Zijlstra wrote:
> On Mon, Sep 16, 2019 at 06:41:22AM -0700, kan.liang@xxxxxxxxxxxxxxx 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...