Message-ID: <7c86c4470811190913u743706abgafff3b0f0e3559ec@mail.gmail.com>
Date:	Wed, 19 Nov 2008 18:13:11 +0100
From:	"stephane eranian" <eranian@...glemail.com>
To:	"Metzger, Markus T" <markus.t.metzger@...el.com>
Cc:	"Markus Metzger" <markus.t.metzger@...glemail.com>,
	"Ingo Molnar" <mingo@...e.hu>, "Andi Kleen" <andi@...stfloor.org>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: debugctl msr

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@...el.com> wrote:
>>-----Original Message-----
>>From: stephane eranian [mailto:eranian@...glemail.com]
>>Sent: Mittwoch, 19. November 2008 14:00
>>To: Metzger, Markus T
>>Cc: Markus Metzger; Ingo Molnar; Andi Kleen; Andrew Morton;
>>linux-kernel@...r.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.
>
>

Download attachment "ds.diff" of type "application/octet-stream" (4067 bytes)

Powered by blists - more mailing lists