From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754061Ab2A3ACW (ORCPT ); Sun, 29 Jan 2012 19:02:22 -0500 Received: from mail-bk0-f46.google.com ([209.85.214.46]:65517 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754012Ab2A3ACU convert rfc822-to-8bit (ORCPT ); Sun, 29 Jan 2012 19:02:20 -0500 MIME-Version: 1.0 In-Reply-To: <1327639925-12920-24-git-send-email-ebiederm@xmission.com> References: <1327639925-12920-24-git-send-email-ebiederm@xmission.com> From: Lucian Adrian Grijincu Date: Mon, 30 Jan 2012 02:01:48 +0200 Message-ID: Subject: Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets. To: "Eric W. Biederman" Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org, Damien Millescamps Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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