Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754335AbbKXQtf (ORCPT ); Tue, 24 Nov 2015 11:49:35 -0500 Received: from mail-yk0-f178.google.com ([209.85.160.178]:33913 "EHLO mail-yk0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753872AbbKXQtc (ORCPT ); Tue, 24 Nov 2015 11:49:32 -0500 Date: Tue, 24 Nov 2015 11:49:27 -0500 From: Tejun Heo To: serge@hallyn.com Cc: linux-kernel@vger.kernel.org, adityakali@google.com, linux-api@vger.kernel.org, containers@lists.linux-foundation.org, cgroups@vger.kernel.org, lxc-devel@lists.linuxcontainers.org, akpm@linux-foundation.org, ebiederm@xmission.com, Serge Hallyn Subject: Re: [PATCH 5/8] cgroup: introduce cgroup namespaces Message-ID: <20151124164927.GP17033@mtj.duckdns.org> References: <1447703505-29672-1-git-send-email-serge@hallyn.com> <1447703505-29672-6-git-send-email-serge@hallyn.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447703505-29672-6-git-send-email-serge@hallyn.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7560 Lines: 267 Hello, On Mon, Nov 16, 2015 at 01:51:42PM -0600, serge@hallyn.com wrote: > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > index 99096be..b3ce9d9 100644 > --- a/include/linux/cgroup.h > +++ b/include/linux/cgroup.h > @@ -17,6 +17,9 @@ > #include > #include > #include > +#include > +#include > +#include > > #include > > @@ -237,6 +240,10 @@ static inline bool cgroup_is_dead(const struct cgroup *cgrp) > return !(cgrp->self.flags & CSS_ONLINE); > } > > +static inline void css_get(struct cgroup_subsys_state *css); > +static inline void css_put(struct cgroup_subsys_state *css); > +static inline bool css_tryget(struct cgroup_subsys_state *css); Heh, what's going on here? > + > static inline void cgroup_get(struct cgroup *cgrp) > { > WARN_ON_ONCE(cgroup_is_dead(cgrp)); > @@ -284,9 +291,11 @@ static inline void cgroup_put(struct cgroup *cgrp) > ; \ > else > > -/* > - * Inline functions. > - */ > +extern char * __must_check cgroup_path_ns(struct cgroup_namespace *ns, > + struct cgroup *cgrp, char *buf, size_t buflen); > + > +extern char * __must_check cgroup_path(struct cgroup *cgrp, char *buf, > + size_t buflen); Please move them next to other prototypes and drop extern. > diff --git a/include/linux/cgroup_namespace.h b/include/linux/cgroup_namespace.h > new file mode 100644 > index 0000000..ed181c3 > --- /dev/null > +++ b/include/linux/cgroup_namespace.h > @@ -0,0 +1,46 @@ > +#ifndef _LINUX_CGROUP_NAMESPACE_H > +#define _LINUX_CGROUP_NAMESPACE_H > + > +#include > +#include > +#include > +#include > + > +struct css_set; Blank line here or linux/cgroup-defs.h can be included. > +struct cgroup_namespace { > + atomic_t count; > + struct ns_common ns; > + struct user_namespace *user_ns; > + struct css_set *root_cgrps; > +}; > + > +extern struct cgroup_namespace init_cgroup_ns; > + > +static inline struct cgroup_namespace *get_cgroup_ns( > + struct cgroup_namespace *ns) I personally prefer putting just the return type on a separate line when things get too long. static inline struct cgroup_namespace * get_cgroup_ns(struct cgroup_namespace *ns) > +{ > + if (ns) > + atomic_inc(&ns->count); > + return ns; > +} Ugh... if the function doesn't do anything about the return type, please make it a void function. We tried the above style with kobj and driver model and it ended up pretty horrible. > +#ifdef CONFIG_CGROUPS > +extern void free_cgroup_ns(struct cgroup_namespace *ns); > +extern struct cgroup_namespace *copy_cgroup_ns(unsigned long flags, > + struct user_namespace *user_ns, > + struct cgroup_namespace *old_ns); Please drop extern. > +#else /* CONFIG_CGROUP */ > +static inline void free_cgroup_ns(struct cgroup_namespace *ns) { } > +static inline struct cgroup_namespace *copy_cgroup_ns(unsigned long flags, > + struct user_namespace *user_ns, > + struct cgroup_namespace *old_ns) > +{ return old_ns; } > +#endif > + > +static inline void put_cgroup_ns(struct cgroup_namespace *ns) > +{ > + if (ns && atomic_dec_and_test(&ns->count)) > + free_cgroup_ns(ns); > +} > + > +#endif /* _LINUX_CGROUP_NAMESPACE_H */ I don't know. Does this warrant a separate file? > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index e972259..1d696de 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -57,6 +57,8 @@ > #include /* TODO: replace with more sophisticated array */ > #include > #include > +#include > +#include > > #include > > @@ -290,6 +292,15 @@ static bool cgroup_on_dfl(const struct cgroup *cgrp) > { > return cgrp->root == &cgrp_dfl_root; > } > +struct cgroup_namespace init_cgroup_ns = { > + .count = { > + .counter = 1, > + }, > + .user_ns = &init_user_ns, > + .ns.ops = &cgroupns_operations, > + .ns.inum = PROC_CGROUP_INIT_INO, > + .root_cgrps = &init_css_set, > +}; Can you please tab align the assignments? > @@ -2148,6 +2159,28 @@ static struct file_system_type cgroup_fs_type = { > .kill_sb = cgroup_kill_sb, > }; > > +char * __must_check cgroup_path_ns(struct cgroup_namespace *ns, > + struct cgroup *cgrp, char *buf, > + size_t buflen) Please align to the same column as the argument on the first line and make the optional @ns the last argument. > +{ > + if (ns) { > + struct cgroup *root; > + root = cset_cgroup_from_root(ns->root_cgrps, cgrp->root); > + return kernfs_path_from_node(root->kn, cgrp->kn, buf, > + buflen); > + } else { > + return kernfs_path(cgrp->kn, buf, buflen); > + } > +} > + > +char * __must_check cgroup_path(struct cgroup *cgrp, char *buf, > + size_t buflen) > +{ > + return cgroup_path_ns(current->nsproxy->cgroup_ns, cgrp, buf, > + buflen); Ditto with alignment. > diff --git a/kernel/cgroup_namespace.c b/kernel/cgroup_namespace.c > new file mode 100644 > index 0000000..ef20777 > --- /dev/null > +++ b/kernel/cgroup_namespace.c > @@ -0,0 +1,127 @@ > +/* > + * Copyright (C) 2014 Google Inc. > + * > + * Author: Aditya Kali (adityakali@google.com) > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation, version 2 of the License. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +const struct proc_ns_operations cgroupns_operations; > + > +static struct cgroup_namespace *alloc_cgroup_ns(void) > +{ > + struct cgroup_namespace *new_ns; > + int ret; > + > + new_ns = kzalloc(sizeof(struct cgroup_namespace), GFP_KERNEL); > + if (!new_ns) > + return ERR_PTR(-ENOMEM); > + ret = ns_alloc_inum(&new_ns->ns); > + if (ret) { > + kfree(new_ns); > + return ERR_PTR(ret); > + } > + atomic_set(&new_ns->count, 1); > + new_ns->ns.ops = &cgroupns_operations; > + return new_ns; > +} > + > +extern void put_css_set(struct css_set *cset); > +extern void get_css_set(struct css_set *cset); Heh, idk, so we're moving cgroup_get/put() to cgroup.h while redclaring css_set functions in this file? > +struct cgroup_namespace *copy_cgroup_ns(unsigned long flags, > + struct user_namespace *user_ns, > + struct cgroup_namespace *old_ns) > +{ > + struct cgroup_namespace *new_ns = NULL; > + struct css_set *cgrps = NULL; > + int err; > + > + BUG_ON(!old_ns); > + > + if (!(flags & CLONE_NEWCGROUP)) > + return get_cgroup_ns(old_ns); > + > + /* Allow only sysadmin to create cgroup namespace. */ > + err = -EPERM; > + if (!ns_capable(user_ns, CAP_SYS_ADMIN)) > + goto err_out; > + > + cgrps = task_css_set(current); > + get_css_set(cgrps); > + > + err = -ENOMEM; > + new_ns = alloc_cgroup_ns(); > + if (!new_ns) > + goto err_out; > + > + new_ns->user_ns = get_user_ns(user_ns); > + new_ns->root_cgrps = cgrps; Let's name it ->root_cset. The data structures involved are already really confusing. No need to add more to it. Thanks. -- tejun -- 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/