lkml.org 
[lkml]   [2011]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 3.0-rc2-tip 4/22] 4: Uprobes: register/unregister probes.
    * Peter Zijlstra <peterz@infradead.org> [2011-06-10 01:03:26]:

    > On Tue, 2011-06-07 at 18:29 +0530, Srikar Dronamraju wrote:
    > > +/*
    > > + * There could be threads that have hit the breakpoint and are entering the
    > > + * notifier code and trying to acquire the uprobes_treelock. The thread
    > > + * calling delete_uprobe() that is removing the uprobe from the rb_tree can
    > > + * race with these threads and might acquire the uprobes_treelock compared
    > > + * to some of the breakpoint hit threads. In such a case, the breakpoint hit
    > > + * threads will not find the uprobe. Finding if a "trap" instruction was
    > > + * present at the interrupting address is racy. Hence provide some extra
    > > + * time (by way of synchronize_sched() for breakpoint hit threads to acquire
    > > + * the uprobes_treelock before the uprobe is removed from the rbtree.
    > > + */
    >
    > 'some' extra time doesn't really sound convincing to me. Either it is
    > sufficient to avoid the race or it is not. It reads to me like: we add a
    > delay so that the race mostly doesn't occur. Not good ;-)

    The extra time provided is sufficient to avoid the race. So will modify
    it to mean "sufficient" instead of "some".

    >
    > > +static void delete_uprobe(struct uprobe *uprobe)
    > > +{
    > > + unsigned long flags;
    > > +
    > > + synchronize_sched();
    > > + spin_lock_irqsave(&uprobes_treelock, flags);
    > > + rb_erase(&uprobe->rb_node, &uprobes_tree);
    > > + spin_unlock_irqrestore(&uprobes_treelock, flags);
    > > + iput(uprobe->inode);
    > > +}
    >
    > Also what are the uprobe lifetime rules here? Does it still exist after
    > this returns?
    >
    > The comment in del_consumer() that says: 'drop creation ref' worries me
    > and makes me thing that is the last reference around and the uprobe will
    > be freed right there, which clearly cannot happen since its not yet
    > removed from the RB-tree.
    >

    When del_consumer() is called in unregister_uprobe() it has atleast two
    (or more if the uprobe is hit) references. One at the creation time and
    the other thro find_uprobe() called in unregister_uprobe before
    del_consumer. So the reference lost in del_consumer is never the last
    reference. I added a commented this as creation reference so that the
    find_uprobe and the put_uprobe() before return would match.

    If the comment is confusing I can delete it or reword it as suggested by
    Steven Rostedt which is /* Have caller drop the creation ref */

    I would prefer to delete the comment.

    --
    Thanks and Regards
    Srikar


    \
     
     \ /
      Last update: 2011-06-16 07:37    [W:3.671 / U:0.668 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site