lkml.org 
[lkml]   [2018]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist
    On Fri, Oct 05, 2018 at 07:46:28PM -0400, Theodore Y. Ts'o wrote:
    > On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
    > >
    > > Here are this week's rcu doc updates based on combing through whatisRCU and
    > > checklists. Hopefully you agree with them. I left several old _bh and _sched
    > > API references as is, since I don't think its a good idea to remove them till
    > > the APIs themselves are removed, however I did remove several of them as well
    > > (like in the first patch in this series) since I feel its better to "encourage"
    > > new users not to use the old API.
    >
    > Hi Joel,
    >
    > As it so happens, I just recently wrote my first RCU patch[1] (file
    > systems, especially on-disk data structures, generally tend not to be
    > good candidates for RCU semantics).
    >
    > [1] http://patchwork.ozlabs.org/patch/979779/

    Very cool!

    One question... In the following hunk:

    ------------------------------------------------------------------------

    @@ -5353,9 +5362,13 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
    #ifdef CONFIG_QUOTA
    sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
    for (i = 0; i < EXT4_MAXQUOTAS; i++) {
    - kfree(sbi->s_qf_names[i]);
    - sbi->s_qf_names[i] = old_opts.s_qf_names[i];
    + to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
    + &sb->s_umount);
    + rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
    }
    + for (i = 0; i < EXT4_MAXQUOTAS; i++)
    + kfree(to_free[i]);
    + synchronize_rcu();
    #endif
    kfree(orig_data);
    return err;
    ------------------------------------------------------------------------
    Shouldn't the synchronize_rcu() precede the loop doing the kfree()
    calls? Or am I missing something subtle?

    Otherwise, looks good! I was worried that seq_show_option() might
    sleep, but it looks like it is just putting characters into an
    array. If there is lingering concern, CONFIG_PROVE_LOCKING will
    usually catch that sort of thing.

    Thanx, Paul

    > So if you are working on improving RCU documentation, I thought I
    > would give two comments on the RCU docs from the perspective of a
    > developer trying to use RCU for the first time.
    >
    > * whatisRCU is great, but one the example in Section 3 uses
    > rcu_dereference_protected() without explaining it. Given that using
    > that function seems to be considered best practice, maybe a few more
    > words there would be in order? That function isn't mentioned in
    > rcu.txt either, BTW.
    >
    > * lockdep.txt *does* explain what rcu_dereference_protected() does,
    > but it doesn't really describe lockdep_is_held(). You can mostly
    > figure it out from context, but it wasn't obvious to me what locks
    > it could be used against, and in the case of a rw_semaphore, whether
    > it applied to shared as well as exclusive locks. That's a lockdep
    > abstraction, and not a RCU abstraction, but lockdep isn't
    > particularly well documented, so I ended up spending 20-30 minutes
    > or so looking at the lockdep implementation before I was sure it
    > actually worked the way I thought it was going to.
    >
    > Anyway, I was going to put submitting a patch to improve whatisRCU on
    > my (vastly over-long) TODO list, but when I saw your patch set, I
    > couldn't resist trying to see if I could fob it off on you. If you
    > don't think that's fair (and it probably isn't really), just let me
    > know, and I'll put it back on my todo list. :-)
    >
    > Cheers,
    >
    > - Ted
    >

    \
     
     \ /
      Last update: 2018-10-06 05:46    [W:9.503 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site