Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

From: Ingo Molnar
Date: Mon Nov 11 2013 - 16:03:40 EST



* Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > > +++ b/kernel/events/uprobes.c
> > > @@ -86,6 +86,25 @@ struct return_instance {
> > > };
> > >
> > > /*
> > > + * On a breakpoint hit, thread contests for a slot. It frees the
> > > + * slot after singlestep. Currently a fixed number of slots are
> > > + * allocated.
> > > + */
> > > +struct xol_area {
> >
> > So, my main complaint about the uprobes code isn't functional but
> > documentational, similar to what I outlined a few days ago: what this
> > comment does not explain is exactly what a 'XOL area' is.
> >
> > You guys are changing code that reads like gobbledygook to people
> > reading it for the first time.
>
> Not that I am trying to defense uprobes, but this is equally true for
> any piece of kernel code, at least to me ;)

I'm really not suggesting to do overly much - only for some minimal blurb
like the scheduler has in most places:

/*
* This is the main, per-CPU runqueue data structure.
*
* Locking rule: those places that want to lock multiple runqueues
* (such as the load balancing or the thread migration code), lock
* acquire operations must be ordered by ascending &runqueue.
*/
struct rq {
/* runqueue lock: */
raw_spinlock_t lock;

But the apparent assumption that the reader knows what 'XOL' means
triggered my suggest :-)

> > Maybe even split the XOL code out into kernel/events/uprobes_xol.c or
> > so?
>
> I do not really think a separate uprobes_xol.c makes sense. [...]

Ok - it's your call really.

> [...] I think it would be nice to have the high-level "uprobes design"
> doc in uprobetracer.txt, or

Even better if the best parts are integrated into the source code!

Thanks,

Ingo
--
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/