perf, x86: Add hw_watchdog_set_attr() in a sake of nmi-watchdog on P4

arch/x86/kernel/cpu/perf_event_p4.c +14 -0

+static void p4_hw_watchdog_set_attr(struct perf_event_attr *wd_attr)
+{
+	/*
+	 * Watchdog ticks are special on Netburst, we use
+	 * that named "non-sleeping" ticks as recommended
+	 * by Intel SDM Vol3b.
+	 */
+	WARN_ON_ONCE(wd_attr->type	!= PERF_TYPE_HARDWARE ||
+		     wd_attr->config	!= PERF_COUNT_HW_CPU_CYCLES);
+
+	wd_attr->type	= PERF_TYPE_RAW;
+	wd_attr->config	=
+		p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_EXECUTION_EVENT)		|
+			P4_ESCR_EMASK_BIT(P4_EVENT_EXECUTION_EVENT, NBOGUS0)		|

* Cyrill Gorcunov <[email protected]> wrote:

So why don't we simply set this for all PERF_COUNT_HW_CPU_CYCLES events in the P4 PMU driver? That would remove half of the patch AFAICS.

Thanks,

Ingo

Quoted 2 lines · Ingo Molnar

* Cyrill Gorcunov <[email protected]> wrote:

...

Quoted 8 lines

So why don't we simply set this for all PERF_COUNT_HW_CPU_CYCLES events in the P4 PMU driver? That would remove half of the patch AFAICS.

Thanks,

Ingo

Unfortunately it doesn't solve the main issue -- nmi-watchdog events and cpu clock events should be a separate events with non-intersected ESCRs, otherwise nmi-watchdog and cpu-cycles can't operate simultaneously like other PMUs does.

non-sleeping ticks use

	[P4_EVENT_EXECUTION_EVENT] = {
		.escr_msr	= { MSR_P4_CRU_ESCR2, MSR_P4_CRU_ESCR3 },
		.cntr		= { {12, 13, 16}, {14, 15, 17}
	},

while cpu-cycles

	[P4_EVENT_GLOBAL_POWER_EVENTS] = {
		.escr_msr	= { MSR_P4_FSB_ESCR0, MSR_P4_FSB_ESCR1 },
		.cntr		= { {0, -1, -1}, {2, -1, -1} },
	},

are 'non-sleeping ticks' non-halted cycles - i.e. cycles that always count with CPU frequency and can thus be used for periodic frequencies?

On Tue, Jul 05, 2011 at 12:59:59PM +0200, Ingo Molnar wrote:
...

Quoted 4 lines

are 'non-sleeping ticks' non-halted cycles - i.e. cycles that always count with CPU frequency and can thus be used for periodic frequencies?

Yes, I think so (btw, as far as I remember oprofile does the same 'threshold' gaming for nmi-watchdog).

Quoted 7 lines · Cyrill Gorcunov

On Tue, Jul 05, 2011 at 12:59:59PM +0200, Ingo Molnar wrote:
...

Quoted 4 lines

are 'non-sleeping ticks' non-halted cycles - i.e. cycles that always count with CPU frequency and can thus be used for periodic frequencies?

Quoted 2 lines

Yes, I think so (btw, as far as I remember oprofile does the same 'threshold' gaming for nmi-watchdog).

But the NMI watchdog does not need a constant frequency event - it can use unhalted cycles just fine. Why does it need unhalted cycles on P4?

Quoted 10 lines · Ingo Molnar

Quoted 7 lines · Cyrill Gorcunov

On Tue, Jul 05, 2011 at 12:59:59PM +0200, Ingo Molnar wrote:
...

Quoted 4 lines

are 'non-sleeping ticks' non-halted cycles - i.e. cycles that always count with CPU frequency and can thus be used for periodic frequencies?

Quoted 7 lines

Quoted 2 lines

Yes, I think so (btw, as far as I remember oprofile does the same 'threshold' gaming for nmi-watchdog).

But the NMI watchdog does not need a constant frequency event - it can use unhalted cycles just fine. Why does it need unhalted cycles on P4?

Ingo, the main problem there is not halted or unhalted events but rather inability of p4 architecture to move events between counters, with core or nehalem you can (if constraints allow) simply move nmi-watchdog event to another free counter and assign cpu cycles to a free slot. With p4 the situation is radically different -- every event has a number of contraints and once nmi-watchdog armed (it could be halted or unhaled event, whatever) ESCR/CCCR/counter registers tuple borrowed forever and we need to find out some different non-intersected ESCR/CCCR/counter tuple to count cpu-cycles in a sake of perf utility purpose. That is why this assignments done in a such weird way -- just to be able to run nmi-watchdog and cpu-cycles simultaneously.

Probably I miss something and you mean something completely different?

What i am missing is that you have not pointed out the *core problem* you are fixing and it's not obvious from the changelog either!

On Tue, Jul 05, 2011 at 01:44:37PM +0200, Ingo Molnar wrote:
...

Quoted 3 lines

What i am missing is that you have not pointed out the *core problem* you are fixing and it's not obvious from the changelog either!

OK, seems I put there some tech details while human readable changelog was needed. Will fix that, thanks!

Cyrill

perf, x86: P4 PMU - Add hw_watchdog_set_attr helper to simulate cpu-cycles counting in nmi-watchdog

Because of constraints existed in Netburst PMU counting cpu cycles is allowed for one consumer only.

If the kernel is booted up with nmi-watchdog enabled the watchdog become a consumer of such event and there is no more room left for "perf top" and friends (ie any attempts to count cpu cycles simultaneously with nmi-watchdog doomed to fail).

Quoted 10 lines · Cyrill Gorcunov

perf, x86: P4 PMU - Add hw_watchdog_set_attr helper to simulate cpu-cycles counting in nmi-watchdog

Because of constraints existed in Netburst PMU counting cpu cycles is allowed for one consumer only.

If the kernel is booted up with nmi-watchdog enabled the watchdog become a consumer of such event and there is no more room left for "perf top" and friends (ie any attempts to count cpu cycles simultaneously with nmi-watchdog doomed to fail).

Hm, what is the symptom - 'perf top' reports nothing?

If multiple users request cycles then perf will time-share them - this is what happens if you run many 'perf top' or 'perf stat' sessions in parallel for example. For example i just tried to run six separate 'perf top' in parallel - and all six worked fine.

So the question is, why does the NMI watchdog prevent 'perf top' from working on a P4?

Quoted 2 lines

So the question is, why does the NMI watchdog prevent 'perf top' from working on a P4?

because the NMI watchdog is a pinned event, you don't want to share the counter, that would be very bad, suppose you lock up when the NMI watchdog was scheduled out. Unreliably debug tools are worse than no tools.

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines

So the question is, why does the NMI watchdog prevent 'perf top' from working on a P4?

because the NMI watchdog is a pinned event, you don't want to share the counter, that would be very bad, suppose you lock up when the NMI watchdog was scheduled out. Unreliably debug tools are worse than no tools.

Yeah, indeed that explains the symptom.

Firstly, we should fix/enhance perf top to print out an error message in this case, not just hang there doing nothing.

Secondly, the proper solution would be to allow the multiplexing of like-minded hw events. Here if we have two events:

- pinned NMI watchdog, set to a period of 2 billion cycles
- perf top with a default of 1 khz auto-freq cycles

We should first change the NMI watchdog to use auto-freq samples - the hw_nmi_get_sample_period() looks unnecessary - if we set the NMI watchdog to 1 Hz by default it should be more than enough.

Thus we'd have two events:

- pinned NMI watchdog, set to 1 Hz
- perf top, set to 1000 Hz

The 1000 Hz event could serve the 1 Hz event just fine.

Thirdly, even if we mucked the NMI event to be somewhat different from the real cycles event (so the P4 PMU can co-schedule it with perf top), the real solution there would *still* be to express this as a variation of a cycles event: use the same trick to allow up to two cycles events to be present in the PMU - but hide this from the generic interfaces, just allow up to two cycles events to be scheduled at once.

This will have the advantage of not only fixing the NMI watchdog, but other users as well.

Now, if there's some real behavioral difference between the two events (one is halted cycles the other is unhalted cycles), then i'd suggest to use PERF_COUNT_HW_BUS_CYCLES in the NMI watchdog - that is the generic 'constant frequency' cycles event.

So there's lots of options to fix/improve this more intelligently.

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines

So the question is, why does the NMI watchdog prevent 'perf top' from working on a P4?

because the NMI watchdog is a pinned event, you don't want to share the counter, that would be very bad, suppose you lock up when the NMI watchdog was scheduled out. Unreliably debug tools are worse than no tools.

Quoted 4 lines

Yeah, indeed that explains the symptom.

Firstly, we should fix/enhance perf top to print out an error message in this case, not just hang there doing nothing.

It waits for event to be scheduled, so seems first the kernel should have top level context-schedule-in functions changed from void to int (so I have admit I might be missin something here).

Quoted 7 lines

Secondly, the proper solution would be to allow the multiplexing of like-minded hw events. Here if we have two events:

- pinned NMI watchdog, set to a period of 2 billion cycles
- perf top with a default of 1 khz auto-freq cycles

Quoted 3 lines

We should first change the NMI watchdog to use auto-freq samples - the hw_nmi_get_sample_period() looks unnecessary - if we set the NMI watchdog to 1 Hz by default it should be more than enough.

Should we drop tunability of watchdog period as well? At moment there is a way to setup period via /sys.

Secondly, the proper solution would be to allow the multiplexing of like-minded hw events.

So there's lots of options to fix/improve this more intelligently.

Quoted 1 line · Ingo Molnar

So there's lots of options to fix/improve this more intelligently.

Sure, and I like the alternative encoding thing best, but doing that will take time, in the meantime this patch, which is relatively small (and easy to revert once we get something better going) does fix a problem for those few unfortunate souls still using P4 hardware.

Quoted 3 lines · Peter Zijlstra

Quoted 1 line · Ingo Molnar

So there's lots of options to fix/improve this more intelligently.

Quoted 5 lines

Sure, and I like the alternative encoding thing best, but doing that will take time, in the meantime this patch, which is relatively small (and easy to revert once we get something better going) does fix a problem for those few unfortunate souls still using P4 hardware.

Well, the BUS_CYCLES thing looks similarly straightforward and should result in an even simpler patch.

On P4 BUS_CYCLES would be able to co-exist with CPU_CYCLES so it will solve the P4 issue naturally as well.

Thanks,

Ingo

i don't think it changes much, Ingo, if I change it to bus cycles I still will have to setup nmi-watchdog event separately (but simply with bus event).

so an only option is the aliases, i'll try to deal with it but no milestones

Cyrill

Quoted 2 lines

On P4 BUS_CYCLES would be able to co-exist with CPU_CYCLES so it will solve the P4 issue naturally as well.

i don't think it changes much, Ingo, if I change it to bus cycles I still will have to setup nmi-watchdog event separately (but simply with bus event).

Quoted 6 lines · Cyrill Gorcunov

Quoted 2 lines

On P4 BUS_CYCLES would be able to co-exist with CPU_CYCLES so it will solve the P4 issue naturally as well.

i don't think it changes much, Ingo, if I change it to bus cycles I still will have to setup nmi-watchdog event separately (but simply with bus event).

You did not understand my point, my suggestion is to change this in kernel/watchdog.c:

static struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,

to:

static struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_BUS_CYCLES,

Thanks,

Ingo

Quoted 19 lines · Ingo Molnar

Quoted 6 lines · Cyrill Gorcunov

Quoted 2 lines

On P4 BUS_CYCLES would be able to co-exist with CPU_CYCLES so it will solve the P4 issue naturally as well.

i don't think it changes much, Ingo, if I change it to bus cycles I still will have to setup nmi-watchdog event separately (but simply with bus event).

You did not understand my point, my suggestion is to change this in kernel/watchdog.c:

static struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,

to:

Quoted 7 lines

static struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_BUS_CYCLES,

Thanks,

Ingo

Ah, I see. Ingo, if events alias patch I posted yesterday will do the trick we will be able to simply drop all this hooks. So as only Don (or someone) confirm the patch works I'll kill this mess.

Changing cpu-cycles to bus cycles will allow nmi-watchdog and perf top co-exsist but it means perf bus-cycles won't work anymore with active nmi-watchdog. So I think events aliases (again, if it work :) will be better for us, right?

Changing cpu-cycles to bus cycles will allow nmi-watchdog and perf top co-exsist but it means perf bus-cycles won't work anymore

with active nmi-watchdog. So I think events aliases (again, if it work :) will be better for us, right?

Quoted 5 lines · Cyrill Gorcunov

perf, x86: P4 PMU - Add hw_watchdog_set_attr helper to simulate cpu-cycles counting in nmi-watchdog

Because of constraints existed in Netburst PMU counting cpu cycles is allowed for one consumer only.

Quoted 5 lines

If the kernel is booted up with nmi-watchdog enabled the watchdog become a consumer of such event and there is no more room left for "perf top" and friends (ie any attempts to count cpu cycles simultaneously with nmi-watchdog doomed to fail).

Hm, what is the symptom - 'perf top' reports nothing?

If multiple users request cycles then perf will time-share them - this is what happens if you run many 'perf top' or 'perf stat' sessions in parallel for example. For example i just tried to run six separate 'perf top' in parallel - and all six worked fine.

So the question is, why does the NMI watchdog prevent 'perf top' from working on a P4?

Quoted 1 line

Probably I miss something and you mean something completely different?

What i am missing is that you have not pointed out the *core problem* you are fixing and it's not obvious from the changelog either!

I hope that clears things up Ingo.

Cheers,
Don

On Tue, Jul 05, 2011 at 08:24:05AM -0400, Don Zickus wrote:
...

Quoted 5 lines

I hope that clears things up Ingo.

Cheers,
Don

Don, I've just sent out updated changelog, please check it out, does it look better?

Quoted 8 lines · Cyrill Gorcunov

On Tue, Jul 05, 2011 at 08:24:05AM -0400, Don Zickus wrote:
...

Quoted 5 lines

I hope that clears things up Ingo.

Cheers,
Don

Quoted 2 lines

Don, I've just sent out updated changelog, please check it out, does it look better?

It seems to be, then again I am biased because I know what the patch does (and would like it :-) ).

Cheers,
Don