Messages in this thread | | | Date | Wed, 19 Nov 2008 18:13:11 +0100 | From | "stephane eranian" <> | 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@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. > > [unhandled content-type:application/octet-stream] | |