Re: [PATCH 4/6] hw-breakpoints: Rewrite the hw-breakpoints layer on top of perf events

[Frederic Weisbecker <fweisbec@gmail.com>]

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 :)