Hello,
I like it generally (well I suggested it so...) but can you please
post a proper patch with SOB against cgroup/for-3.5 branch?
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5
Also, can you please cc fsnotify people so that they can go over the
new usage?
> @@ -179,6 +179,8 @@ struct cgroup {
> struct cgroup *parent; /* my parent */
> struct dentry __rcu *dentry; /* cgroup fs entry, RCU protected */
>
> + struct dentry *tasks_dentry; /* "tasks" dentry */
Urgh... not the prettiest but I suppose it's necessary. It will
probably be better to point to cfent instead.
> +static inline void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
No need to make it inline.
> +{
> + struct cgroupfs_root *root;
> + struct inode *d_inode;
> + struct cgroup *cgrp;
What are the locking rules?
> + for_each_active_root(root) {
> + cgrp = task_cgroup_from_root(tsk, root);
> + d_inode = cgrp->tasks_dentry->d_inode;
> +
> + fsnotify_parent(NULL, cgrp->tasks_dentry, mask);
> + fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
The interface is rather weird. It's called fsnotify_cgroup() and it
always generates the requested event on its tasks file?
> int cgroup_add_file(struct cgroup *cgrp,
> struct cgroup_subsys *subsys,
> - const struct cftype *cft)
> + const struct cftype *cft,
> + int tasks)
Ugh... this is ugly.
> {
> struct dentry *dir = cgrp->dentry;
> struct dentry *dentry;
> @@ -2629,6 +2649,12 @@ int cgroup_add_file(struct cgroup *cgrp,
> dput(dentry);
> } else
> error = PTR_ERR(dentry);
> +
> + if(tasks) {
^ missing space
> + pr_warn("%s(): cft name: %s\n", __func__, name);
Why pr_warn?
> + cgrp->tasks_dentry = dentry;
> + }
> +
> return error;
> }
> EXPORT_SYMBOL_GPL(cgroup_add_file);
> @@ -2640,7 +2666,7 @@ int cgroup_add_files(struct cgroup *cgrp,
> {
> int i, err;
> for (i = 0; i < count; i++) {
> - err = cgroup_add_file(cgrp, subsys, &cft[i]);
> + err = cgroup_add_file(cgrp, subsys, &cft[i], 0);
> if (err)
> return err;
> }
> @@ -3642,12 +3668,16 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
> /* First clear out any existing files */
> cgroup_clear_directory(cgrp->dentry);
>
> - err = cgroup_add_files(cgrp, NULL, files, ARRAY_SIZE(files));
> + err = cgroup_add_file(cgrp, NULL, files, 1);
> + if (err)
> + return err;
> +
> + err = cgroup_add_files(cgrp, NULL, files + 1, ARRAY_SIZE(files) - 1);
> if (err < 0)
> return err;
>
> if (cgrp == cgrp->top_cgroup) {
> - if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent)) < 0)
> + if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent, 0)) < 0)
> return err;
> }
Wouldn't it be better to make cgroup_add_file() return the created cft
and let the caller handle the tasks special case? Also, why use 1/0
for boolean values instead of true/false?
> @@ -4480,6 +4510,7 @@ static const struct file_operations proc_cgroupstats_operations = {
> */
> void cgroup_fork(struct task_struct *child)
> {
> +
Why the new line?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
