Debug messages added for better debugging.
Signed-off-by: Robert Richter <[email protected]>
Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
===================================================================
--- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
+++ linux-2.6.22-rc3/perfmon/perfmon_file.c
@@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
unsigned long flags;
int ret;
+ PFM_DBG("pfm_file_ops");
+
ctx = file->private_data;
size = (vma->vm_end - vma->vm_start);
@@ -332,6 +334,8 @@ static ssize_t pfm_read(struct file *fil
union pfarg_msg msg_buf;
int non_block, ret;
+ PFM_DBG("pfm_file_ops");
+
ctx = filp->private_data;
if (ctx == NULL) {
PFM_ERR("no ctx for pfm_read");
@@ -375,6 +379,8 @@ static unsigned int pfm_poll(struct file
unsigned long flags;
unsigned int mask = 0;
+ PFM_DBG("pfm_file_ops");
+
if (filp->f_op != &pfm_file_ops) {
PFM_ERR("pfm_poll bad magic");
return 0;
@@ -449,6 +455,8 @@ static int pfm_fasync(int fd, struct fil
struct pfm_context *ctx;
int ret;
+ PFM_DBG("pfm_file_ops");
+
ctx = filp->private_data;
if (ctx == NULL) {
PFM_ERR("pfm_fasync no ctx");
@@ -611,6 +619,8 @@ static int pfm_close(struct inode *inode
{
struct pfm_context *ctx;
+ PFM_DBG("pfm_file_ops");
+
ctx = filp->private_data;
if (ctx == NULL) {
PFM_ERR("no ctx");
@@ -621,6 +631,8 @@ static int pfm_close(struct inode *inode
static int pfm_no_open(struct inode *irrelevant, struct file *dontcare)
{
+ PFM_DBG("pfm_file_ops");
+
return -ENXIO;
}
@@ -637,6 +649,8 @@ static int pfm_flush(struct file *filp,
{
struct pfm_context *ctx;
+ PFM_DBG("pfm_file_ops");
+
ctx = filp->private_data;
if (ctx == NULL) {
PFM_ERR("pfm_flush no ctx");
Index: linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
===================================================================
--- linux-2.6.22-rc3.orig/perfmon/perfmon_syscalls.c
+++ linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
@@ -403,6 +403,8 @@ asmlinkage long sys_pfm_create_context(s
void *fmt_arg = NULL;
int ret;
+ PFM_DBG("syscall");
+
if (atomic_read(&perfmon_disabled))
return -ENOSYS;
@@ -433,8 +435,12 @@ asmlinkage long sys_pfm_write_pmcs(int f
size_t sz;
int ret, fput_needed;
- if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
+ PFM_DBG("syscall");
+
+ if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq)) {
+ PFM_DBG("invalid arg count %d", count);
return -EINVAL;
+ }
sz = count*sizeof(*ureq);
@@ -475,6 +481,8 @@ asmlinkage long sys_pfm_write_pmcs(int f
kfree(fptr);
error:
fput_light(filp, fput_needed);
+ if (ret)
+ PFM_DBG("failed: errno=%d", -ret);
return ret;
}
@@ -490,6 +498,8 @@ asmlinkage long sys_pfm_write_pmds(int f
size_t sz;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
return -EINVAL;
@@ -543,6 +553,8 @@ asmlinkage long sys_pfm_read_pmds(int fd
size_t sz;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
return -EINVAL;
@@ -591,6 +603,8 @@ asmlinkage long sys_pfm_restart(int fd)
unsigned long flags;
int ret, fput_needed, complete_needed;
+ PFM_DBG("syscall");
+
filp = fget_light(fd, &fput_needed);
if (unlikely(filp == NULL)) {
PFM_DBG("invalid fd %d", fd);
@@ -647,6 +661,8 @@ asmlinkage long sys_pfm_stop(int fd)
unsigned long flags;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
filp = fget_light(fd, &fput_needed);
if (unlikely(filp == NULL)) {
PFM_DBG("invalid fd %d", fd);
@@ -682,6 +698,8 @@ asmlinkage long sys_pfm_start(int fd, st
unsigned long flags;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
filp = fget_light(fd, &fput_needed);
if (unlikely(filp == NULL)) {
PFM_DBG("invalid fd %d", fd);
@@ -724,6 +742,8 @@ asmlinkage long sys_pfm_load_context(int
struct pfarg_load req;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (copy_from_user(&req, ureq, sizeof(req)))
return -EFAULT;
@@ -792,6 +812,8 @@ asmlinkage long sys_pfm_unload_context(i
int is_system, can_release = 0;
u32 cpu;
+ PFM_DBG("syscall");
+
filp = fget_light(fd, &fput_needed);
if (unlikely(filp == NULL)) {
PFM_DBG("invalid fd %d", fd);
@@ -834,6 +856,8 @@ asmlinkage long sys_pfm_create_evtsets(i
size_t sz;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
return -EINVAL;
@@ -890,6 +914,8 @@ asmlinkage long sys_pfm_getinfo_evtsets
size_t sz;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
return -EINVAL;
@@ -932,6 +958,8 @@ asmlinkage long sys_pfm_getinfo_evtsets
kfree(fptr);
error:
fput_light(filp, fput_needed);
+ if (ret)
+ PFM_DBG("failed: errno=%d", -ret);
return ret;
}
@@ -945,6 +973,8 @@ asmlinkage long sys_pfm_delete_evtsets(i
size_t sz;
int ret, fput_needed;
+ PFM_DBG("syscall");
+
if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
return -EINVAL;
Index: linux-2.6.22-rc3/perfmon/perfmon_rw.c
===================================================================
--- linux-2.6.22-rc3.orig/perfmon/perfmon_rw.c
+++ linux-2.6.22-rc3/perfmon/perfmon_rw.c
@@ -474,7 +474,7 @@ int __pfm_write_pmcs(struct pfm_context
return 0;
error:
pfm_retflag_set(req->reg_flags, error_code);
- PFM_DBG("set%u pmc%u error=%d", set_id, cnum, error_code);
+ PFM_DBG("set%u pmc%u error=0x%08x", set_id, cnum, error_code);
return ret;
}
Index: linux-2.6.22-rc3/include/asm-i386/perfmon.h
===================================================================
--- linux-2.6.22-rc3.orig/include/asm-i386/perfmon.h
+++ linux-2.6.22-rc3/include/asm-i386/perfmon.h
@@ -140,6 +140,10 @@ static inline void pfm_arch_write_pmc(st
if (ctx && ctx->flags.started == 0)
return;
+ PFM_DBG_ovfl("pfm_arch_write_pmc(0x%016Lx, 0x%016Lx)",
+ (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
+ (unsigned long long) value);
+ BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
if (arch_info->pmu_style == PFM_X86_PMU_P4)
__pfm_write_reg_p4(&arch_info->pmc_addrs[cnum], value);
else
@@ -155,6 +159,10 @@ static inline void pfm_arch_write_pmd(st
if (pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_C64)
value |= ~pfm_pmu_conf->ovfl_mask;
+ PFM_DBG_ovfl("pfm_arch_write_pmd(0x%016Lx, 0x%016Lx)",
+ (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
+ (unsigned long long) value);
+ BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
if (arch_info->pmu_style == PFM_X86_PMU_P4)
__pfm_write_reg_p4(&arch_info->pmd_addrs[cnum], value);
else
@@ -165,10 +173,14 @@ static inline u64 pfm_arch_read_pmd(stru
{
struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
u64 tmp;
+ BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
if (arch_info->pmu_style == PFM_X86_PMU_P4)
__pfm_read_reg_p4(&arch_info->pmd_addrs[cnum], &tmp);
else
rdmsrl(pfm_pmu_conf->pmd_desc[cnum].hw_addr, tmp);
+ PFM_DBG_ovfl("pfm_arch_read_pmd(0x%016Lx) = 0x%016Lx",
+ (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
+ (unsigned long long) tmp);
return tmp;
}
@@ -176,10 +188,14 @@ static inline u64 pfm_arch_read_pmc(stru
{
struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
u64 tmp;
+ BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
if (arch_info->pmu_style == PFM_X86_PMU_P4)
__pfm_read_reg_p4(&arch_info->pmc_addrs[cnum], &tmp);
else
rdmsrl(pfm_pmu_conf->pmc_desc[cnum].hw_addr, tmp);
+ PFM_DBG_ovfl("pfm_arch_read_pmc(0x%016Lx) = 0x%016Lx",
+ (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
+ (unsigned long long) tmp);
return tmp;
}
Index: linux-2.6.22-rc3/perfmon/perfmon.c
===================================================================
--- linux-2.6.22-rc3.orig/perfmon/perfmon.c
+++ linux-2.6.22-rc3/perfmon/perfmon.c
@@ -865,7 +865,8 @@ int __init pfm_init(void)
{
int ret;
- PFM_LOG("version %u.%u", PFM_VERSION_MAJ, PFM_VERSION_MIN);
+ PFM_LOG("version %u.%u, compiled: " __DATE__ ", " __TIME__,
+ PFM_VERSION_MAJ, PFM_VERSION_MIN);
pfm_ctx_cachep = kmem_cache_create("pfm_context",
sizeof(struct pfm_context)+PFM_ARCH_CTX_SIZE,
Index: linux-2.6.22-rc3/perfmon/perfmon_intr.c
===================================================================
--- linux-2.6.22-rc3.orig/perfmon/perfmon_intr.c
+++ linux-2.6.22-rc3/perfmon/perfmon_intr.c
@@ -170,6 +170,7 @@ static void pfm_overflow_handler(struct
* check for overflow condition
*/
if (likely(old_val > new_val)) {
+ PFM_DBG_ovfl("64 bit overflow of PMD%d", i);
has_64b_ovfl = 1;
if (has_ovfl_sw && ovfl_thres > 0) {
if (ovfl_thres == 1)
@@ -188,11 +189,13 @@ static void pfm_overflow_handler(struct
max_pmd);
} else {
+ PFM_DBG_ovfl("Hardware counter overflow of PMD%d=0x%04llx",
+ i, new_val);
/* only keep track of 64-bit overflows */
__clear_bit(i, ulp(pend_ovfls));
/*
- * on some PMU, it may be necessary to re-arm the PMD
- */
+ * on some PMU, it may be necessary to re-arm the PMD
+ */
pfm_arch_ovfl_reset_pmd(ctx, i);
}
--
AMD Saxony, Dresden, Germany
Operating System Research Center
email: [email protected]
On Wed, 20 Jun 2007, Robert Richter wrote:
> Debug messages added for better debugging.
>
And you added BUG_ON()'s.
> Signed-off-by: Robert Richter <[email protected]>
>
> Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
> +++ linux-2.6.22-rc3/perfmon/perfmon_file.c
> @@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
> unsigned long flags;
> int ret;
>
> + PFM_DBG("pfm_file_ops");
After commenting on your first set of patches, I've been using it a little
more. In my use, these debugging messages weren't very helpful because
"pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
these be changed to be more specific based on the function they're in?
> Index: linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/perfmon/perfmon_syscalls.c
> +++ linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
> @@ -403,6 +403,8 @@ asmlinkage long sys_pfm_create_context(s
> void *fmt_arg = NULL;
> int ret;
>
> + PFM_DBG("syscall");
Likewise. Using "syscall" for all debugging messages in the syscall
handlers isn't very informative. Could this be PFM_DBG(__FUNCTION__)
instead?
> +
> if (atomic_read(&perfmon_disabled))
> return -ENOSYS;
>
> @@ -433,8 +435,12 @@ asmlinkage long sys_pfm_write_pmcs(int f
> size_t sz;
> int ret, fput_needed;
>
> - if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
> + PFM_DBG("syscall");
> +
> + if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq)) {
> + PFM_DBG("invalid arg count %d", count);
This is whitespace damaged.
> return -EINVAL;
> + }
>
> sz = count*sizeof(*ureq);
>
> @@ -475,6 +481,8 @@ asmlinkage long sys_pfm_write_pmcs(int f
> kfree(fptr);
> error:
> fput_light(filp, fput_needed);
> + if (ret)
> + PFM_DBG("failed: errno=%d", -ret);
What failed? More information would be helpful since this is, after all,
a diagnostic message.
> Index: linux-2.6.22-rc3/include/asm-i386/perfmon.h
> ===================================================================
> --- linux-2.6.22-rc3.orig/include/asm-i386/perfmon.h
> +++ linux-2.6.22-rc3/include/asm-i386/perfmon.h
> @@ -140,6 +140,10 @@ static inline void pfm_arch_write_pmc(st
> if (ctx && ctx->flags.started == 0)
> return;
>
> + PFM_DBG_ovfl("pfm_arch_write_pmc(0x%016Lx, 0x%016Lx)",
> + (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
> + (unsigned long long) value);
Casting here should be unnecessary. Changing %L would be advisible to
display the data as it is stored in the object.
> + BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
> if (arch_info->pmu_style == PFM_X86_PMU_P4)
> __pfm_write_reg_p4(&arch_info->pmc_addrs[cnum], value);
> else
> @@ -155,6 +159,10 @@ static inline void pfm_arch_write_pmd(st
> if (pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_C64)
> value |= ~pfm_pmu_conf->ovfl_mask;
>
> + PFM_DBG_ovfl("pfm_arch_write_pmd(0x%016Lx, 0x%016Lx)",
> + (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
> + (unsigned long long) value);
> + BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
> if (arch_info->pmu_style == PFM_X86_PMU_P4)
> __pfm_write_reg_p4(&arch_info->pmd_addrs[cnum], value);
> else
> @@ -165,10 +173,14 @@ static inline u64 pfm_arch_read_pmd(stru
> {
> struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> u64 tmp;
> + BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
> if (arch_info->pmu_style == PFM_X86_PMU_P4)
> __pfm_read_reg_p4(&arch_info->pmd_addrs[cnum], &tmp);
> else
> rdmsrl(pfm_pmu_conf->pmd_desc[cnum].hw_addr, tmp);
> + PFM_DBG_ovfl("pfm_arch_read_pmd(0x%016Lx) = 0x%016Lx",
> + (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
> + (unsigned long long) tmp);
> return tmp;
> }
>
> @@ -176,10 +188,14 @@ static inline u64 pfm_arch_read_pmc(stru
> {
> struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> u64 tmp;
> + BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
> if (arch_info->pmu_style == PFM_X86_PMU_P4)
> __pfm_read_reg_p4(&arch_info->pmc_addrs[cnum], &tmp);
> else
> rdmsrl(pfm_pmu_conf->pmc_desc[cnum].hw_addr, tmp);
> + PFM_DBG_ovfl("pfm_arch_read_pmc(0x%016Lx) = 0x%016Lx",
> + (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
> + (unsigned long long) tmp);
More whitespace damage.
David,
On Wed, Jun 20, 2007 at 12:49:05PM -0700, David Rientjes wrote:
> On Wed, 20 Jun 2007, Robert Richter wrote:
>
> > Debug messages added for better debugging.
> >
>
> And you added BUG_ON()'s.
>
> > Signed-off-by: Robert Richter <[email protected]>
> >
> > Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
> > +++ linux-2.6.22-rc3/perfmon/perfmon_file.c
> > @@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
> > unsigned long flags;
> > int ret;
> >
> > + PFM_DBG("pfm_file_ops");
>
> After commenting on your first set of patches, I've been using it a little
> more. In my use, these debugging messages weren't very helpful because
> "pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
> these be changed to be more specific based on the function they're in?
>
the PFM_DBG() macro adds the CPU, function name and line number to the
printed string.
--
-Stephane
On Wed, 20 Jun 2007, Stephane Eranian wrote:
> > After commenting on your first set of patches, I've been using it a little
> > more. In my use, these debugging messages weren't very helpful because
> > "pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
> > these be changed to be more specific based on the function they're in?
> >
> the PFM_DBG() macro adds the CPU, function name and line number to the
> printed string.
>
That's not the case in what I currently have in my tree, I'll go back and
see what I'm missing.
What's the current status of the perfmon2 patchset that Robert's work is
based on?
On Wed, Jun 20, 2007 at 01:28:50PM -0700, David Rientjes wrote:
> On Wed, 20 Jun 2007, Stephane Eranian wrote:
>
> > > After commenting on your first set of patches, I've been using it a little
> > > more. In my use, these debugging messages weren't very helpful because
> > > "pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
> > > these be changed to be more specific based on the function they're in?
> > >
> > the PFM_DBG() macro adds the CPU, function name and line number to the
> > printed string.
> >
>
> That's not the case in what I currently have in my tree, I'll go back and
> see what I'm missing.
Bizarre. Becuase if you've disabled CONFIG_PERFMON_DEBUG, then all the PFM_DBG()
are removed.
>
> What's the current status of the perfmon2 patchset that Robert's work is
> based on?
Well, I have been hacking on it a lot especially to allow Oprofile and
perfmon2 to co-exist (mutual-exclusion). I am hoping to release something
before I fly to OLS next week. I have started integrating Robert's patches.
I also have to integrate IBM's Cell support patch.
--
-Stephane
On Wed, 20 Jun 2007, Stephane Eranian wrote:
> > What's the current status of the perfmon2 patchset that Robert's work is
> > based on?
>
> Well, I have been hacking on it a lot especially to allow Oprofile and
> perfmon2 to co-exist (mutual-exclusion). I am hoping to release something
> before I fly to OLS next week. I have started integrating Robert's patches.
> I also have to integrate IBM's Cell support patch.
>
Mutual exclusion between oprofile and perfmon2 will be great; I'm
starting to see why you added the "implementation" field. If you could
integrate Robert's patches into your own then I think it will be easier
for review from the community.
David,
On Wed, Jun 20, 2007 at 01:45:22PM -0700, David Rientjes wrote:
> On Wed, 20 Jun 2007, Stephane Eranian wrote:
>
> > > What's the current status of the perfmon2 patchset that Robert's work is
> > > based on?
> >
> > Well, I have been hacking on it a lot especially to allow Oprofile and
> > perfmon2 to co-exist (mutual-exclusion). I am hoping to release something
> > before I fly to OLS next week. I have started integrating Robert's patches.
> > I also have to integrate IBM's Cell support patch.
> >
>
> Mutual exclusion between oprofile and perfmon2 will be great; I'm
> starting to see why you added the "implementation" field. If you could
> integrate Robert's patches into your own then I think it will be easier
> for review from the community.
I have removed the "implementation" field for now. It is only useful if
Oprofile is implemented on top of perfmon2. It is used by Oprofile
tools to detects which interface to use so that they can be used with
older kernels as well. But for now, I am shooting for Oprofile using
its own kernel code and /dev/oprofile interface. That will provide
for a smoother transition.
--
-Stephane
On 20.06.07 13:22:16, Stephane Eranian wrote:
> David,
>
> On Wed, Jun 20, 2007 at 12:49:05PM -0700, David Rientjes wrote:
> > On Wed, 20 Jun 2007, Robert Richter wrote:
> >
> > > Debug messages added for better debugging.
> > >
> >
> > And you added BUG_ON()'s.
> >
> > > Signed-off-by: Robert Richter <[email protected]>
> > >
> > > Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
> > > ===================================================================
> > > --- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
> > > +++ linux-2.6.22-rc3/perfmon/perfmon_file.c
> > > @@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
> > > unsigned long flags;
> > > int ret;
> > >
> > > + PFM_DBG("pfm_file_ops");
> >
> > After commenting on your first set of patches, I've been using it a little
> > more. In my use, these debugging messages weren't very helpful because
> > "pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
> > these be changed to be more specific based on the function they're in?
> >
> the PFM_DBG() macro adds the CPU, function name and line number to the
> printed string.
Right, I had this in mind.
-Robert
--
AMD Saxony, Dresden, Germany
Operating System Research Center
email: [email protected]
"Robert Richter" <[email protected]> writes:
> Debug messages added for better debugging.
>
> Signed-off-by: Robert Richter <[email protected]>
>
> Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
> +++ linux-2.6.22-rc3/perfmon/perfmon_file.c
> @@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
> unsigned long flags;
> int ret;
>
> + PFM_DBG("pfm_file_ops");
I don't think that should be in the code when it hits mainline
-Andi
On 20.06.07 12:49:05, David Rientjes wrote:
> On Wed, 20 Jun 2007, Robert Richter wrote:
>
> > Debug messages added for better debugging.
> >
>
> And you added BUG_ON()'s.
>
> > Signed-off-by: Robert Richter <[email protected]>
> >
> > Index: linux-2.6.22-rc3/perfmon/perfmon_file.c
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/perfmon/perfmon_file.c
> > +++ linux-2.6.22-rc3/perfmon/perfmon_file.c
> > @@ -192,6 +192,8 @@ static int pfm_mmap(struct file *file, s
> > unsigned long flags;
> > int ret;
> >
> > + PFM_DBG("pfm_file_ops");
>
> After commenting on your first set of patches, I've been using it a little
> more. In my use, these debugging messages weren't very helpful because
> "pfm_file_ops" can indicate pfm_mmap, pfm_read, pfm_poll, etc. Could
> these be changed to be more specific based on the function they're in?
>
> > Index: linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/perfmon/perfmon_syscalls.c
> > +++ linux-2.6.22-rc3/perfmon/perfmon_syscalls.c
> > @@ -403,6 +403,8 @@ asmlinkage long sys_pfm_create_context(s
> > void *fmt_arg = NULL;
> > int ret;
> >
> > + PFM_DBG("syscall");
>
> Likewise. Using "syscall" for all debugging messages in the syscall
> handlers isn't very informative. Could this be PFM_DBG(__FUNCTION__)
> instead?
>
> > +
> > if (atomic_read(&perfmon_disabled))
> > return -ENOSYS;
> >
> > @@ -433,8 +435,12 @@ asmlinkage long sys_pfm_write_pmcs(int f
> > size_t sz;
> > int ret, fput_needed;
> >
> > - if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq))
> > + PFM_DBG("syscall");
> > +
> > + if (count < 0 || count >= PFM_MAX_ARG_COUNT(ureq)) {
> > + PFM_DBG("invalid arg count %d", count);
>
> This is whitespace damaged.
Fixed in patch version 3.
>
> > return -EINVAL;
> > + }
> >
> > sz = count*sizeof(*ureq);
> >
> > @@ -475,6 +481,8 @@ asmlinkage long sys_pfm_write_pmcs(int f
> > kfree(fptr);
> > error:
> > fput_light(filp, fput_needed);
> > + if (ret)
> > + PFM_DBG("failed: errno=%d", -ret);
>
> What failed? More information would be helpful since this is, after all,
> a diagnostic message.
Removed in version 3.
>
> > Index: linux-2.6.22-rc3/include/asm-i386/perfmon.h
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/include/asm-i386/perfmon.h
> > +++ linux-2.6.22-rc3/include/asm-i386/perfmon.h
> > @@ -140,6 +140,10 @@ static inline void pfm_arch_write_pmc(st
> > if (ctx && ctx->flags.started == 0)
> > return;
> >
> > + PFM_DBG_ovfl("pfm_arch_write_pmc(0x%016Lx, 0x%016Lx)",
> > + (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
> > + (unsigned long long) value);
>
> Casting here should be unnecessary. Changing %L would be advisible to
> display the data as it is stored in the object.
Not casting here would lead to warnings if compiling a 32 bit kernel.
>
> > + BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
> > if (arch_info->pmu_style == PFM_X86_PMU_P4)
> > __pfm_write_reg_p4(&arch_info->pmc_addrs[cnum], value);
> > else
> > @@ -155,6 +159,10 @@ static inline void pfm_arch_write_pmd(st
> > if (pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_C64)
> > value |= ~pfm_pmu_conf->ovfl_mask;
> >
> > + PFM_DBG_ovfl("pfm_arch_write_pmd(0x%016Lx, 0x%016Lx)",
> > + (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
> > + (unsigned long long) value);
> > + BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
> > if (arch_info->pmu_style == PFM_X86_PMU_P4)
> > __pfm_write_reg_p4(&arch_info->pmd_addrs[cnum], value);
> > else
> > @@ -165,10 +173,14 @@ static inline u64 pfm_arch_read_pmd(stru
> > {
> > struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> > u64 tmp;
> > + BUG_ON(pfm_pmu_conf->pmd_desc[cnum].type & PFM_REG_V);
> > if (arch_info->pmu_style == PFM_X86_PMU_P4)
> > __pfm_read_reg_p4(&arch_info->pmd_addrs[cnum], &tmp);
> > else
> > rdmsrl(pfm_pmu_conf->pmd_desc[cnum].hw_addr, tmp);
> > + PFM_DBG_ovfl("pfm_arch_read_pmd(0x%016Lx) = 0x%016Lx",
> > + (unsigned long long) pfm_pmu_conf->pmd_desc[cnum].hw_addr,
> > + (unsigned long long) tmp);
> > return tmp;
> > }
> >
> > @@ -176,10 +188,14 @@ static inline u64 pfm_arch_read_pmc(stru
> > {
> > struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> > u64 tmp;
> > + BUG_ON(pfm_pmu_conf->pmc_desc[cnum].type & PFM_REG_V);
> > if (arch_info->pmu_style == PFM_X86_PMU_P4)
> > __pfm_read_reg_p4(&arch_info->pmc_addrs[cnum], &tmp);
> > else
> > rdmsrl(pfm_pmu_conf->pmc_desc[cnum].hw_addr, tmp);
> > + PFM_DBG_ovfl("pfm_arch_read_pmc(0x%016Lx) = 0x%016Lx",
> > + (unsigned long long) pfm_pmu_conf->pmc_desc[cnum].hw_addr,
> > + (unsigned long long) tmp);
>
> More whitespace damage.
Fixed in patch version 3.
>
>
-Robert
--
AMD Saxony, Dresden, Germany
Operating System Research Center
email: [email protected]