Re: [PATCH 3/4] perf, nmi: Move LVT un-masking into irq handlers

[Cyrill Gorcunov]

On 04/25/2011 05:39 PM, Don Zickus wrote:
> On Fri, Apr 22, 2011 at 10:26:36AM +0200, Ingo Molnar wrote:
>>
>> * Don Zickus <dzickus@xxxxxxxxxx> wrote:
>>
>>> --- a/arch/x86/kernel/cpu/perf_event.c
>>> +++ b/arch/x86/kernel/cpu/perf_event.c
>>> @@ -1284,6 +1284,9 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
>>>  
>>>  	cpuc = &__get_cpu_var(cpu_hw_events);
>>>  
>>> +	/* chipsets have their own quirks when to unmask */
>>> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
>>> +
>>
>> What sense does this comment make in this place?
>>
>> Yes, chipsets have their own quirks - but the generic handler is not one of 
>> them. So a more appropriate comment would be to point out why we want to unmask 
>> there - before PMU handling or after it, etc.
> 
> Yup.  I rework that.

I suspect the comment from one of my draft version might fulfit, Don do you have it?
/me looking in sent mbox

+	/*
+	 * We have to unmask LVTPC *before* enabling counters,
+	 * the key moment is that LVTPC is getting masked on
+	 * PMI arrival and if PMU enabled before APIC unmasked
+	 * a new oveflow signal may reach it but not propagated
+	 * as NMI (due to transition timings) and LVTPC eventually
+	 * stick masked forever dropping any further PMI signals.
+	 */

it was for pmu-intel but i think same applies to general pmu handler.

> 
>>
>> Like the P4 quirk is documented a bit better:
>>
>>> +        /*
>>> +	 * P4 quirks:
>>> +	 * - An overflown perfctr will assert its interrupt
>>> +	 *   until the OVF flag in its CCCR is cleared.
>>> +	 * - LVTPC is masked on interrupt and must be
>>> +	 *   unmasked by the LVTPC handler.
>>> +	 */
>>> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
>>
>> (btw., there's whitespace damage above as well.)
>>
>> Furthermore, the P4 comment should *explain* the quirk coherently, not just 
>> list random facts. What happens, why, where, and why do we unmask the LVTPC in 
>> that spot.

  Ingo, actually I don't understand what else could be added here, should we post
the backtrace calls there? Or plain "we reach this point with OVF bit set" is enough?

  Don't get me wrong please but the whole picture of what is happening can be seen only when
all caller sequence is taken into account and once (for some reason) the sequence
get changed the "detailed" comment would simply mess the comment reader so I think
the former comment is detailed enough and what is more important it's "general" enough
so it doesn't depend on when code is called but points a reader on hw details and it's
up to a reader to check "current" calling sequence because kernel code changes too
damn fast ;)

> 
> Yeah, I copied that from the old watchdog code.  I'll revise that too and
> repost.
> 
> Cheers,
> Don


-- 
    Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/