Messages in this thread | | | Subject | Re: [RFC PATCH 2/3 v3] perf: Implement Nehalem uncore pmu | From | Lin Ming <> | Date | Fri, 10 Dec 2010 16:31:02 +0800 |
| |
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?
| |