perfmon: X86 generic code (x86)

+ * set cannot be NULL. Context is locked. Interrupts are masked.
+ *
+ * Caller has already restored all PMD and PMC registers, if
+ * necessary (i.e., lazy restore scheme).
+ *
+ * On x86, the only common code just needs to unsecure RDPMC if necessary

+ */
+static inline void pfm_arch_unload_context(struct pfm_context *ctx)

arch/x86/include/asm/perfmon_kern.h line 379

Quoted 8 lines

+ * x86 does not need extra alignment requirements for the sampling buffer
+ */

arch/x86/include/asm/perfmon_kern.h new file +5 -0

+#define PFM_ARCH_SMPL_ALIGN_SIZE	0
+
+asmlinkage void  pmu_interrupt(void);
+
+static inline void pfm_arch_bv_copy(u64 *a, u64 *b, int nbits)

All these bitmap wrappers just seem like unnecessary obfuscation. Could you just drop them and call the standard functions directly?

Hi Andi,

Quoted 11 lines · On Wed, 26 Nov 2008 12:33:09 +0100 Andi Kleen <[email protected]> wrote:

arch/x86/include/asm/perfmon_kern.h line 379

Quoted 8 lines

+ * x86 does not need extra alignment requirements for the sampling buffer
+ */

arch/x86/include/asm/perfmon_kern.h new file +5 -0

+#define PFM_ARCH_SMPL_ALIGN_SIZE	0
+
+asmlinkage void  pmu_interrupt(void);
+
+static inline void pfm_arch_bv_copy(u64 *a, u64 *b, int nbits)

All these bitmap wrappers just seem like unnecessary obfuscation. Could you just drop them and call the standard functions directly?

These were added after comments from the PowerPC maintainer since how the bitmaps are accessed needs to be arch specific.

Quoted 14 lines · Stephen Rothwell

Quoted 11 lines · On Wed, 26 Nov 2008 12:33:09 +0100 Andi Kleen <[email protected]> wrote:

arch/x86/include/asm/perfmon_kern.h line 379

Quoted 8 lines

+ * x86 does not need extra alignment requirements for the sampling buffer
+ */

arch/x86/include/asm/perfmon_kern.h new file +5 -0

+#define PFM_ARCH_SMPL_ALIGN_SIZE	0
+
+asmlinkage void  pmu_interrupt(void);
+
+static inline void pfm_arch_bv_copy(u64 *a, u64 *b, int nbits)

All these bitmap wrappers just seem like unnecessary obfuscation. Could you just drop them and call the standard functions directly?

Quoted 2 lines

These were added after comments from the PowerPC maintainer since how the bitmaps are accessed needs to be arch specific.

Why? They should not be exported outside, so endian shouldn't matter, should it?

Hi Andi,

Quoted 11 lines · On Wed, 26 Nov 2008 12:33:09 +0100 Andi Kleen <[email protected]> wrote:

arch/x86/include/asm/perfmon_kern.h line 379

Quoted 8 lines

+ * x86 does not need extra alignment requirements for the sampling buffer
+ */

arch/x86/include/asm/perfmon_kern.h new file +5 -0

+#define PFM_ARCH_SMPL_ALIGN_SIZE	0
+
+asmlinkage void  pmu_interrupt(void);
+
+static inline void pfm_arch_bv_copy(u64 *a, u64 *b, int nbits)

All these bitmap wrappers just seem like unnecessary obfuscation. Could you just drop them and call the standard functions directly?

These were added after comments from the PowerPC maintainer since how the bitmaps are accessed needs to be arch specific.

Quoted 15 lines · Stephen Rothwell

Hi Andi,

Quoted 11 lines · On Wed, 26 Nov 2008 12:33:09 +0100 Andi Kleen <[email protected]> wrote:

arch/x86/include/asm/perfmon_kern.h line 379

Quoted 8 lines

+ * x86 does not need extra alignment requirements for the sampling buffer
+ */

arch/x86/include/asm/perfmon_kern.h new file +5 -0

+#define PFM_ARCH_SMPL_ALIGN_SIZE 0
+
+asmlinkage void  pmu_interrupt(void);
+
+static inline void pfm_arch_bv_copy(u64 *a, u64 *b, int nbits)

All these bitmap wrappers just seem like unnecessary obfuscation. Could you just drop them and call the standard functions directly?

Quoted 2 lines

These were added after comments from the PowerPC maintainer since how the bitmaps are accessed needs to be arch specific.

Just because perfmon uses u64 for bitfields instead of unsigned long, therefor it breaks on BE32 machines.

I have not yet found a good reason why it needs to use u64 instead of using what's there already.

Quoted 3 lines

I have not yet found a good reason why it needs to use u64 instead of using what's there already.

There is a good reason why we cannot use unsigned long. We must make sure all data structures exchanged between user mode and the kernel have fixed size. This way, we can have a 32-bit tool run unmodified on top of a 64-bit kernel AND we do not need trampoline code to marshall/unmarshall the parameters.

And yes, the abstraction for bitmask ops was introduced because of issues casting u64 -> unsigned long on Big-Endian32-bit machines such as PPC32.

Stephane,

Quoted 7 lines · stephane eranian

Quoted 3 lines

I have not yet found a good reason why it needs to use u64 instead of using what's there already.

There is a good reason why we cannot use unsigned long. We must make sure all data structures exchanged between user mode and the kernel have fixed size. This way, we can have a 32-bit tool run unmodified on top of a 64-bit kernel AND we do not need trampoline code to marshall/unmarshall the parameters.

That's not a good reason at all. We have in kernel interfaces and kernel-userspace interfaces. Making them the same is nice if it works, but horrible if it imposes crappy hackery like the bitops wrappers.

Quoted 2 lines

And yes, the abstraction for bitmask ops was introduced because of issues casting u64 -> unsigned long on Big-Endian32-bit machines such as PPC32.

Sorry, I think it is simply stupid.

You can keep the userspace interface u64 and use unsigned long for the bitmasks in the kernel and take care of it in the user space interface code and do the BE32 conversion when you copy stuff from and to user.

That's a single well defined place and does not add extra crappola over the kernel especially not into hot pathes like the interrupt.

Why do you want to do u64 -> u32 BE32 magic on every interrupt, context switch etc., if you can do it once in the userspace interface ?

Quoted 3 lines

Why do you want to do u64 -> u32 BE32 magic on every interrupt, context switch etc., if you can do it once in the userspace interface ?

I think we agree as to why the user visible structures have to have fixed size.

Some structures have bitfields in them (no in the current patchset yet). Here is an example:

#define PFM_PMD_BV (256/sizeof(__u64))

struct pfarg_pmd_attr {

        __u16 reg_num;          /* which register */
        __u16 reg_set;          /* which event set */
        __u32 reg_flags;        /* REGFL flags */
        __u64 reg_value;        /* 64-bit value */
        __u64 reg_long_reset;   /* write: value to reload after notification */
        __u64 reg_short_reset;  /* write: reset after counter overflow */
        __u64 reg_random_mask;  /* write: bitmask used to limit random value */

__u64 reg_smpl_pmds[PFM_PMD_BV]; /* write: record in sample */ __u64 reg_reset_pmds[PFM_PMD_BV]; /* write: reset on overflow */
__u64 reg_ovfl_swcnt; /* write: # of overflows before switch */
__u64 reg_smpl_eventid; /* write: opaque event identifier */
__u64 reg_last_value; /* read : return: PMD last reset value */
__u64 reg_reserved[8]; /* for future use */
};

So you are advocating keeping that layout for the user level code, i.e., the user level perfmon.h.
But then, in the kernel, you'd have a different version of the same structure with the same name
and the size but with the bitmask defined as unsigned long instead. All internal only bitmask
would also be unsigned long. So the structure would like as follows:

#define PFM_PMD_BV (256/sizeof(unsigned long))
struct pfarg_pmd_attr {

        __u16 reg_num;          /* which register */
        __u16 reg_set;          /* which event set */
        __u32 reg_flags;        /* REGFL flags */
        __u64 reg_value;        /* 64-bit value */
        __u64 reg_long_reset;   /* write: value to reload after notification */
        __u64 reg_short_reset;  /* write: reset after counter overflow */
        __u64 reg_random_mask;  /* write: bitmask used to limit random value */

unsigned long reg_smpl_pmds[PFM_PMD_BV]; /* write: record in sample */ unsigned long reg_reset_pmds[PFM_PMD_BV]; /* write: reset on overflow */
__u64 reg_ovfl_swcnt; /* write: # of overflows before switch */
__u64 reg_smpl_eventid; /* write: opaque event identifier */
__u64 reg_last_value; /* read : return: PMD last reset value */
__u64 reg_reserved[8]; /* for future use */
};

Then we could not directly export include/linux/perfmon.h to user via Kbuild.

Stephane,

Quoted 18 lines · stephane eranian

Quoted 3 lines

Why do you want to do u64 -> u32 BE32 magic on every interrupt, context switch etc., if you can do it once in the userspace interface ?

I think we agree as to why the user visible structures have to have fixed size.

Some structures have bitfields in them (no in the current patchset yet). Here is an example:

#define PFM_PMD_BV (256/sizeof(__u64))

struct pfarg_pmd_attr {

        __u16 reg_num;          /* which register */
        __u16 reg_set;          /* which event set */
        __u32 reg_flags;        /* REGFL flags */
        __u64 reg_value;        /* 64-bit value */
        __u64 reg_long_reset;   /* write: value to reload after notification */
        __u64 reg_short_reset;  /* write: reset after counter overflow */
        __u64 reg_random_mask;  /* write: bitmask used to limit random value */

Quoted 7 lines

__u64 reg_smpl_pmds[PFM_PMD_BV]; /* write: record in sample */ __u64 reg_reset_pmds[PFM_PMD_BV]; /* write: reset on overflow */
__u64 reg_ovfl_swcnt; /* write: # of overflows before switch */
__u64 reg_smpl_eventid; /* write: opaque event identifier */
__u64 reg_last_value; /* read : return: PMD last reset value */
__u64 reg_reserved[8]; /* for future use */
};

__attribute__ ((packed)); ???

From: Thomas Gleixner <[email protected]>

Quoted 1 line

__attribute__ ((packed)); ???

Shouldn't be needed and this woule kill performance on RISC (every struct member is accessed with byte sized loads and shifts/masking).

We did this for the pfkey.h structures by accident, and I truly regret it.

This patch adds the X86 generic perfmon code. It is in charge of implementing certain key functionalities required by the generic code such as read/write of the PMU registers, low-level interrupt handling.

Signed-off-by Stephane Eranian <[email protected]>

--

Index: o3/arch/x86/perfmon/Kconfig
===================================================================

+/**
+ * pfm_arch_ctxswin_thread - thread context switch in
+ * @task: task switched in
+ * @ctx: context for the task
+ * @set: active event set
+ *
+ * Called from pfm_ctxsw(). Task is guaranteed to be current.
+ * set cannot be NULL. Context is locked. Interrupts are masked.
+ *

+ * Caller has already restored all PMD and PMC registers, if
+ * necessary (i.e., lazy restore scheme).
+ *
+ * On x86, the only common code just needs to unsecure RDPMC if necessary
+ *
+ * On model-specific features, e.g., PEBS, IBS, are taken care of in the
+ * corresponding PMU description module
+ */

arch/x86/perfmon/Kconfig new file +11 -0

+	num = set->nused_pmds;
+
+	/*
+	 * we can restore only the PMD we use because:
+	 *
+	 * 	- can only read with pfm_read_pmds() the registers
+	 * 	  declared used via pfm_write_pmds()
+	 *
+	 * 	- if cr4.pce=1, only counters are exposed to user. RDPMC
+	 * 	  does not work with other types of PMU registers.Thus, no
+	 * 	  address is ever exposed by counters

arch/x86/perfmon/Kconfig new file +4 -0

+/**
+ * smp_pmu_interrupt - lowest level PMU interrupt handler for X86
+ * @regs: machine state
+ *

arch/x86/perfmon/Kconfig new file +39 -0

+/**
+ * pfm-arch_pmu_release_percpu - clear NMI state for one CPU
+ *
+ */
+static void pfm_arch_pmu_release_percpu(void *data)
+{
+	struct pfm_arch_pmu_info *pmu_info;
+
+	pmu_info = pfm_pmu_conf->pmu_info;
+
+	__get_cpu_var(pfm_using_nmi) = 0;
+	/*
+	 * invoke model specific release routine.
+	 */
+	if (pmu_info->release_pmu_percpu)
+		pmu_info->release_pmu_percpu();
+}
+
+/**
+ * pfm_arch_pmu_release - release PMU resource to system
+ *
+ * called from pfm_pmu_release()
+ * interrupts are not masked
+ *
+ * On x86, we return the PMU registers to the MSR allocator
+ */
+void pfm_arch_pmu_release(void)
+{
+	struct pfm_regmap_desc *d;
+	u16 i, n;
+
+	d = pfm_pmu_conf->pmc_desc;
+	n = pfm_pmu_conf->regs_all.num_pmcs;
+	for (i = 0; n; i++, d++) {
+		if (!pfm_arch_bv_test_bit(i, pfm_pmu_conf->regs_all.pmcs))
+			continue;
+		release_evntsel_nmi(d->hw_addr);
+		n--;
+		PFM_DBG("pmc%u released", i);

arch/x86/perfmon/Kconfig new file +5 -0

+	/* clear NMI variable if used */
+	if (__get_cpu_var(pfm_using_nmi))
+		on_each_cpu(pfm_arch_pmu_release_percpu, NULL , 1);
+}
+

Index: o3/arch/x86/include/asm/perfmon_kern.h

+static inline int pfm_arch_context_create(struct pfm_context *ctx, u32 ctx_flags)

arch/x86/perfmon/Kconfig new file +17 -0

+int  pfm_arch_init(void);
+void pfm_arch_resend_irq(struct pfm_context *ctx);
+
+int  pfm_arch_ctxswout_thread(struct task_struct *task, struct pfm_context *ctx);
+void pfm_arch_ctxswin_thread(struct task_struct *task, struct pfm_context *ctx);
+
+void pfm_arch_restore_pmds(struct pfm_context *ctx, struct pfm_event_set *set);
+int  pfm_arch_pmu_config_init(struct pfm_pmu_config *cfg);
+void pfm_arch_pmu_config_remove(void);
+char *pfm_arch_get_pmu_module_name(void);
+int pfm_arch_pmu_acquire(u64 *unavail_pmcs, u64 *unavail_pmds);
+void pfm_arch_pmu_release(void);
+
+static inline void pfm_arch_serialize(void)
+{}
+
+static inline void pfm_arch_arm_handle_work(struct task_struct *task)

Quoted 8 lines

arch/x86/perfmon/perfmon.c new file +8 -0

+	 * 	  does not work with other types of PMU registers.Thus, no
+	 * 	  address is ever exposed by counters
+	 *
+	 * 	- there is never a dependency between one pmd register and
+	 * 	  another
+	 */
+	for (i = 0; num; i++) {
+		if (likely(pfm_arch_bv_test_bit(i, set->used_pmds))) {

arch/x86/perfmon/perfmon.c line 203

Quoted 4 lines

arch/x86/perfmon/perfmon.c new file +4 -0

+			pfm_write_pmd(ctx, i, set->pmds[i]);
+			num--;
+		}
+	}

This loop construct looks scary. It relies on set->nused_pmds >= bits set in set->used_pmds. I had to look more than once to understand that. It's used all over the code in variations.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 */
+	pfm_arch_resend_irq(ctx);

Do we really need this whole NMI business ?

9 simple wrappers around generic bitops. The only reason you need those is because you use 64bit variables and that does not work on 32bit BE machines.

I do not understand in the first place why you cant use simple unsigned longs for the bitfields, but if this is necessary for whatever non obvious reason, then its not an excuse to make this arch dependent code at all. You need a LE/BE64 and a BE32 version. So you need a generic and a special be32 version. That's not arch specific.

Quoted 8 lines · Thomas Gleixner

Quoted 8 lines

arch/x86/perfmon/perfmon.c new file +8 -0

+	 * 	  does not work with other types of PMU registers.Thus, no
+	 * 	  address is ever exposed by counters
+	 *
+	 * 	- there is never a dependency between one pmd register and
+	 * 	  another
+	 */
+	for (i = 0; num; i++) {
+		if (likely(pfm_arch_bv_test_bit(i, set->used_pmds))) {

Quoted 8 lines

arch/x86/perfmon/perfmon.c line 203

Quoted 4 lines

arch/x86/perfmon/perfmon.c new file +4 -0

+			pfm_write_pmd(ctx, i, set->pmds[i]);
+			num--;
+		}
+	}

This loop construct looks scary. It relies on set->nused_pmds >= bits set in set->used_pmds. I had to look more than once to understand that. It's used all over the code in variations.

FWIW this loop style tripped me up during review too.

Quoted 14 lines · Andi Kleen

Quoted 13 lines · Thomas Gleixner

Quoted 12 lines

arch/x86/perfmon/perfmon.c new file +12 -0

+  *        does not work with other types of PMU registers.Thus, no
+  *        address is ever exposed by counters
+  *
+  *      - there is never a dependency between one pmd register and
+  *        another
+  */
+ for (i = 0; num; i++) {
+         if (likely(pfm_arch_bv_test_bit(i, set->used_pmds))) {
+                 pfm_write_pmd(ctx, i, set->pmds[i]);
+                 num--;
+         }
+ }

Quoted 5 lines

Quoted 3 lines

This loop construct looks scary. It relies on set->nused_pmds >= bits set in set->used_pmds. I had to look more than once to understand that. It's used all over the code in variations.

FWIW this loop style tripped me up during review too.

It's even worse than I thought when looking at it a second time:

All the loops iterate over an array which means in the worst case we do full array_size iterations. In each iteration we check whether the corresponding bit in the bitmask is set or not.

What a nonsense. We have a bitmask already. Why not iterate over the bitmask and be done ?

Quoted 3 lines

What a nonsense. We have a bitmask already. Why not iterate over the bitmask and be done ?

Bitmask can be sparsed. Num represents the number of bits we have to find. The idea is that we don't need to scan the entire bitmask, we stop as soon as we have found all the bits we care about (i.e., all the bits that are set).

Example:

         num = 3
     bitmask=0000000010001001
                                    ^ we will iterate until we are

done with that bit.

Quoted 8 lines · stephane eranian

Quoted 3 lines

What a nonsense. We have a bitmask already. Why not iterate over the bitmask and be done ?

Bitmask can be sparsed. Num represents the number of bits we have to find. The idea is that we don't need to scan the entire bitmask, we stop as soon as we have found all the bits we care about (i.e., all the bits that are set).

Quoted 5 lines

Example:

         num = 3
     bitmask=0000000010001001
                                    ^ we will iterate until we are

done with that bit.

Errm.

#define for_each_bit(bit, addr, size) \
       for ((bit) = find_first_bit((addr), (size)); \
            (bit) < (size); \
            (bit) = find_next_bit((addr), (size), (bit) + 1))

find_first_bit() and find_next_bit() are single instructions on most architectures. "size" is known upfront at setup time of the context/set and can be cached.

This takes exactly 3 iterations, while your method needs 8. And it gets worse with the following example:

Example:
num = 1
bitmask=1000 0000 0000 0000 0000 0000 0000 00000

^ you will iterate until we are done with that bit (32 times)

for_each_bit() will iterate exactly _once_.

Quoted 4 lines · Thomas Gleixner

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 */
+	pfm_arch_resend_irq(ctx);

Do we really need this whole NMI business ?

Without it you cannot profile interrupts off regions well.

Quoted 7 lines · Andi Kleen

Quoted 4 lines · Thomas Gleixner

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 */
+	pfm_arch_resend_irq(ctx);

Do we really need this whole NMI business ?

Without it you cannot profile interrupts off regions well.

Fair enough, but I doubt that this is a real solution.

There is not even an attempt to avoid the obvious wrmrsl races, while there are several comments which explain how expensive wrmrsl is. In the NMI handler we enable the NMI right away. This might cause multiple NMIs for nothing when the NMIs hit between the manipulations of the counters. Not likely but can happen depending on the counter settings.

Sending an self-IPI from NMI simply sucks: For every NMI we get an extra local interrupt and we have an extra of 2 * NR_ACTIVE_COUNTERS accesses to MSRs.

Designing that code to use lockless buffers instead is not really rocket science.

Quoted 18 lines · Thomas Gleixner

Quoted 7 lines · Andi Kleen

Quoted 4 lines · Thomas Gleixner

Review

Quoted 2 lines

(quoted diff) +2 -0

+        */
+       pfm_arch_resend_irq(ctx);

Do we really need this whole NMI business ?

Without it you cannot profile interrupts off regions well.

Fair enough, but I doubt that this is a real solution.

There is not even an attempt to avoid the obvious wrmrsl races, while there are several comments which explain how expensive wrmrsl is. In the NMI handler we enable the NMI right away. This might cause multiple NMIs for nothing when the NMIs hit between the manipulations of the counters. Not likely but can happen depending on the counter settings.

Quoted 3 lines

Sending an self-IPI from NMI simply sucks: For every NMI we get an extra local interrupt and we have an extra of 2 * NR_ACTIVE_COUNTERS accesses to MSRs.

In newer Intel the counters can be reset/rearmed by accessing only a few global control msrs. But it's probably still a problem on other PMUs.

On the other hand it also has PEBS which allows at least some profiling of irq-off regions without using NMIs.

Quoted 1 line

Quoted 2 lines

Designing that code to use lockless buffers instead is not really rocket science.

Lockless buffers are nasty, but it works in oprofile at least.

Taking out NMis in the first version at least seems like a reasonable solution. After all you can still use standard oprofile where they work just fine.

Quoted 6 lines

Lockless buffers are nasty, but it works in oprofile at least.

Taking out NMis in the first version at least seems like a reasonable solution. After all you can still use standard oprofile where they work just fine.

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

The NMI watchdog is now off by default so failing with it enabled is fine.

Longer term having NMI profiling is still a useful feature I think, but of course it needs to be implemented cleanly.

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

From: "stephane eranian" <[email protected]>
Date: Thu, 27 Nov 2008 12:35:54 +0100

Quoted 2 lines

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

There's only one profiling buffer active on a given cpu, so it's pure per-cpu value insertion.

In any event I think that NMI profiling is a must, especially for the kernel. You get total unusable crap otherwise. I just learned this the hard way having gotten an NMI'ish scheme working on sparc64 just the other day.

Quoted 6 lines · David Miller

From: "stephane eranian" <[email protected]>
Date: Thu, 27 Nov 2008 12:35:54 +0100

Quoted 2 lines

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

Quoted 7 lines

There's only one profiling buffer active on a given cpu, so it's pure per-cpu value insertion.

In any event I think that NMI profiling is a must, especially for the kernel. You get total unusable crap otherwise. I just learned this the hard way having gotten an NMI'ish scheme working on sparc64 just the other day.

Not arguing about that, I'm just not agreeing with the implementation.

So for the moment we can go w/o the NMI and implement it cleanly after we got the initial lot in.

Quoted 18 lines · Thomas Gleixner

Quoted 6 lines · David Miller

From: "stephane eranian" <[email protected]>
Date: Thu, 27 Nov 2008 12:35:54 +0100

Quoted 2 lines

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

Quoted 7 lines

There's only one profiling buffer active on a given cpu, so it's pure per-cpu value insertion.

In any event I think that NMI profiling is a must, especially for the kernel. You get total unusable crap otherwise. I just learned this the hard way having gotten an NMI'ish scheme working on sparc64 just the other day.

Not arguing about that, I'm just not agreeing with the implementation.

Quoted 2 lines

So for the moment we can go w/o the NMI and implement it cleanly after we got the initial lot in.

Note once Stephane readds PEBS support (it is currently stripped out) you'll be also able to get somewhat reasonable results at least on modern Intel x86 without NMI profiling. But longer term it is still very useful because PEBS has some drawbacks too

That's true for Intel.
On AMD64, I think you can get at most one sample out of a irq-off region using IBS.

That's true for Intel.
On AMD64, I think you can get at most one sample out of a irq-off region using IBS.

Quoted 3 lines · stephane eranian

That's true for Intel.
On AMD64, I think you can get at most one sample out of a irq-off region using IBS.

Exactly the same amount which you get with the NMI/self_IPI out of an irq-off region.

Quoted 5 lines · Thomas Gleixner

Quoted 3 lines · stephane eranian

That's true for Intel.
On AMD64, I think you can get at most one sample out of a irq-off region using IBS.

Quoted 3 lines

Exactly the same amount which you get with the NMI/self_IPI out of an irq-off region.

Why would you get only one sample per irq-off region if you use NMI? I would think you could get multiple, depending on your sampling period and the size of the region.

Quoted 3 lines · stephane eranian

That's true for Intel.
On AMD64, I think you can get at most one sample out of a irq-off region using IBS.

Exactly the same amount which you get with the NMI/self_IPI out of an irq-off region.

Quoted 7 lines · Andi Kleen

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

The NMI watchdog is now off by default so failing with it enabled is fine.

Yes, but most likely it is on in distro kernels.

Quoted 6 lines · stephane eranian

Quoted 5 lines · Andi Kleen

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

Quoted 4 lines

Quoted 2 lines

The NMI watchdog is now off by default so failing with it enabled is fine.

Yes, but most likely it is on in distro kernels.

So? You can disable it on the fly when there is a perfmon user.

Peter,

Quoted 7 lines · Peter Zijlstra

Quoted 6 lines · stephane eranian

Quoted 5 lines · Andi Kleen

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

The NMI watchdog is now off by default so failing with it enabled is fine.

Yes, but most likely it is on in distro kernels.

So? You can disable it on the fly when there is a perfmon user.

Yes, you can. There is clearly an interface to do this. I think this is the best solution. I know it can work because it experimented with this approach no later than last month. But I ran into a bug which I reported on LKML. I did not provide a patch because I did not fully understand the connection to suspend/resume.

The bug has to do with some obscure suspend/resume sequence in:

void setup_apic_nmi_watchdog(void *unused)

{
        if (__get_cpu_var(wd_enabled))
                return;

        /* cheap hack to support suspend/resume */
        /* if cpu0 is not active neither should the other cpus */
        if (smp_processor_id() != 0 && atomic_read(&nmi_active) <= 0)
                return;

Basically, when you re-enable the NMI watchdog, it is not always re-enabled correctly on all CPUs, it depends on the order if which they process the IPI.

Quoted 7 lines · Andi Kleen

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

The NMI watchdog is now off by default so failing with it enabled is fine.

Yes, but most likely it is on in distro kernels.

You have to handle the case where the NMI fires while you are holding a perfmon lock. What you have in the patch (and the the fully-featured version) is that we get the NMI and we stop the PMU WITHOUT grabbing any perfmon lock, and the we repost the interrupt with the regular vector. We also make sure we grab the RIP at NMI. That is the one we want to see reported in the sampling buffer.

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

Quoted 6 lines · stephane eranian

Quoted 5 lines · Andi Kleen

Quoted 4 lines

The only reason why I have to deal with NMI is not so much to allow for profiling irq-off regions but because I have to share the PMU with the NMI watchdog. Otherwise I'd have to fail or disable the NMI watchdog on the fly.

Quoted 4 lines

Quoted 2 lines

The NMI watchdog is now off by default so failing with it enabled is fine.

Yes, but most likely it is on in distro kernels.

Really? Why?

Old distros of course do it but only because they run old kernels.

Quoted 7 lines

You have to handle the case where the NMI fires while you are holding a perfmon lock. What you have in the patch (and the the fully-featured version) is that we get the NMI and we stop the PMU WITHOUT grabbing any perfmon lock, and the we repost the interrupt with the regular vector. We also make sure we grab the RIP at NMI. That is the one we want to see reported in the sampling buffer.

Quoted 2 lines

I am still wondering how Oprofile handles the case where multiple processes or threads access the same file descriptor.

It uses per CPU buffers (so no races on the writer) and readers can of course use a lock to coordinate between themselves.

What if a threads reprograms the counters while another is reading them?

How is the buffer reset?

Quoted 1 line

What if a threads reprograms the counters while another is reading them?

In the worst case you get an invalid event, which is then discarded. I think. I've never tried to understand it in all details, but at least it seems to work.

Quoted 1 line

How is the buffer reset?

drivers/oprofile/cpu_buffer.c:

/* Resets the cpu buffer to a sane state. */
void cpu_buffer_reset(struct oprofile_cpu_buffer *cpu_buf)

{
       /* reset these to invalid values; the next sample
        * collected will populate the buffer with proper
        * values to initialize the buffer
        */
       cpu_buf->last_is_kernel = -1;
       cpu_buf->last_task = NULL;
}

What about a thread doing this and another one in the middle of read the buffer?

Or what about a thread trying to reset the buffer while you're processing an PMU interrupt on another CPU. I know each buffer is per-CPU, but that does not prevent two threads for trying to operate on it at the same time from different CPUs.

Quoted 1 line

What about a thread doing this and another one in the middle of read the buffer?

I assume that uses locks. Locking between threads is easy.

Quoted 5 lines

Or what about a thread trying to reset the buffer while you're processing an PMU interrupt on another CPU. I know each buffer is per-CPU, but that does not prevent two threads for trying to operate on it at the same time from different CPUs.

That happens per CPU.