Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
> Just in case let me repeat, I can be easily wrong because I forgot
> how this series actually look and I don't have the patches now ;)
> 
> On 11/09, Frederic Weisbecker wrote:
> >
> > On Sat, Nov 09, 2013 at 04:11:56PM +0100, Oleg Nesterov wrote:
> > > On 11/08, Frederic Weisbecker wrote:
> > > >
> > > >
> > > > Does this feature only work on data breakpoint or is instruction breakpoint
> > > > address range supported as well?
> > >
> > > IIRC, execute range is supported as well.
> > >
> > > But. I can't look at the code now, but iirc this can't really work until
> > > we fix the (already discussed) problems with bp_len && X86_BREAKPOINT_LEN_X.
> > > IOW, we should not blame these patches if it doesn't work.
> >
> > Yeah, don't worry I don't plan to push back these patches for the sake of that bug,
> > that would be definetly unfair, especially as I introduced that issue :)
> >
> > And the patchset looks good overall, except for a few details but it's mostly ok,
> 
> OK,
> 
> > I just would like to fix that issue along the way. It would be really nice if we can
> > avoid having a mask _and_ a len for breakpoints.
> 
> Up to you and Suravee, but can't we cleanup this later?
> 
> This series was updated many times to address a lot of (sometimes
> contradictory) complaints.

Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.

I can to the fixup myself BTW, no problem with that, and I don't want to hold up this patchset,
but we just need to agree on a proper solution.
 
> > I mean, that doesn't look right to me,
> > it's two units basically measuring the same thing, so that's asking for conflicting troubles.
> 
> Yes. And we can kill either _len or _mask, not sure what would be more
> clean.
> 
> At least with the current implementation (again, iirc) mask == len -1.
> Although amd supports the more generic masks (but I can't recall the
> details).

Right. I think len is probably more powerful. Unless the mask could be used to define
multiple ranges of breakpoints, not sure it's ever going to be used that way though. But we
can still extend the interface if we need a mask later.

> 
> > I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
> > initial purpose of it) without breaking the tools.
> 
> Confused... user-space still uses len to express the range? Just
> the kernel "switches" to mask if len > 8 ?

Right but what if we want breakpoints having a size below 8? Like break on instructions
from 0x1000 to 0x1008 ?

Or should we ignore range instruction breakpoints when len < 8?