lkml.org 
[lkml]   [2009]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/6] hw-breakpoints: Rewrite the hw-breakpoints layer on top of perf events
On Thu, Nov 05, 2009 at 09:04:04PM +0530, K.Prasad wrote:
> On Tue, Nov 03, 2009 at 08:11:12PM +0100, Frederic Weisbecker wrote:
> [snipped]
> >
> > /* Available HW breakpoint length encodings */
> > -#define HW_BREAKPOINT_LEN_1 0x40
> > -#define HW_BREAKPOINT_LEN_2 0x44
> > -#define HW_BREAKPOINT_LEN_4 0x4c
> > -#define HW_BREAKPOINT_LEN_EXECUTE 0x40
> > +#define X86_BREAKPOINT_LEN_1 0x40
> > +#define X86_BREAKPOINT_LEN_2 0x44
> > +#define X86_BREAKPOINT_LEN_4 0x4c
> > +#define X86_BREAKPOINT_LEN_EXECUTE 0x40
> >
>
> It had previously been suggested http://lkml.org/lkml/2009/5/28/554
> that users be allowed to specify the lengths in numerals. Despite having
> some divergent views initially, I see that it would help minimise the
> amount of code required to request a breakpoint if numerals (such as 1,
> 2, 4 and 8 for x86_64) are allowed.
>
> The conversion to encoded values can happen later inside the
> bkpt-specific code.



That's what I did, I've redefined them in linux/hw_breakpoint.h:

#define HW_BREAKPOINT_LEN_1 1
#define HW_BREAKPOINT_LEN_2 2
#define HW_BREAKPOINT_LEN_4 4

And the arch interpret that using its own corresponding values.



> > --- a/include/asm-generic/hw_breakpoint.h
> > +++ /dev/null
>
> Can you split this patch into fine granular ones? It is very difficult
> to review the changes this way.



Sure, I personally don't like either this big monolithic patch, but
it is hard/impossible to split it as we change the whole base of a
subsystem inside.

But this header moving has been done in the v2 and I thought git-format-patch
would detect the rename but the file has probably too much changed.

I'll do another iteration that split up this part.


> > diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
> > new file mode 100644
> > index 0000000..7eba9b9
> > --- /dev/null
> > +++ b/include/linux/hw_breakpoint.h
>
> Have you clubbed file renaming along with changes inside the file?
> Again, it'd be good to have them in separate patches for easy review.


There have been this rename only. But I'll split up this part.



> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +extern struct perf_event *
> > +register_user_hw_breakpoint(unsigned long addr,
> > + int len,
> > + int type,
> > + perf_callback_t triggered,
> > + struct task_struct *tsk,
> > + bool active);
> > +
>
> I don't understand the benefit behind bringing these parameters into the
> interfaces' prototype. Besides they will make addition of new attributes
> (if needed later) quite cumbersome. Given that these values are
> eventually copied into members of perf_event_attr, I'd suggest that they
> accept a pointer to an instance of the structure.



Yeah, that's a bit intended as a temporary thing. The preffered
way for that would be to pass a pointer to a perf_event_attr
structure.

I plan to do this change incrementally, once we have defined
breakpoints attributes generic enough to support most archs
possibilities.



> > +/* FIXME: only change from the attr, and don't unregister */
> > +extern struct perf_event *
> > +modify_user_hw_breakpoint(struct perf_event *bp,
> > + unsigned long addr,
> > + int len,
> > + int type,
> > + perf_callback_t triggered,
> > + struct task_struct *tsk,
> > + bool active);
> > +
> > +/*
> > + * Kernel breakpoints are not associated with any particular thread.
> > + */
> > +extern struct perf_event *
> > +register_wide_hw_breakpoint_cpu(unsigned long addr,
> > + int len,
> > + int type,
> > + perf_callback_t triggered,
> > + int cpu,
>
> Can't it be cpumask_t instead of int cpu? Given that per-cpu breakpoints
> will be implemented, it should be very different to implement them for a
> subset of cpus too.



I can't figure out any usecase where we want to only bind to,
say, cpu 1 and 3 or any kind of such strange combination.

Either we want a wide breakpoint, or we want to profile
a single cpu, but I don't imagine we need a middle case.



> > -static DEFINE_SPINLOCK(hw_breakpoint_lock);
>
> Wouldn't you want to hold this lock while checking for system-wide
> availability of debug registers (during rollbacks) to avoid contenders
> from checking for the same simultaneously?


If we want to lock such path, we probably more likely want a mutex.
Registering a breakpoint is not a fastpath and also perf does
some sleepable things while creating a counter.

The check to register constraints, which is part of this path,
is itself a mutex.

But we'll probably need something NMI safe in the future so
that it can be used without any problem by kgdb.



> <snipped>
>
> > -int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > +struct perf_event **
> > +register_wide_hw_breakpoint(unsigned long addr,
> > + int len,
> > + int type,
> > + perf_callback_t triggered,
> > + bool active)
> > {
> > - int rc;
> > + struct perf_event **cpu_events, **pevent, *bp;
> > + long err;
> > + int cpu;
> > +
> > + cpu_events = alloc_percpu(typeof(*cpu_events));
> > + if (!cpu_events)
> > + return ERR_PTR(-ENOMEM);
> >
> > - rc = arch_validate_hwbkpt_settings(bp, NULL);
> > - if (rc)
> > - return rc;
> > + for_each_possible_cpu(cpu) {
> > + pevent = per_cpu_ptr(cpu_events, cpu);
> > + bp = register_kernel_hw_breakpoint_cpu(addr, len, type,
> > + triggered, cpu, active);
> >
>
> I'm assuming that there'd be an implementation for system-wide
> perf-events (and hence breakpoints) in the forthcoming version(s) of
> this patchset.


If that becomes a necessary feature, then yeah.


> Have you tested these changes from perf-events' user-space command?
> Would you like to re-use the patches from here:
> http://lkml.org/lkml/2009/10/29/304 to test them?


Yeah, I have planned to reuse your patches for the perf subcommand
support :)



\
 
 \ /
  Last update: 2009-11-05 22:09    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site