Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets.

[Lucian Adrian Grijincu]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/