[PATCH 00/15] oprofile fixes and updates for v2.6.34

Robert Richter

unread,

Feb 26, 2010, 12:50:01 PM2/26/10

to

This patch series contains oprofile fixes and updates for v2.6.34. It
includes:

* Kconfig updates and fixes (patches 1, 2, 3)
* bug fixes (patches 4, 10, 12)
* IBS cpuid detection (patch 5)
* IBS software randomization (patches 6, 7)
* print warnings if counters are reserved or in use (patches 8, 9)
* code improvements (patch 11)
* x86 updates to perf_event.h to share code between subsystems
(patches 13, 14, 15)

Robert Richter

unread,

Feb 26, 2010, 12:50:02 PM2/26/10

to

This patch moves code from oprofile to perf_event.h to make it also
available for usage by perf.

Signed-off-by: Robert Richter <robert....@amd.com>
---
arch/x86/include/asm/perf_event.h | 10 ++++++++++
arch/x86/oprofile/op_model_amd.c | 11 -----------
2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 1380367..4745f65 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -105,6 +105,16 @@ union cpuid10_edx {
*/
#define X86_PMC_IDX_FIXED_BTS (X86_PMC_IDX_FIXED + 16)

+/* IbsFetchCtl bits/masks */
+#define IBS_FETCH_RAND_EN (1ULL<<57)
+#define IBS_FETCH_VAL (1ULL<<49)
+#define IBS_FETCH_ENABLE (1ULL<<48)
+#define IBS_FETCH_CNT_MASK 0xFFFF0000ULL
+
+/* IbsOpCtl bits */
+#define IBS_OP_CNT_CTL (1ULL<<19)
+#define IBS_OP_VAL (1ULL<<18)
+#define IBS_OP_ENABLE (1ULL<<17)

#ifdef CONFIG_PERF_EVENTS
extern void init_hw_perf_events(void);
diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index 6a58256..c671749 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -46,17 +46,6 @@

static unsigned long reset_value[NUM_VIRT_COUNTERS];

-/* IbsFetchCtl bits/masks */
-#define IBS_FETCH_RAND_EN (1ULL<<57)
-#define IBS_FETCH_VAL (1ULL<<49)
-#define IBS_FETCH_ENABLE (1ULL<<48)
-#define IBS_FETCH_CNT_MASK 0xFFFF0000ULL
-
-/* IbsOpCtl bits */
-#define IBS_OP_CNT_CTL (1ULL<<19)
-#define IBS_OP_VAL (1ULL<<18)
-#define IBS_OP_ENABLE (1ULL<<17)
-
#define IBS_FETCH_SIZE 6
#define IBS_OP_SIZE 12

--
1.6.6

Robert Richter

unread,

Feb 26, 2010, 12:50:03 PM2/26/10

to

This patch generates a warning if a counter is already active.

Implemented for AMD and P6 models. P4 is not supported.

Cc: Naga Chumbalkar <nagananda....@hp.com>
Cc: Shashi Belur <shashi-ki...@hp.com>
Cc: Tony Jones <to...@suse.de>

Signed-off-by: Robert Richter <robert....@amd.com>
---

arch/x86/oprofile/op_model_amd.c | 11 ++++++++++-
arch/x86/oprofile/op_model_ppro.c | 11 ++++++++++-
arch/x86/oprofile/op_x86_model.h | 11 +++++++++++
3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index a9d1947..ef9d735 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -194,9 +194,18 @@ static void op_amd_setup_ctrs(struct op_x86_model_spec const *model,

/* clear all counters */
for (i = 0; i < NUM_CONTROLS; ++i) {
- if (unlikely(!msrs->controls[i].addr))
+ if (unlikely(!msrs->controls[i].addr)) {
+ if (counter_config[i].enabled && !smp_processor_id())
+ /*
+ * counter is reserved, this is on all
+ * cpus, so report only for cpu #0
+ */
+ op_x86_warn_reserved(i);
continue;
+ }
rdmsrl(msrs->controls[i].addr, val);
+ if (val & ARCH_PERFMON_EVENTSEL0_ENABLE)
+ op_x86_warn_in_use(i);
val &= model->reserved;
wrmsrl(msrs->controls[i].addr, val);
}
diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 8eb0587..c344525 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -82,9 +82,18 @@ static void ppro_setup_ctrs(struct op_x86_model_spec const *model,

/* clear all counters */
for (i = 0; i < num_counters; ++i) {
- if (unlikely(!msrs->controls[i].addr))
+ if (unlikely(!msrs->controls[i].addr)) {
+ if (counter_config[i].enabled && !smp_processor_id())
+ /*
+ * counter is reserved, this is on all
+ * cpus, so report only for cpu #0
+ */
+ op_x86_warn_reserved(i);
continue;
+ }
rdmsrl(msrs->controls[i].addr, val);
+ if (val & ARCH_PERFMON_EVENTSEL0_ENABLE)
+ op_x86_warn_in_use(i);
val &= model->reserved;
wrmsrl(msrs->controls[i].addr, val);
}
diff --git a/arch/x86/oprofile/op_x86_model.h b/arch/x86/oprofile/op_x86_model.h
index 7b8e75d..59fa2bd 100644
--- a/arch/x86/oprofile/op_x86_model.h
+++ b/arch/x86/oprofile/op_x86_model.h
@@ -57,6 +57,17 @@ struct op_x86_model_spec {

struct op_counter_config;

+static inline void op_x86_warn_in_use(int counter)
+{
+ pr_warning("oprofile: counter #%d on cpu #%d may already be used\n",
+ counter, smp_processor_id());
+}
+
+static inline void op_x86_warn_reserved(int counter)
+{
+ pr_warning("oprofile: counter #%d is already reserved\n", counter);
+}
+
extern u64 op_x86_get_ctrl(struct op_x86_model_spec const *model,
struct op_counter_config *counter_config);
extern int op_x86_phys_to_virt(int phys);
--
1.6.6

Ingo Molnar

unread,

Feb 27, 2010, 4:10:01 AM2/27/10

to

Hm, the perf_event.c bits conflict quite heavily with pending changes in
tip:perf/core.

So to not hold up the oprofile changes for v2.6.34 i've pulled the core
oprofile changes for v2.6.34 into tip:oprofile (up to cfc9c0b, if that is fine
with you), and mind reworking the last 3 patches against perf/core?

On a related note, wrt. your ongoing work for perf IBS support. The following
patch by Peter:

f22f54f: perf_events, x86: Split PMU definitions into separate files

Will probably interact with those patches.

As usual you can avoid such interactions and conflicts by pushing any pending
bits to Peter and me earlier. It can even be disabled temporarily (if that can
be done cleanly), if IBS support is not fully working yet.

Thanks,

Ingo

Robert Richter

unread,

Mar 1, 2010, 9:50:02 AM3/1/10

to

On 27.02.10 10:03:31, Ingo Molnar wrote:
> Hm, the perf_event.c bits conflict quite heavily with pending changes in
> tip:perf/core.
>
> So to not hold up the oprofile changes for v2.6.34 i've pulled the core
> oprofile changes for v2.6.34 into tip:oprofile (up to cfc9c0b, if that is fine
> with you), and mind reworking the last 3 patches against perf/core?

Ingo & Peter,

I have a rebased version containing also a merge from:

tip/oprofile -> tip/perf/core

Please pull again from:

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

>
> On a related note, wrt. your ongoing work for perf IBS support. The following
> patch by Peter:
>
> f22f54f: perf_events, x86: Split PMU definitions into separate files

Yes, the patches went also upstream on Friday. Will update my current
patch stack now. Maybe it would have been better to first integrate
all pending patches before doing that split. Conflict resolution is a
pain for moving code. But anyway, it is supposed to go upstream and I
will rebase my patches.

>
> Will probably interact with those patches.
>
> As usual you can avoid such interactions and conflicts by pushing any pending
> bits to Peter and me earlier. It can even be disabled temporarily (if that can
> be done cleanly), if IBS support is not fully working yet.

Hmm, I was working with latest tip/perf/core, but I was waiting with
my submission for Peter's quilt queue since I know there were
conflicts again.

I will start submitting patches now relative to latest
tip/perf/core. There shouldn't be more patches flying around.

I decided to send the perf_event.h changes via the oprofile tree since
tip/perf/core was not stable enough. The patches contain cross
subsystem changes and are not only for perf. Perf-only patches I wont
send upstream via oprofile.

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert....@amd.com

Ingo Molnar

unread,

Mar 2, 2010, 5:10:02 AM3/2/10

to

* Robert Richter <robert....@amd.com> wrote:

> On 27.02.10 10:03:31, Ingo Molnar wrote:
> > Hm, the perf_event.c bits conflict quite heavily with pending changes in
> > tip:perf/core.
> >
> > So to not hold up the oprofile changes for v2.6.34 i've pulled the core
> > oprofile changes for v2.6.34 into tip:oprofile (up to cfc9c0b, if that is fine
> > with you), and mind reworking the last 3 patches against perf/core?
>
> Ingo & Peter,
>
> I have a rebased version containing also a merge from:
>
> tip/oprofile -> tip/perf/core
>
> Please pull again from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core

Pulled, thanks Robert!

> > On a related note, wrt. your ongoing work for perf IBS support. The following
> > patch by Peter:
> >
> > f22f54f: perf_events, x86: Split PMU definitions into separate files
>
> Yes, the patches went also upstream on Friday. Will update my current patch
> stack now. Maybe it would have been better to first integrate all pending
> patches before doing that split. Conflict resolution is a pain for moving
> code. But anyway, it is supposed to go upstream and I will rebase my
> patches.

The way to do it is to not keep that many pending patches in the first place.
There are always 'pending patches' - if we waited for them to drain then we
could never do cleanups.

Peter, Arnaldo, Frederic and all the other perf developers sync their patches
with perf/core very frequently and push these bits to me. This has various
advantages:

- breakages are discovered quickly and can be mitigated before more harm is
done.

- patch review is a lot more gradual (and a lot less taxing) as well.

- other people can get interested and jump in and help you out.

- design mistakes/disagreements can also be hashed out before too much work
is invested into a particular direction.

- as a maintainer i can see progress and can see problem areas. This makes it
much easier to judge and plan a new topic's upstream integration schedule.
It's not tucked away in some tree, with a large stream of patches showing
up at once.

i.e. it's all variations of the 'release early, release often' Linux mantra.

So if you have a significant amount of pending patches then you are doing
something wrong. Please consider adapting your workflow and please get any
pending pieces to us in small, reviewable pieces.

That's especially true of large, intrusive features: for example the IBS bits
could be moved into perf/core gradually, in a series of clean steps, even if
they are not functional yet.

Thanks,

Ingo