bug fix mmap read in large PEBS

 arch/x86/events/core.c     | 14 ++++++++++++++
 arch/x86/events/intel/ds.c |  8 +++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

arch/x86/events/core.c +14 -0

index 35552ea..f74e21d 100644
@@ -100,6 +100,20 @@ u64 x86_perf_event_update(struct perf_event *event,
 	 * of the count.
 	 */
 	delta = (new_raw_count << shift) - (prev_raw_count << shift);
+
+	/*
+	 * Take auto-reload into account
+	 * For the auto-reload before the last time, it went through the
+	 * whole period (reload_val) every time.
+	 * Just simply add period * times to the event.
+	 *
+	 * For the last load, the elapsed delta (event-)time need to be
+	 * corrected by adding the period. Because the start point is -period.
+	 */
+	if (reload_times > 0) {
+		delta += (reload_val << shift);
+		local64_add(reload_val * (reload_times - 1), &event->count);
+	}
 	delta >>= shift;
 
 	local64_add(delta, &event->count);

arch/x86/events/intel/ds.c +1 -0

index 0b693b7..f0f6026 100644
@@ -1256,11 +1256,17 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 				   void *base, void *top,
 				   int bit, int count)
 {
+	struct hw_perf_event *hwc = &event->hw;
 	struct perf_sample_data data;
 	struct pt_regs regs;
 	void *at = get_next_pebs_record_by_bit(base, top, bit);
 

Quoted 42 lines · On Mon, Dec 18, 2017 at 03:34:49AM -0800, [email protected] wrote:

  arch/x86/events/core.c     | 14 ++++++++++++++
  arch/x86/events/intel/ds.c |  8 +++++++-
  2 files changed, 21 insertions(+), 1 deletion(-)

arch/x86/events/core.c +14 -0

index 35552ea..f74e21d 100644
@@ -100,6 +100,20 @@ u64 x86_perf_event_update(struct perf_event *event,
  	 * of the count.
  	 */
  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
+
+	/*
+	 * Take auto-reload into account
+	 * For the auto-reload before the last time, it went through the
+	 * whole period (reload_val) every time.
+	 * Just simply add period * times to the event.
+	 *
+	 * For the last load, the elapsed delta (event-)time need to be
+	 * corrected by adding the period. Because the start point is -period.
+	 */
+	if (reload_times > 0) {
+		delta += (reload_val << shift);
+		local64_add(reload_val * (reload_times - 1), &event->count);
+	}
  	delta >>= shift;
 
  	local64_add(delta, &event->count);

arch/x86/events/intel/ds.c +1 -0

index 0b693b7..f0f6026 100644
@@ -1256,11 +1256,17 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
  				   void *base, void *top,
  				   int bit, int count)
  {
+	struct hw_perf_event *hwc = &event->hw;
  	struct perf_sample_data data;
  	struct pt_regs regs;
  	void *at = get_next_pebs_record_by_bit(base, top, bit);
 

arch/x86/events/intel/ds.c static void __intel_pmu_pebs_event(struct perf_event *event, · line 1272

Quoted 10 lines

arch/x86/events/intel/ds.c +6 -1

-	if (!intel_pmu_save_and_restart(event, 0, 0) &&
+	/*
+	 * Now, auto-reload is only enabled in fixed period mode.
+	 * The reload value is always hwc->sample_period.
+	 * May need to change it, if auto-reload is enabled in freq mode later.
+	 */
+	if (!intel_pmu_save_and_restart(event, hwc->sample_period, count - 1) &&
  	    !(event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD))
  		return;
 

This all looks very wrong... In auto reload we should never call intel_pmu_save_and_restore() in the first place I think.

Things like x86_perf_event_update() and x86_perf_event_set_period() simply _cannot_ do the right thing when we auto reload the counter.

Quoted 58 lines · Peter Zijlstra

Quoted 47 lines · On Mon, Dec 18, 2017 at 03:34:49AM -0800, [email protected] wrote:

  arch/x86/events/core.c     | 14 ++++++++++++++
  arch/x86/events/intel/ds.c |  8 +++++++-

  2 files changed, 21 insertions(+), 1 deletion(-)

arch/x86/events/core.c +14 -1

index 35552ea..f74e21d 100644
@@ -100,6 +100,20 @@ u64 x86_perf_event_update(struct perf_event *event,
        * of the count.
        */
       delta = (new_raw_count << shift) - (prev_raw_count << shift);
+
+    /*
+     * Take auto-reload into account
+     * For the auto-reload before the last time, it went through the
+     * whole period (reload_val) every time.
+     * Just simply add period * times to the event.
+     *
+     * For the last load, the elapsed delta (event-)time need to be
+     * corrected by adding the period. Because the start point is 
-period.
+     */
+    if (reload_times > 0) {
+        delta += (reload_val << shift);
+        local64_add(reload_val * (reload_times - 1), &event->count);
+    }
       delta >>= shift;
       local64_add(delta, &event->count);

arch/x86/events/intel/ds.c +5 -1

index 0b693b7..f0f6026 100644
@@ -1256,11 +1256,17 @@ static void __intel_pmu_pebs_event(struct 
 perf_event *event,
                      void *base, void *top,
                      int bit, int count)
   {
+    struct hw_perf_event *hwc = &event->hw;
       struct perf_sample_data data;
       struct pt_regs regs;
       void *at = get_next_pebs_record_by_bit(base, top, bit);
-    if (!intel_pmu_save_and_restart(event, 0, 0) &&
+    /*
+     * Now, auto-reload is only enabled in fixed period mode.
+     * The reload value is always hwc->sample_period.
+     * May need to change it, if auto-reload is enabled in freq mode 

Quoted 6 lines

later.
+     */
+    if (!intel_pmu_save_and_restart(event, hwc->sample_period, count
- 1) &&
          !(event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)) return;

This all looks very wrong... In auto reload we should never call intel_pmu_save_and_restore() in the first place I think.

Quoted 3 lines

Things like x86_perf_event_update() and x86_perf_event_set_period() simply _cannot_ do the right thing when we auto reload the counter.

I think it should be OK to call it in first place.
For x86_perf_event_update(), the reload_times will tell if it's auto reload. Both period_left and event->count are carefully recalculated for auto reload.

For x86_perf_event_set_period(), there is nothing special needed for auto reload. The period is fixed. The period_left from x86_perf_event_update() is already handled.

BTW: It should be 'count' not 'count - 1' which pass to intel_pmu_save_and_restart(). I just found the issue. I will fix it in V2 with other improvements if there are any.

Thanks,
Kan

Quoted 6 lines

This all looks very wrong... In auto reload we should never call intel_pmu_save_and_restore() in the first place I think.

Things like x86_perf_event_update() and x86_perf_event_set_period() simply _cannot_ do the right thing when we auto reload the counter.

I think it should be OK to call it in first place.
For x86_perf_event_update(), the reload_times will tell if it's auto reload. Both period_left and event->count are carefully recalculated for auto reload.

For x86_perf_event_set_period(), there is nothing special needed for auto reload. The period is fixed. The period_left from x86_perf_event_update() is already handled.

Quoted 7 lines · Kan

Quoted 6 lines

This all looks very wrong... In auto reload we should never call intel_pmu_save_and_restore() in the first place I think.

Things like x86_perf_event_update() and x86_perf_event_set_period() simply _cannot_ do the right thing when we auto reload the counter.

Quoted 4 lines

I think it should be OK to call it in first place.
For x86_perf_event_update(), the reload_times will tell if it's auto reload. Both period_left and event->count are carefully recalculated for auto reload.

How does prev_count make sense when we've already reloaded a bunch of times?

Quoted 7 lines · Kan

Quoted 6 lines

This all looks very wrong... In auto reload we should never call intel_pmu_save_and_restore() in the first place I think.

Things like x86_perf_event_update() and x86_perf_event_set_period() simply _cannot_ do the right thing when we auto reload the counter.

Quoted 4 lines

I think it should be OK to call it in first place.
For x86_perf_event_update(), the reload_times will tell if it's auto reload. Both period_left and event->count are carefully recalculated for auto reload.

How does prev_count make sense when we've already reloaded a bunch of times?

Quoted 4 lines

For x86_perf_event_set_period(), there is nothing special needed for auto reload. The period is fixed. The period_left from x86_perf_event_update() is already handled.

Hurm.. I see. But rather than make an ever bigger trainwreck of things, I'd rather you just write a special purpose intel_pmu_save_and_restart() just for AUTO_RELOAD.

arch/x86/events/intel/ds.c +0 -0

@@ -926,6 +926,16 @@ void intel_pmu_pebs_del(struct perf_event *event)
 	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
 }

Quoted 6 lines · On Mon, Dec 18, 2017 at 03:34:51AM -0800, [email protected] wrote:

arch/x86/events/intel/ds.c +0 -0

@@ -926,6 +926,16 @@ void intel_pmu_pebs_del(struct perf_event *event)
  	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
  }
 

arch/x86/events/intel/ds.c void intel_pmu_pebs_del(struct perf_event *event) · line 937

Quoted 9 lines

arch/x86/events/intel/ds.c +9 -0

+void intel_pmu_pebs_read(struct perf_event *event)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+
+	if (pebs_needs_sched_cb(cpuc))
+		return intel_pmu_drain_pebs_buffer();
+
+	x86_perf_event_update(event, 0, 0);
+}

This is completely broken.. what if @event isn't a pebs event, but we do have an auto-reloading pebs event configured?