Re: debugctl msr

["stephane eranian" <eranian@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

Markus,

Speaking of locking, I also ran into another issue with ds_lock.
Perfmon sessions each have a spinlock for access serialization, but to
prevent from PMU and timers interrupts, interrupts are masked. Thus,
when perfmon
calls ds.c, interrupts are masked. That means that we lock/unlock ds_lock
with interrupts disabled. The lock checker triggered when I ran a simple perfmon
session and warned of possible lock inversion. Suppose you are coming from the
ptrace code into ds. You grab ds_lock, but the same process is also running
a perfmon session with PEBS and a counter overflows, you get into
the PMU interrupt handler which may call into ds.c and try to grab the ds_lock.
For that reason, I think you should use a
spin_lock_irqsave/spin_unlock_irqrestore
pairs to protect your ds context.

I found another issue with ds_release(). You need to skip freeing the
buffer when it
is NULL, i.e., was already allocated by caller of ds_request_pebs().

I have attached a diff for the ds.c interface. It disables
ds_validate_access(), export
the PEBS functions to modules, fixes ds_release().

As for handling the interrupt is ds.c, not clear how this could work
with current perfmon.
I don't know how this work on the BTS side. On the PMU side, that is not because
I am using PEBS, that I don't also use other counters as well. Longer
term, I think, there
needs to be a lower-level PMU interrupt service where you would
register a callback
on PMU interrupts. It would be used by NMI watchdog, perfmon,
Oprofile, ds.c. Callbacks
would be chained, just like what happens with the NMI interrupt
handler. Such service would
go hand in hand with a centralized PMU MSR allocator which would
provide safe sharing of
the PMU registers between different subsystems (there is a primitive
version of this already
today in perfctr-watchdog.c).



On Wed, Nov 19, 2008 at 4:47 PM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>>-----Original Message-----
>>From: stephane eranian [mailto:eranian@googlemail.com]
>>Sent: Mittwoch, 19. November 2008 14:00
>>To: Metzger, Markus T
>>Cc: Markus Metzger; Ingo Molnar; Andi Kleen; Andrew Morton;
>>linux-kernel@vger.kernel.org
>>Subject: Re: debugctl msr
>>
>
>>I had to hack ds.c some more to make forward progress with PEBS. First
>>of all my PEBS code is
>>in a kernel module, so all PEBS functions have to be exported.
>>Furhtermore, I need a
>>ds_get_pebs_thres() and ds_set_pebs_thres() calls.
>
> I'm having a deja vu. We had this discussion before. You reported those
> issues and I fixed them. Same for the PEBS size; and Andi Kleen asked
> to exclude ds.c from the build instead of guarding the .c file and to
> use the mm semaphore in ds_allocate_buffer().
>
> That thread ended in the multiplexing discussion and my fixes never got in.
> I'll send a patch covering those problems next week.
>
>
> Regarding access to the interrupt threshold, we never completed
> our discussion.
> If we look towards multiplexing, ds.c has to handle interrupts and
> copy the trace to the various users - at least for BTS.
> I will pull some of the BTS handling from ptrace into ds.c - ds.c needs
> to be able to disable BTS recording for overflow handling and we would
> not want this knowledge in two different places.
>
> I would add those functions now to proceed with the perfmon2 adaptation
> and later remove them again and put overflow handling into ds.c
>
> I'll send a separate patch for those.
>
>
>>But the one key problem is ds_validate_access(). I had to disable this
>>function.
>
> I agree that this is too ptrace centric.
>
> It works fine for the ptrace bts extenstion since ptrace has similar
> restrictions, already.
>
> A better choice would be to return an opaque handle.
>
>
> regards,
> markus.
> ---------------------------------------------------------------------
> Intel GmbH
> Dornacher Strasse 1
> 85622 Feldkirchen/Muenchen Germany
> Sitz der Gesellschaft: Feldkirchen bei Muenchen
> Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
> Registergericht: Muenchen HRB 47456 Ust.-IdNr.
> VAT Registration No.: DE129385895
> Citibank Frankfurt (BLZ 502 109 00) 600119052
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>

[-- Attachment #2: ds.diff --]
[-- Type: application/octet-stream, Size: 4067 bytes --]

diff --git a/arch/x86/include/asm/ds.h b/arch/x86/include/asm/ds.h
index 72c5a19..fed664a 100644
--- a/arch/x86/include/asm/ds.h
+++ b/arch/x86/include/asm/ds.h
@@ -78,6 +78,7 @@ extern int ds_release_pebs(struct task_struct *task);
  */
 extern int ds_get_bts_index(struct task_struct *task, size_t *pos);
 extern int ds_get_pebs_index(struct task_struct *task, size_t *pos);
+extern int ds_get_pebs_thres(struct task_struct *task, size_t *pos);
 
 /*
  * Return the (array) index one record beyond the end of the array.
diff --git a/arch/x86/kernel/ds.c b/arch/x86/kernel/ds.c
index 2b69994..5882014 100644
--- a/arch/x86/kernel/ds.c
+++ b/arch/x86/kernel/ds.c
@@ -30,7 +30,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/mm.h>
-
+#include <linux/module.h>
 
 /*
  * The configuration for a particular DS hardware implementation.
@@ -131,7 +131,7 @@ static inline int ds_validate_access(struct ds_context *context,
 {
 	if (!context)
 		return -EPERM;
-
+return 0;
 	if (context->owner[qual] == current)
 		return 0;
 
@@ -467,6 +467,7 @@ int ds_request_pebs(struct task_struct *task, void *base, size_t size,
 {
 	return ds_request(task, base, size, ovfl, ds_pebs);
 }
+EXPORT_SYMBOL(ds_request_pebs);
 
 static int ds_release(struct task_struct *task, enum ds_qualifier qual)
 {
@@ -478,12 +479,14 @@ static int ds_release(struct task_struct *task, enum ds_qualifier qual)
 	if (error < 0)
 		goto out;
 
-	kfree(context->buffer[qual]);
-	context->buffer[qual] = NULL;
+	if (context->buffer[qual]) {
+		kfree(context->buffer[qual]);
+		context->buffer[qual] = NULL;
 
-	current->mm->total_vm  -= context->pages[qual];
-	current->mm->locked_vm -= context->pages[qual];
-	context->pages[qual] = 0;
+		current->mm->total_vm  -= context->pages[qual];
+		current->mm->locked_vm -= context->pages[qual];
+		context->pages[qual] = 0;
+	}
 	context->owner[qual] = NULL;
 
 	/*
@@ -506,6 +509,7 @@ int ds_release_pebs(struct task_struct *task)
 {
 	return ds_release(task, ds_pebs);
 }
+EXPORT_SYMBOL(ds_release_pebs);
 
 static int ds_get_index(struct task_struct *task, size_t *pos,
 			enum ds_qualifier qual)
@@ -539,6 +543,32 @@ int ds_get_pebs_index(struct task_struct *task, size_t *pos)
 {
 	return ds_get_index(task, pos, ds_pebs);
 }
+EXPORT_SYMBOL(ds_get_pebs_index);
+
+int ds_get_pebs_thres(struct task_struct *task, size_t *pos)
+{
+	struct ds_context *context;
+	unsigned long base, thr;
+	int error;
+
+	context = ds_get_context(task);
+	error = ds_validate_access(context, ds_pebs);
+	if (error < 0)
+		goto out;
+
+	base  = ds_get(context->ds, ds_pebs, ds_buffer_base);
+	thr = ds_get(context->ds, ds_pebs, ds_interrupt_threshold);
+
+	error = ((thr - base) / ds_cfg.sizeof_rec[ds_pebs]);
+	if (pos)
+		*pos = error;
+	return error;
+ out:
+	ds_put_context(context);
+	return error;
+}
+EXPORT_SYMBOL(ds_get_pebs_thres);
+
 
 static int ds_get_end(struct task_struct *task, size_t *pos,
 		      enum ds_qualifier qual)
@@ -572,6 +602,7 @@ int ds_get_pebs_end(struct task_struct *task, size_t *pos)
 {
 	return ds_get_end(task, pos, ds_pebs);
 }
+EXPORT_SYMBOL(ds_get_pebs_end);
 
 static int ds_access(struct task_struct *task, size_t index,
 		     const void **record, enum ds_qualifier qual)
@@ -747,6 +778,7 @@ int ds_reset_pebs(struct task_struct *task)
 {
 	return ds_reset_or_clear(task, ds_pebs, /* clear = */ 0);
 }
+EXPORT_SYMBOL(ds_reset_pebs);
 
 int ds_clear_bts(struct task_struct *task)
 {
@@ -796,6 +828,7 @@ int ds_set_pebs_reset(struct task_struct *task, u64 value)
 	ds_put_context(context);
 	return error;
 }
+EXPORT_SYMBOL(ds_set_pebs_reset);
 
 static const struct ds_configuration ds_cfg_var = {
 	.sizeof_ds    = sizeof(long) * 12,
@@ -804,10 +837,10 @@ static const struct ds_configuration ds_cfg_var = {
 	.sizeof_rec[ds_pebs]  = sizeof(long) * 10
 };
 static const struct ds_configuration ds_cfg_64 = {
-	.sizeof_ds    = 8 * 12,
+	.sizeof_ds    = 8 * 9,
 	.sizeof_field = 8,
 	.sizeof_rec[ds_bts]   = 8 * 3,
-	.sizeof_rec[ds_pebs]  = 8 * 10
+	.sizeof_rec[ds_pebs]  = 8 * 18
 };
 
 static inline void