lkml.org 
[lkml]   [2013]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
    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 ;)

    > It's understandable that you want to use
    > abbreviations and I don't object against that, but please explain key
    > concepts and data structures when they first come up

    Well, this patch only move the definition with the comments, but:

    > - a very good place
    > to do that is in places where key structures are declared.
    >
    > I didn't find any high level description of the XOL code, one which makes
    > clear that how we manage these out of line execution areas:

    I have to agree, all these comments do not really help...

    > The one that comes closest is:
    >
    > * This area will be used for storing instructions for execution out of line.
    >
    > ... but that is a single sentence and deep inside the XOL code already.

    and even this comment should be probably moved up to the "struct xol_area",


    > Really, please make a better job of introducing other kernel hackers to
    > the code you are writing ...
    >
    > 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. I think it would
    be nice to have the high-level "uprobes design" doc in uprobetracer.txt, or

    > That will give a natural place to explain yourselves at the beginning of
    > the file.

    or even in the beginning of uprobes.c, I agree.

    Don't get me wrong, I am not volunteering ;) But at least I'll try to
    pay more attention to the comments when I change the code next time.

    Oleg.



    \
     
     \ /
      Last update: 2013-11-11 21:41    [W:2.366 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site