oprofile fixes for v2.6.40

Ingo,

please pull oprofile fixes for v2.6.40 (tip/perf/urgent):

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

Thanks,

-Robert

The following changes since commit a2d063ac216c1618bfc2b4d40b7176adffa63511:

extable, core_kernel_data(): Make sure all archs define _sdata (2011-05-20 08:56:56 +0200)

are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

Anton Blanchard (1):
oprofile: Use linux/mutex.h

Eric B Munson (1):
oprofile, powerpc: Handle events that raise an exception without overflowing

Robert Richter (1):
oprofile, x86: Enable preemption during pci device setup in IBS init

 arch/powerpc/oprofile/op_model_power4.c |   24 ++++++++-
 arch/x86/oprofile/op_model_amd.c        |   95 +++++++++++++++++-------------
 drivers/oprofile/event_buffer.h         |    2 +-
 drivers/oprofile/oprof.c                |    2 +-
 4 files changed, 79 insertions(+), 44 deletions(-)

Advanced Micro Devices, Inc.
Operating System Research Center

I'm wondering about this bit, as from a preempt_enable()/disable() pattern POV it looks bogus. It does:

        preempt_disable();
        for (offset = 1; offset < APIC_EILVT_NR_MAX; offset++) {
                if (get_eilvt(offset))
                        break;
        }
        preempt_enable();

	...


	ret = setup_ibs_ctl(offset);

So whatever condition it established at the get_eilvt() stage might not be valid at the setup_ibs_ctl() point: we might be on a different CPU.

Or are the IBS MSRs (which get twiddled in get_eilvt() absolutely symmetric across all CPUs, thus making this a preempt invariant thing? In that case we'd at least need a big fat comment in the code as it's not obvious at all ...

But i really do not see how this can work. get_eilvt() twiddles the APIC LVT of this current CPU. Does IBS only activate on a single CPU? How will this LVT entry be cleared on deinit?

Mysteries, mysteries!

Thanks,

Ingo

The MSRC001_103A (IBS Control Register) register contains the LVT offset. It is a per-node msr and a read-only copy of F3x1CC (pci device register). The offset must be valid (bit set) and not yet taken by another vector (kernel keeps track of it in eilvt_offsets). Though offsets must be the same on all nodes, the kernel implementation uses same offsets on all cores of the system.

The BIOS is responsible to setup the vector. If the vector in IBS_CTL is invalid or already taken, we reassign a different offset in force_ibs_eilvt_setup() (family 10h only).

In get_eilvt()/setup_APIC_eilvt() there are reads and writes to the local apic. Preemption must be disabled to not be switched to another cpu in the middle of a sequence.

This is done on the current cpu to get the offset which is then used on all cpus of the system. The actually per-core setup for ibs is in op_amd_setup_ctrs(). Deinit is in op_amd_cpu_shutdown().

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

The way IBS is set up is not very obvious and atypical of existing preempt_*() patters. Might be worth adding a comment to the preempt_disable(), to unconfuse unsuspecting readers who are wondering about its oddness?

Anyway, i suspected that it was correctly working code so i pulled it and pushed it to Linus, it should all be in 3.0-rc1.

Thanks,

Ingo

Yes, will add some comments.

Have seen it, thanks Ingo,

-Robert

Advanced Micro Devices, Inc.
Operating System Research Center

Ingo,

please see below and apply the patch to tip, if it makes sense to you.

Thanks,

-Robert

From: Robert Richter <[email protected]>
Date: Mon, 30 May 2011 16:31:11 +0200
Subject: [PATCH] oprofile, x86: Add comments to IBS LVT offset initialization

Adding a comment in the code as IBS LVT setup is not obvious at all ...

---
 arch/x86/kernel/apic/apic.c      |    3 ++-
 arch/x86/oprofile/op_model_amd.c |   13 +++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

index fabf01e..a0bf78a 100644
@@ -390,7 +390,8 @@ static unsigned int reserve_eilvt_offset(int offset, unsigned int new)
 
 /*
  * If mask=1, the LVT entry does not generate interrupts while mask=0
- * enables the vector. See also the BKDGs.
+ * enables the vector. See also the BKDGs. Must be called with
+ * preemption disabled.
  */
 
 int setup_APIC_eilvt(u8 offset, u8 vector, u8 msg_type, u8 mask)

index 9fd8a56..9cbb710 100644
@@ -609,16 +609,21 @@ static int setup_ibs_ctl(int ibs_eilvt_off)
 	return 0;
 }
 
+/*
+ * This runs only on the current cpu. We try to find an LVT offset and
+ * setup the local APIC. For this we must disable preemption. On
+ * success we initialize all nodes with this offset. This updates then
+ * the offset in the IBS_CTL per-node msr. The per-core APIC setup of
+ * the IBS interrupt vector is called from op_amd_setup_ctrs()/op_-
+ * amd_cpu_shutdown() using the new offset.
+ */
 static int force_ibs_eilvt_setup(void)
 {
 	int offset;
 	int ret;
 
-	/*
-	 * find the next free available EILVT entry, skip offset 0,
-	 * pin search to this cpu
-	 */
 	preempt_disable();
+	/* find the next free available EILVT entry, skip offset 0 */
 	for (offset = 1; offset < APIC_EILVT_NR_MAX; offset++) {
 		if (get_eilvt(offset))
 			break;

1.7.5.rc3



-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Yeah, looks good to me - feel free to handle this via the regular oprofile flow of patches.

Thanks,

Ingo

(resent with modified subject)

Ingo,

please see below and apply the patch to tip, if it makes sense to you.

Thanks,

-Robert

From: Robert Richter <[email protected]>
Date: Mon, 30 May 2011 16:31:11 +0200
Subject: [PATCH] oprofile, x86: Add comments to IBS LVT offset initialization

Adding a comment in the code as IBS LVT setup is not obvious at all ...

---
 arch/x86/kernel/apic/apic.c      |    3 ++-
 arch/x86/oprofile/op_model_amd.c |   13 +++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

index fabf01e..a0bf78a 100644
@@ -390,7 +390,8 @@ static unsigned int reserve_eilvt_offset(int offset, unsigned int new)
 
 /*
  * If mask=1, the LVT entry does not generate interrupts while mask=0
- * enables the vector. See also the BKDGs.
+ * enables the vector. See also the BKDGs. Must be called with
+ * preemption disabled.
  */
 
 int setup_APIC_eilvt(u8 offset, u8 vector, u8 msg_type, u8 mask)

index 9fd8a56..9cbb710 100644
@@ -609,16 +609,21 @@ static int setup_ibs_ctl(int ibs_eilvt_off)
 	return 0;
 }
 
+/*
+ * This runs only on the current cpu. We try to find an LVT offset and
+ * setup the local APIC. For this we must disable preemption. On
+ * success we initialize all nodes with this offset. This updates then
+ * the offset in the IBS_CTL per-node msr. The per-core APIC setup of
+ * the IBS interrupt vector is called from op_amd_setup_ctrs()/op_-
+ * amd_cpu_shutdown() using the new offset.
+ */
 static int force_ibs_eilvt_setup(void)
 {
 	int offset;
 	int ret;
 
-	/*
-	 * find the next free available EILVT entry, skip offset 0,
-	 * pin search to this cpu
-	 */
 	preempt_disable();
+	/* find the next free available EILVT entry, skip offset 0 */
 	for (offset = 1; offset < APIC_EILVT_NR_MAX; offset++) {
 		if (get_eilvt(offset))
 			break;

1.7.5.rc3



-- 
Advanced Micro Devices, Inc.
Operating System Research Center