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.li...@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...