Re: [patch 02/11] x86 architecture implementation of HardwareBreakpoint interfaces

[Alan Stern]

On Tue, 10 Mar 2009, Ingo Molnar wrote:

> > > why this redirection, why dont just use the structure as-is? 
> > > If there's any arch weirdness then that arch should have 
> > > arch-special accessors - not the generic code.
> > 
> > These _are_ the arch-specific accessors.  Look at the 
> > filename: arch/x86/include/asm/hw_breakpoint.h.
> 
> I very well know which file this is, you need to read my reply 
> again.
> 
> These are very generic-sounding fields and they should not be 
> hidden via pointless wrappers by the generic code. Dont let the 
> tail wag the dog. If there's architecture weirdness that does 
> not fit the generic code, then _that_ architecture should have 
> the ugliness - not the generic code. (note that these accessors 
> are used by the generic code so the uglification spreads there)

Hm.  I haven't been keeping careful track of all the updates Prasad has
been making.  In my fairly-old copy of the hw-breakpoint work, the
accessors are _not_ used by the generic code.  They are there for
future users of the API, not for internal use by the API itself.  Is 
there something I'm missing?

I have the feeling that this doesn't really address your comment, but
I'm not sure if that's because I don't understand your point or you
don't understand mine...

> These are very generic-sounding fields ...

Would you be happier if the field names were changed to be less 
generic-sounding?

> > > > + int num_installed; /* Number of installed bps */ + 
> > > > unsigned gennum; /* update-generation number */
> > > 
> > > i suspect the gennum we can get rid of if we get rid of the 
> > > notion of priorities, right?
> > 
> > No.  gennum has nothing to do with priorities.
> 
> Well it's introduced because we have a priority-sorted list of 
> breakpoints not an array.

More generally, it's there because kernel & userspace breakpoints can
be installed and uninstalled while a task is running -- and yes, this
is partially because breakpoints are prioritized.  (Although it's worth
pointing out that even your suggestion of always prioritizing kernel
breakpoints above userspace breakpoints would have the same effect.)  
However the fact that the breakpoints are stored in a list rather than
an array doesn't seem to be relevant.

> A list needs to be maintained and when 
> updated it's reloaded.

The same is true of an array.

> I was thinking about possibly getting rid 
> of that list complication and go back to the simpler array. But 
> it's hard because the lifetime of a kernel space breakpoint 
> spans context-switches so there has to be separation.

Yes, kernel breakpoints have to be kept separate from userspace 
breakpoints.  But even if you focus just on userspace breakpoints, you 
still need to use a list because debuggers can try to register an 
arbitrarily large number of breakpoints.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/