Re: [RFC PATCH 2/3 v3] perf: Implement Nehalem uncore pmu

[Lin Ming <ming.m.lin@intel.com>]

Thanks for your great comments.

Let me read it carefully, and then reply back.

Lin Ming

On Fri, 2010-12-10 at 07:46 +0800, Stephane Eranian wrote:
> Hi,
> 
> So I have tested this patch a bit on WSM and as I expected there
> are issues with sampling.
> 
> When HT is on, both siblings CPUs get the interrupt. The HW does not
> allow you to only point interrupts to a single HT thread (CPU).
> 
> I did verify that indeed both threads get the interrupt and that you have a
> race condition. Both sibling CPUs stop uncore, get the status. They may get
> the same overflow status. Both will pass the uncore->active_mask because
> it's shared among siblings cores. Thus,  you have a race for the whole
> interrupt handler execution.
> 
> You need some serialization in there. But the patch does not address this.
> The problem is different from the back-to-back interrupt issue that
> Don worked on.
> The per-cpu marked/handled trick cannot work to avoid this problem.
> 
> You cannot simply say "the lowest indexed" CPU of a sibling pair
> handles the interrupt
> because you don't know if this in an uncore intr, core interrupt or
> something else. You
> need to check. That means each HT thread needs to check uncore
> ovfl_status. IF the
> status is zero, then return. Otherwise, you need to do a 2nd level
> check before you can
> execute the handler. You need to know if the sibling CPU has already
> "consumed" that
> interrupt.
> 
> I think you need some sort of generation counter per physical core and
> per HT thread.
> On interrupt, you could do something along the line of:
>       if (mycpu->intr_count == mysibling->intr_count) {
>           then mycpu->intr_count++
>           execute intr_handler()
>       } else {
>           mycpu->intr_count++
>           return;
>       }
> Of course, the above needs some atomicity and ad locking (but I don't
> think you can
> use locks in NMI context).
> 
> This makes me wonder if vectoring uncore to NMI is really needed,
> given you cannot
> correlated to an IP, incl. a kernel IP. If we were to vector to a
> dedicated (lower prio)
> vector, then we could use the trick of saying the lowest indexed CPU in a pair
> handles the interrupt (because we would already know this is an uncore
> interrupt).
> This would be much simpler. Price: not samples in kernel's critical
> section. But those
> are useless anyway with uncore events.
> 
> - uncore_get_status().
>   PERF_GLOBAL_STATUS contains more than overflow
>   status, bit 61-63 need to be masked off.
> 
> - uncore_pmu_cpu_prepare()
>   I don't understand the x86_max_cores < 2 test. If you run your
>   NHM/WSM is single core mode, you still have uncore to deal with
>   thus, you need cpuc->intel_uncore initialized, don't you?
> 
> - I assume that the reason the uncore->refcnt management is not
>   using atomic ops because the whole CPU hotplug is serialized, right?