perf: Fix intel shared extra msr allocation

From: "Yan, Zheng" <[email protected]>

intel_shared_reg_get/put_constraints() can be indirectly called by validate_group(). In that case, they should avoid modifying the perf_event date structure because the event can be already in active state. Otherwise the shared extra msr's reference count will be left in inconsistent state.

Signed-off-by Zheng Yan <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_intel.c |   31 +++++++++++++++++++++++--------

 1 files changed, 23 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/perf_event_intel.c +3 -2

index 166546e..10840d0 100644
@@ -1119,11 +1119,21 @@ intel_bts_constraints(struct perf_event *event)
        return NULL;
  }
 
-static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
+static bool intel_try_alt_er(struct perf_event *event, int *idx,
+                            int orig_idx, bool fake_cpuc)
  {
-       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
+       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1) || *idx != orig_idx)
                return false;

arch/x86/kernel/cpu/perf_event_intel.c +5 -2

        if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
                event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
                event->hw.config |= 0x01bb;

@@ -1139,6 +1149,7 @@ static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
        if (event->hw.extra_reg.idx == orig_idx)
                return false;
 
+       *idx = event->hw.extra_reg.idx;
        return true;
  }
 

@@ -1155,16 +1166,18 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
                                   struct hw_perf_event_extra *reg)
  {
        struct event_constraint *c = &emptyconstraint;
+       struct intel_shared_regs *shared_regs = cpuc->shared_regs;
        struct er_account *era;
        unsigned long flags;
        int orig_idx = reg->idx;
+       int idx = orig_idx;
 
-       /* already allocated shared msr */
-       if (reg->alloc)
+       /* shared msr is already allocated and cpuc is not faked */
+       if (reg->alloc && shared_regs->core_id != -1)

               return NULL; /* call x86_get_event_constraint() */

 again:
-       era = &cpuc->shared_regs->regs[reg->idx];
+       era = &shared_regs->regs[idx];
       /*
        * we use spin_lock_irqsave() to avoid lockdep issues when
        * passing a fake cpuc

arch/x86/kernel/cpu/perf_event_intel.c +5 -4

@@ -1181,14 +1194,16 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
                atomic_inc(&era->ref);
 
                /* no need to reallocate during incremental event scheduling */
-               reg->alloc = 1;
+               if (shared_regs->core_id != -1)
+                       reg->alloc = 1;
 
                /*
                 * need to call x86_get_event_constraint()
                 * to check if associated event has constraints
                 */
                c = NULL;
-       } else if (intel_try_alt_er(event, orig_idx)) {
+       } else if (intel_try_alt_er(event, &idx, orig_idx,
+                                   shared_regs->core_id == -1)) {
                raw_spin_unlock_irqrestore(&era->lock, flags);
                goto again;
        }

@@ -1208,7 +1223,7 @@ __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
         * allocated. Also takes care of event which do
         * not use an extra shared reg
         */
-       if (!reg->alloc)
+       if (!reg->alloc || cpuc->shared_regs->core_id == -1)
                return;
 
        era = &cpuc->shared_regs->regs[reg->idx];
--

1.7.7.6

Quoted 8 lines · Zheng

From: "Yan, Zheng" <[email protected]>

intel_shared_reg_get/put_constraints() can be indirectly called by validate_group(). In that case, they should avoid modifying the perf_event date structure because the event can be already in active state. Otherwise the shared extra msr's reference count will be left in inconsistent state.

I understand the problem but I am wondering if you actually saw it in real life. The reason I am asking is because  of the way validate_group() collects the events and how they are added to sibling_list. The new event is added at the tail. Thus it will come last, and will get to __intel_shared_reg_get_constraints() last, thus I am wondering if it can really modify the programming on the existing events.

See more comments inline.

Quoted 20 lines

Signed-off-by Zheng Yan <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_intel.c |   31 +++++++++++++++++++++++--------

 1 files changed, 23 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/perf_event_intel.c +3 -2

index 166546e..10840d0 100644
@@ -1119,11 +1119,21 @@ intel_bts_constraints(struct perf_event *event)
        return NULL;
  }
 
-static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
+static bool intel_try_alt_er(struct perf_event *event, int *idx,
+                            int orig_idx, bool fake_cpuc)
  {
-       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
+       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1) || *idx != orig_idx)
                return false;

arch/x86/kernel/cpu/perf_event_intel.c intel_bts_constraints(struct perf_event *event) · line 1136

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +9 -0

 
+       /* don't modify the event structure if the cpuc is faked */
+       if (fake_cpuc) {
+               if (*idx == EXTRA_REG_RSP_0)
+                       *idx = EXTRA_REG_RSP_1;
+               else if (*idx == EXTRA_REG_RSP_1)
+                       *idx = EXTRA_REG_RSP_0;
+               return (*idx != orig_idx);
+       }
+

I understand that.

Quoted 21 lines

arch/x86/kernel/cpu/perf_event_intel.c +3 -0

        if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
                event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
                event->hw.config |= 0x01bb;

@@ -1139,6 +1149,7 @@ static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
        if (event->hw.extra_reg.idx == orig_idx)
                return false;
 
+       *idx = event->hw.extra_reg.idx;
        return true;
  }
 

@@ -1155,16 +1166,18 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
                                   struct hw_perf_event_extra *reg)
  {
        struct event_constraint *c = &emptyconstraint;
+       struct intel_shared_regs *shared_regs = cpuc->shared_regs;
        struct er_account *era;
        unsigned long flags;
        int orig_idx = reg->idx;
+       int idx = orig_idx;
 

Quoted 6 lines

arch/x86/kernel/cpu/perf_event_intel.c +2 -2

-       /* already allocated shared msr */
-       if (reg->alloc)
+       /* shared msr is already allocated and cpuc is not faked */
+       if (reg->alloc && shared_regs->core_id != -1)

               return NULL; /* call x86_get_event_constraint() */

I don't understand what you need this stuff. Shared_regs is faked as well.

Quoted 9 lines · Stephane Eranian

Quoted 8 lines · Zheng

From: "Yan, Zheng" <[email protected]>

intel_shared_reg_get/put_constraints() can be indirectly called by validate_group(). In that case, they should avoid modifying the perf_event date structure because the event can be already in active state. Otherwise the shared extra msr's reference count will be left in inconsistent state.

Quoted 7 lines

I understand the problem but I am wondering if you actually saw it in real life. The reason I am asking is because  of the way validate_group() collects the events and how they are added to sibling_list. The new event is added at the tail. Thus it will come last, and will get to __intel_shared_reg_get_constraints() last, thus I am wondering if it can really modify the programming on the existing events.

The real problem is from __intel_shared_reg_put_constraints(). it set reg->alloc to 0 and decreases fake_cpuc->shared_regs->regs[reg->idx]'s reference count. Later when deleting the event,  put_constraints() will find reg->alloc is 0 and it won't decrease the shared msr's reference count.

Run 'perf stat --group -a -C 0 -e LLC-loads -e LLC-stores sleep 1" on Nehalem can trigger the bug.

Quoted 56 lines

See more comments inline.

Quoted 20 lines

Signed-off-by Zheng Yan <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_intel.c |   31 +++++++++++++++++++++++--------

 1 files changed, 23 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/perf_event_intel.c +3 -2

index 166546e..10840d0 100644
@@ -1119,11 +1119,21 @@ intel_bts_constraints(struct perf_event *event)
        return NULL;
  }
 
-static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
+static bool intel_try_alt_er(struct perf_event *event, int *idx,
+                            int orig_idx, bool fake_cpuc)
  {
-       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
+       if (!(x86_pmu.er_flags & ERF_HAS_RSP_1) || *idx != orig_idx)
                return false;

arch/x86/kernel/cpu/perf_event_intel.c intel_bts_constraints(struct perf_event *event) · line 1136

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +9 -0

 
+       /* don't modify the event structure if the cpuc is faked */
+       if (fake_cpuc) {
+               if (*idx == EXTRA_REG_RSP_0)
+                       *idx = EXTRA_REG_RSP_1;
+               else if (*idx == EXTRA_REG_RSP_1)
+                       *idx = EXTRA_REG_RSP_0;
+               return (*idx != orig_idx);
+       }
+

I understand that.

Quoted 21 lines

arch/x86/kernel/cpu/perf_event_intel.c +3 -0

        if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
                event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
                event->hw.config |= 0x01bb;

@@ -1139,6 +1149,7 @@ static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
        if (event->hw.extra_reg.idx == orig_idx)
                return false;
 
+       *idx = event->hw.extra_reg.idx;
        return true;
  }
 

@@ -1155,16 +1166,18 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
                                   struct hw_perf_event_extra *reg)
  {
        struct event_constraint *c = &emptyconstraint;
+       struct intel_shared_regs *shared_regs = cpuc->shared_regs;
        struct er_account *era;
        unsigned long flags;
        int orig_idx = reg->idx;
+       int idx = orig_idx;
 

Quoted 7 lines

Quoted 6 lines

arch/x86/kernel/cpu/perf_event_intel.c +2 -2

-       /* already allocated shared msr */
-       if (reg->alloc)
+       /* shared msr is already allocated and cpuc is not faked */
+       if (reg->alloc && shared_regs->core_id != -1)

               return NULL; /* call x86_get_event_constraint() */

I don't understand what you need this stuff. Shared_regs is faked as well.

The event can be in active state, we should avoid clearing reg->alloc.

Regards
Yan, Zheng

Quoted 23 lines · Zheng

Quoted 9 lines · Stephane Eranian

Quoted 8 lines · Zheng

From: "Yan, Zheng" <[email protected]>

intel_shared_reg_get/put_constraints() can be indirectly called by validate_group(). In that case, they should avoid modifying the perf_event date structure because the event can be already in active state. Otherwise the shared extra msr's reference count will be left in inconsistent state.

Quoted 7 lines

I understand the problem but I am wondering if you actually saw it in real life. The reason I am asking is because of the way validate_group() collects the events and how they are added to sibling_list. The new event is added at the tail. Thus it will come last, and will get to __intel_shared_reg_get_constraints() last, thus I am wondering if it can really modify the programming on the existing events.

The real problem is from __intel_shared_reg_put_constraints(). it set reg->alloc to 0 and decreases fake_cpuc->shared_regs->regs[reg->idx]'s reference count. Later when deleting the event, put_constraints() will find reg->alloc is 0 and it won't decrease the shared msr's reference count.

Quoted 3 lines

Run 'perf stat --group -a -C 0 -e LLC-loads -e LLC-stores sleep 1" on Nehalem can trigger the bug.

And what do you see in this particular example?

I'd like to see the results via libpfm4 and /perf_examples/syst_count:

$ sudo ./syst_count -e offcore_response_0:dmnd_data_rd
-eoffcore_response_0:dmnd_rfo  -p -d 10 -c 0

The real problem is from __intel_shared_reg_put_constraints(). it set reg->alloc to 0

and decreases fake_cpuc->shared_regs->regs[reg->idx]'s reference count.

Later when deleting the event, put_constraints() will find reg->alloc is 0 and it won't decrease the shared msr's reference count.

Quoted 3 lines · Zheng

The real problem is from __intel_shared_reg_put_constraints(). it set reg->alloc to 0

ok

Quoted 2 lines

and decreases fake_cpuc->shared_regs->regs[reg->idx]'s reference count.

So? That's fake state, who cares what its left in?

Quoted 2 lines

Later when deleting the event,  put_constraints() will find reg->alloc is 0 and it won't decrease the shared msr's reference count.

OK, so the only problem is us setting reg->alloc to 0?

I agree with the first part of the patch in intel_try_alt_er(), we should not touch
the actual event struct. But I am still unclear about the reg->alloc part.

Quoted 3 lines · Stephane Eranian

I agree with the first part of the patch in intel_try_alt_er(), we should not touch
the actual event struct. But I am still unclear about the reg->alloc part.

reg->alloc is part of the actual event.

Thing is, the patch is horridly ugly.. while I agree that changing event state isn't good, special casing all that code isn't good either.

I was looking at cloning the events for validate_group() as well, but so far that's not turning out too pretty either.

How about we add a field or flag to cpuc to tell it's fake, and then in
try_alt_er() and __intel_shared_reg_get_constraints() we avoid touching
live struct (like reg->alloc) if fake==1. I think he was trying to do the same with the core_id == -1 test.

Quoted 6 lines · Stephane Eranian

How about we add a field or flag to cpuc to tell it's fake, and then in
try_alt_er() and __intel_shared_reg_get_constraints() we avoid touching
live struct (like reg->alloc) if fake==1. I think he was trying to do the same with the core_id == -1 test.

We might have to do something like that, however I'm trying to figure out when that reg->alloc test in __intel_shared_reg_get_contraints() is useful.

If it is useful in event scheduling, we cannot just leave it out in validate_group().

Quoted 12 lines · Peter Zijlstra

Quoted 6 lines · Stephane Eranian

How about we add a field or flag to cpuc to tell it's fake, and then in
try_alt_er() and __intel_shared_reg_get_constraints() we avoid touching
live struct (like reg->alloc) if fake==1. I think he was trying to do the same with the core_id == -1 test.

We might have to do something like that, however I'm trying to figure out when that reg->alloc test in __intel_shared_reg_get_contraints() is useful.

Quoted 2 lines

If it is useful in event scheduling, we cannot just leave it out in validate_group().

OK, so x86_schedule_events() can call that multiple times on the same event in case we keep adding events.. it needs the constraints of the previous events as well, and in that case we skip the whole extra_reg muck.

But for validate_group() we only do this once, so it should work. Nasty though.

So I think if you say in:
__intel_shared_reg_put_constraints()

     if (!cpuc->is_fake)
           reg->alloc = 1;

That should do it given that:

__intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
struct hw_perf_event_extra *reg)

{
        struct er_account *era;
        if (!reg->alloc)
                return;

}

But then, if reg->alloc was already set to 1, you will have a problem if you leave
it as is. So I think in __intel_shared_reg_put_constraints(), you still need:

__intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
struct hw_perf_event_extra *reg)

{
       if (!reg->alloc || cpuc->is_fake)
          return;

Or something like that.

arch/x86/kernel/cpu/perf_event_intel.c +24 -1

 again:
-	era = &cpuc->shared_regs->regs[reg->idx];
+	era = &cpuc->shared_regs->regs[idx];
 	/*
 	 * we use spin_lock_irqsave() to avoid lockdep issues when
 	 * passing a fake cpuc

@@ -1173,6 +1179,29 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 
 	if (!atomic_read(&era->ref) || era->config == reg->config) {
 
+		/*
+		 * If its a fake cpuc -- as per validate_{group,event}() we
+		 * shouldn't touch event state and we can avoid doing so
+		 * since both will only call get_event_constraints() once
+		 * on each event, this avoids the need for reg->alloc.
+		 *
+		 * Not doing the ER fixup will only result in era->reg being
+		 * wrong, but since we won't actually try and program hardware
+		 * this isn't a problem either.
+		 */
+		if (!cpuc->is_fake) {
+			if (idx != reg->idx)
+				intel_fixup_er(event, idx);
+
+			/* 
+			 * x86_schedule_events() can call get_event_constraints()
+			 * multiple times on events in the case of incremental
+			 * scheduling(). reg->alloc ensures we only do the ER
+			 * allocation once.
+			 */
+			reg->alloc = 1;
+		}
+
 		/* lock in msr value */
 		era->config = reg->config;
 		era->reg = reg->reg;

Quoted 10 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event_intel.c +1 -1

@@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
       struct event_constraint *c = &emptyconstraint;
       struct er_account *era;
       unsigned long flags;
-     int orig_idx = reg->idx;
+     int idx = reg->idx;
 
       /* already allocated shared msr */
       if (reg->alloc)

              return NULL; /* call x86_get_event_constraint() */

I'm afraid that needs to be:

       /*
        * reg->alloc can be set due to existing state, so for fake cpuc
        * we need to ignore this, otherwise we might fail to allocate
        * proper fake state for this extra reg constraint. Also see
        * the comment below.
        */
       if (reg->alloc && !cpuc->is_fake)
               return NULL; /* call x86_get_event_constraints() */

Quoted 1 line

Quoted 28 lines

 again:
-     era = &cpuc->shared_regs->regs[reg->idx];
+     era = &cpuc->shared_regs->regs[idx];
      /*
       * we use spin_lock_irqsave() to avoid lockdep issues when
       * passing a fake cpuc

arch/x86/kernel/cpu/perf_event_intel.c +18 -0

@@ -1173,6 +1179,29 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 
       if (!atomic_read(&era->ref) || era->config == reg->config) {
 
+             /*
+              * If its a fake cpuc -- as per validate_{group,event}() we
+              * shouldn't touch event state and we can avoid doing so
+              * since both will only call get_event_constraints() once
+              * on each event, this avoids the need for reg->alloc.
+              *
+              * Not doing the ER fixup will only result in era->reg being
+              * wrong, but since we won't actually try and program hardware
+              * this isn't a problem either.
+              */
+             if (!cpuc->is_fake) {
+                     if (idx != reg->idx)
+                             intel_fixup_er(event, idx);
+
+                     /*
+                      * x86_schedule_events() can call get_event_constraints()
+                      * multiple times on events in the case of incremental
+                      * scheduling(). reg->alloc ensures we only do the ER

arch/x86/kernel/cpu/perf_event_intel.c __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, · line 1207

Quoted 8 lines

arch/x86/kernel/cpu/perf_event_intel.c +5 -0

+                      * allocation once.
+                      */
+                     reg->alloc = 1;
+             }
+
               /* lock in msr value */
               era->config = reg->config;
               era->reg = reg->reg;

Quoted 10 lines

(quoted diff) +0 -0

@@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 >  	struct event_constraint *c = &emptyconstraint;
 >  	struct er_account *era;
 >  	unsigned long flags;
 > -	int orig_idx = reg->idx;
 > +	int idx = reg->idx;

arch/x86/kernel/cpu/perf_event_intel.c __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, · line 1170

Quoted 4 lines

arch/x86/kernel/cpu/perf_event_intel.c +0 -0

 	/* already allocated shared msr */
 	if (reg->alloc)
 		return NULL; /* call x86_get_event_constraint() */

I'm afraid that needs to be:

/*
* reg->alloc can be set due to existing state, so for fake cpuc
* we need to ignore this, otherwise we might fail to allocate
* proper fake state for this extra reg constraint. Also see
* the comment below.
*/
if (reg->alloc && !cpuc->is_fake)
return NULL; /* call x86_get_event_constraints() */

Quoted 21 lines · Stephane Eranian

So I think if you say in:
__intel_shared_reg_put_constraints()

     if (!cpuc->is_fake)
           reg->alloc = 1;

That should do it given that:

__intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc, struct hw_perf_event_extra *reg) {
        struct er_account *era;
        if (!reg->alloc)
                return;

}

But then, if reg->alloc was already set to 1, you will have a problem if you leave
it as is. So I think in __intel_shared_reg_put_constraints(), you still need:

Quoted 7 lines

__intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc, struct hw_perf_event_extra *reg) {
       if (!reg->alloc || cpuc->is_fake)
          return;

Or something like that.

Right, and then something slightly less hideous for intel_try_alt_er().

How does something like this look?

---
 arch/x86/kernel/cpu/perf_event_intel.c |   43 +++++++++++++++++++++-----------

 1 file changed, 28 insertions(+), 15 deletions(-)

arch/x86/kernel/cpu/perf_event_intel.c +16 -10

index 166546e..a3b7eb3 100644
@@ -1119,27 +1119,33 @@ intel_bts_constraints(struct perf_event *event)
        return NULL;
  }
 
-static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
+static int intel_alt_er(int idx)
  {
        if (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
-               return false;
+               return idx;
 
-       if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
+       if (idx == EXTRA_REG_RSP_0)
+               return EXTRA_REG_RSP_1;
+
+       if (idx == EXTRA_REG_RSP_1)
+               return EXTRA_REG_RSP_0;
+
+       return idx;
+}
+
+static void intel_fixup_er(struct perf_event *event, int idx)
+{
+       if (idx == EXTRA_REG_RSP_0) {
                event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
                event->hw.config |= 0x01bb;
                event->hw.extra_reg.idx = EXTRA_REG_RSP_1;
                event->hw.extra_reg.reg = MSR_OFFCORE_RSP_1;
-       } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
+       } else if (idx == EXTRA_REG_RSP_1) {
                event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
                event->hw.config |= 0x01b7;
                event->hw.extra_reg.idx = EXTRA_REG_RSP_0;
                event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0;
        }
-
-       if (event->hw.extra_reg.idx == orig_idx)
-               return false;
-
-       return true;
  }
 
  /*

@@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
        struct event_constraint *c = &emptyconstraint;
        struct er_account *era;
        unsigned long flags;
-       int orig_idx = reg->idx;
+       int idx = reg->idx;
 
        /* already allocated shared msr */
        if (reg->alloc)

               return NULL; /* call x86_get_event_constraint() */

 again:
-       era = &cpuc->shared_regs->regs[reg->idx];
+       era = &cpuc->shared_regs->regs[idx];
       /*
        * we use spin_lock_irqsave() to avoid lockdep issues when
        * passing a fake cpuc

arch/x86/kernel/cpu/perf_event_intel.c +11 -4

@@ -1181,16 +1187,23 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
                atomic_inc(&era->ref);
 
                /* no need to reallocate during incremental event scheduling */
-               reg->alloc = 1;
+               if (!cpuc->is_fake) {
+                       if (idx != reg->idx)
+                               intel_fixup_er(event, idx);
+                       reg->alloc = 1;
+               }
 
                /*
                 * need to call x86_get_event_constraint()
                 * to check if associated event has constraints
                 */
                c = NULL;
-       } else if (intel_try_alt_er(event, orig_idx)) {
-               raw_spin_unlock_irqrestore(&era->lock, flags);
-               goto again;
+       } else {
+               idx = intel_alt_er(idx);
+               if (idx != reg->idx) {
+                       raw_spin_unlock_irqrestore(&era->lock, flags);
+                       goto again;
+               }
        }
        raw_spin_unlock_irqrestore(&era->lock, flags);
 

Looks nicer, still missing this little piece + is_fake definition and setting is allocate_fakecpuc())

Quoted 3 lines · Stephane Eranian

Looks nicer, still missing this little piece + is_fake definition and setting is allocate_fakecpuc())

Yeah, just send out something more complete -- although only built tested.

This was just seeing how the intel_try_alt_er() stuff could be done saner.

Will try it on my systems. Though it appears the patch has some ^M all over.

Quoted 2 lines · Stephane Eranian

Will try it on my systems. Though it appears the patch has some ^M all over.

Yeah, stupid evolution can only do quoted-printable these days.. progress they call this :/

I can send you an awk script to unmangle it if you want.. but git can do it too somehow.

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Stephane Eranian

Will try it on my systems. Though it appears the patch has some ^M all over.

Yeah, stupid evolution can only do quoted-printable these days.. progress they call this :/

I can send you an awk script to unmangle it if you want.. but git can do it too somehow.

Will try with git, otherwise can do it manually, patch is small enough. Thanks.

There is something wrong with this patch, I instrumented the code and I can see:

[ 1377.324575] 1. idx=1 reg_idx=1 ref=-1 config=0xff01 era->config=0xff01
                                                   ^^^^^^

The test case on WSM (RSP0, RSP1):

$ perf stat  -a -C13 -e
offcore_response_1:dmnd_data_rd,offcore_response_1:dmnd_data_rd sleep
100 &
$ perf stat -a -C1 -e offcore_response_1:dmnd_rfo sleep 1

I think this happens during scheduling of the events, i.e., during the run and not on initial
programming. That could happen with cgroups, for instance.

There is something wrong with this patch, I instrumented the code and I can see:

[ 1377.324575] 1. idx=1 reg_idx=1 ref=-1 config=0xff01 era->config=0xff01
                                                   ^^^^^^

The test case on WSM (RSP0, RSP1):

$ perf stat  -a -C13 -e
offcore_response_1:dmnd_data_rd,offcore_response_1:dmnd_data_rd sleep
100 &
$ perf stat -a -C1 -e offcore_response_1:dmnd_rfo sleep 1

I think this happens during scheduling of the events, i.e., during the run and not on initial
programming. That could happen with cgroups, for instance.