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

[Ingo Molnar <mingo@kernel.org>]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> 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