lkml.org 
[lkml]   [2012]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets.
    Very nice way to handle netns specific files with links between sets!

    You did a much better job than I did at dealing with them.

    It took me a while to understand how the code works. I'll try to write
    something for Documentation/ because the inner workings are a bit
    intertwined.

    A few comments bellow.

    On Fri, Jan 27, 2012 at 6:52 AM, Eric W. Biederman
    <ebiederm@xmission.com> wrote:
    > Piecing together directories by looking first in one directory
    > tree, than in another directory tree and finally in a third

    than -> then

    > directory tree makes it hard to verify that some directory
    > entries are not multiply defined and makes it hard to create
    > efficient implementations the sysctl filesystem.
    >
    > Replace the sysctl wide list of roots with autogenerated
    > links from the core sysctl directory tree to the other
    > sysctl directory trees.
    >
    > This simplifies sysctl directory reading and lookups as now
    > only entries in a single sysctl directory tree need to be
    > considered.
    >
    > Benchmark before:
    >    make-dummies 0 999 -> 0.44s
    >    rmmod dummy        -> 0.065s
    >    make-dummies 0 9999 -> 1m36s
    >    rmmod dummy         -> 0.4s
    >
    > Benchmark after:
    >    make-dummies 0 999 -> 0.63s
    >    rmmod dummy        -> 0.12s
    >    make-dummies 0 9999 -> 2m35s
    >    rmmod dummy         -> 18s
    >
    > The slowdown is caused by the lookups used in insert_headers

    insert_headers -> insert_header

    > and put_links to see if we need to add links or remove links.




    >  void register_sysctl_root(struct ctl_table_root *root)
    >  {
    > -       spin_lock(&sysctl_lock);
    > -       list_add_tail(&root->root_list, &sysctl_table_root.root_list);
    > -       spin_unlock(&sysctl_lock);
    >  }


    This function is empty. Can be deleted (there are two callers in
    net/sysctl_net.c.


    > @@ -400,6 +373,7 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
    >        struct inode *inode;
    >        struct dentry *err = ERR_PTR(-ENOENT);
    >        struct ctl_dir *ctl_dir;
    > +       int ret;
    >
    >        if (IS_ERR(head))
    >                return ERR_CAST(head);
    > @@ -410,6 +384,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
    >        if (!p)
    >                goto out;
    >


    "sysctl_follow_link" implies that it will follow the link. I would
    pull out the check whether the header is a link or not. This wouldn't
    save much (a function call), but it would make the code easier to
    read:

    /* Get out quickly if not a link */
    if (S_ISLNK(p->mode)) {
    ret = sysctl_follow_link(&h, &p, current->nsproxy);
    err = ERR_PTR(ret);
    if (ret)
    goto out;
    }



    > +       ret = sysctl_follow_link(&h, &p, current->nsproxy);
    > +       err = ERR_PTR(ret);
    > +       if (ret)
    > +               goto out;
    > +
    >        err = ERR_PTR(-ENOMEM);
    >        inode = proc_sys_make_inode(dir->i_sb, h ? h : head, p);
    >        if (h)
    > @@ -547,6 +526,25 @@ static int proc_sys_fill_cache(struct file *filp, void *dirent,
    >        return !!filldir(dirent, qname.name, qname.len, filp->f_pos, ino, type);
    >  }
    >
    > +static int proc_sys_link_fill_cache(struct file *filp, void *dirent,
    > +                                   filldir_t filldir,
    > +                                   struct ctl_table_header *head,
    > +                                   struct ctl_table *table)
    > +{
    > +       int err, ret = 0;
    > +       head = sysctl_head_grab(head);
    > +
    > +       /* It is not an error if we can not follow the link ignore it */
    > +       err = sysctl_follow_link(&head, &table, current->nsproxy);

    similar

    > +       if (err)
    > +               goto out;
    > +
    > +       ret = proc_sys_fill_cache(filp, dirent, filldir, head, table);
    > +out:
    > +       sysctl_head_finish(head);
    > +       return ret;
    > +}
    > +




    > -static struct ctl_dir *get_subdir(struct ctl_table_set *set,
    > -       struct ctl_dir *dir, const char *name, int namelen)
    > +static struct ctl_dir *get_subdir(struct ctl_dir *dir,
    > +                                 const char *name, int namelen)
    >  {
    > +       struct ctl_table_set *set = dir->header.set;
    >        struct ctl_dir *subdir, *new = NULL;
    >        spin_lock(&sysctl_lock);
    > -       subdir = find_subdir(dir->header.set, dir, name, namelen);
    > -       if (!IS_ERR(subdir))
    > -               goto found;
    > -       if ((PTR_ERR(subdir) == -ENOENT) && set != dir->header.set)
    > -               subdir = find_subdir(set, dir, name, namelen);
    > +       subdir = find_subdir(dir, name, namelen);
    >        if (!IS_ERR(subdir))
    >                goto found;
    >        if (PTR_ERR(subdir) != -ENOENT)
    > @@ -817,13 +815,14 @@ static struct ctl_dir *get_subdir(struct ctl_table_set *set,
    >        if (!new)
    >                goto failed;
    >
    > -       subdir = find_subdir(set, dir, name, namelen);
    > +       subdir = find_subdir(dir, name, namelen);
    >        if (!IS_ERR(subdir))
    >                goto found;
    >        if (PTR_ERR(subdir) != -ENOENT)
    >                goto failed;

    I think you're returning the wrong error here. If we got to this point
    then subdir == ERR_PTR(-ENOENT).
    We want to create a new dir here even if one doesn't exist.
    So if we have an error in insert_header() we don't return that error,
    but ENOENT.

    >
    > -       insert_header(dir, &new->header);
    > +       if (insert_header(dir, &new->header))
    > +               goto failed;
    >        subdir = new;

    Something like this would fix it:

    if (subdir = insert_header(dir, &new->header))


    >  found:
    >        subdir->header.nreg++;
    > @@ -841,6 +840,57 @@ failed:
    >        return subdir;
    >  }
    >




    --
     .
    ..: Lucian
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-01-30 01:05    [W:0.034 / U:0.464 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site