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
* Robert Richter <[email protected]> wrote:
> Robert Richter (1):
> oprofile, x86: Enable preemption during pci device setup in IBS init
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
On 27.05.11 08:23:01, Ingo Molnar wrote:
>
> * Robert Richter <[email protected]> wrote:
>
> > Robert Richter (1):
> > oprofile, x86: Enable preemption during pci device setup in IBS init
>
> 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 ...
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.
> 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?
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
* Robert Richter <[email protected]> wrote:
> > 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?
>
> 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().
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
On 30.05.11 06:09:56, Ingo Molnar wrote:
>
> * Robert Richter <[email protected]> wrote:
>
> > > 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?
> >
> > 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().
>
> 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?
Yes, will add some comments.
> 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.
Have seen it, thanks Ingo,
-Robert
--
Advanced Micro Devices, Inc.
Operating System Research Center
On 30.05.11 12:16:19, Robert Richter wrote:
> On 30.05.11 06:09:56, Ingo Molnar wrote:
> > 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?
>
> Yes, will add some comments.
Ingo,
please see below and apply the patch to tip, if it makes sense to you.
Thanks,
-Robert
>From cbf74cea070fa1f705de4712e25d9e56ae6543c7 Mon Sep 17 00:00:00 2001
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 ...
Signed-off-by: Robert Richter <[email protected]>
---
arch/x86/kernel/apic/apic.c | 3 ++-
arch/x86/oprofile/op_model_amd.c | 13 +++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index fabf01e..a0bf78a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -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)
diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index 9fd8a56..9cbb710 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -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
(resent with modified subject)
On 30.05.11 12:16:19, Robert Richter wrote:
> On 30.05.11 06:09:56, Ingo Molnar wrote:
> > 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?
>
> Yes, will add some comments.
Ingo,
please see below and apply the patch to tip, if it makes sense to you.
Thanks,
-Robert
>From cbf74cea070fa1f705de4712e25d9e56ae6543c7 Mon Sep 17 00:00:00 2001
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 ...
Signed-off-by: Robert Richter <[email protected]>
---
arch/x86/kernel/apic/apic.c | 3 ++-
arch/x86/oprofile/op_model_amd.c | 13 +++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index fabf01e..a0bf78a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -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)
diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index 9fd8a56..9cbb710 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -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
* Robert Richter <[email protected]> wrote:
> On 30.05.11 12:16:19, Robert Richter wrote:
> > On 30.05.11 06:09:56, Ingo Molnar wrote:
> > > 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?
> >
> > Yes, will add some comments.
>
> Ingo,
>
> please see below and apply the patch to tip, if it makes sense to you.
Yeah, looks good to me - feel free to handle this via the regular
oprofile flow of patches.
Thanks,
Ingo