[PATCH 0/8] namespaces: Introduce generic refcount

[PATCH 0/8] namespaces: Introduce generic refcount

[Kirill Tkhai]

Every namespace type has its own counter. Some of them are
of refcount_t, some of them are of kref.

This patchset introduces generic ns_common::count for any
type of namespaces instead of them.

---

Kirill Tkhai (8):
      ns: Add common refcount into ns_common add use it as counter for net_ns
      uts: Use generic ns_common::count
      ipc: Use generic ns_common::count
      pid: Use generic ns_common::count
      user: Use generic ns_common::count
      mnt: Use generic ns_common::count
      cgroup: Use generic ns_common::count
      time: Use generic ns_common::count


 fs/mount.h                     |    3 +--
 fs/namespace.c                 |    4 ++--
 include/linux/cgroup.h         |    5 ++---
 include/linux/ipc_namespace.h  |    3 +--
 include/linux/ns_common.h      |    3 +++
 include/linux/pid_namespace.h  |    4 +---
 include/linux/time_namespace.h |    9 ++++-----
 include/linux/user_namespace.h |    5 ++---
 include/linux/utsname.h        |    9 ++++-----
 include/net/net_namespace.h    |   11 ++++-------
 init/version.c                 |    2 +-
 ipc/msgutil.c                  |    2 +-
 ipc/namespace.c                |    4 ++--
 kernel/cgroup/cgroup.c         |    2 +-
 kernel/cgroup/namespace.c      |    2 +-
 kernel/pid.c                   |    2 +-
 kernel/pid_namespace.c         |   13 +++----------
 kernel/time/namespace.c        |    9 +++------
 kernel/user.c                  |    2 +-
 kernel/user_namespace.c        |    4 ++--
 kernel/utsname.c               |    7 ++-----
 net/core/net-sysfs.c           |    6 +++---
 net/core/net_namespace.c       |    6 +++---
 net/ipv4/inet_timewait_sock.c  |    4 ++--
 net/ipv4/tcp_metrics.c         |    2 +-
 25 files changed, 51 insertions(+), 72 deletions(-)

--
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

[PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

[Kirill Tkhai]

Currently, every type of namespaces has its own counter,
which is stored in ns-specific part. Say, @net has
struct net::count, @pid has struct pid_namespace::kref, etc.

This patchset introduces unified counter for all types
of namespaces, and converts net namespace to use it first.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/ns_common.h     |    3 +++
 include/net/net_namespace.h   |   11 ++++-------
 net/core/net-sysfs.c          |    6 +++---
 net/core/net_namespace.c      |    6 +++---
 net/ipv4/inet_timewait_sock.c |    4 ++--
 net/ipv4/tcp_metrics.c        |    2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/ns_common.h b/include/linux/ns_common.h
index 5fbc4000358f..0f1d024bd958 100644
--- a/include/linux/ns_common.h
+++ b/include/linux/ns_common.h
@@ -2,12 +2,15 @@
 #ifndef _LINUX_NS_COMMON_H
 #define _LINUX_NS_COMMON_H
 
+#include <linux/refcount.h>
+
 struct proc_ns_operations;
 
 struct ns_common {
 	atomic_long_t stashed;
 	const struct proc_ns_operations *ops;
 	unsigned int inum;
+	refcount_t count;
 };
 
 #endif
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 2ee5901bec7a..cb4b33d7834b 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -60,9 +60,6 @@ struct net {
 	refcount_t		passive;	/* To decide when the network
 						 * namespace should be freed.
 						 */
-	refcount_t		count;		/* To decided when the network
-						 *  namespace should be shut down.
-						 */
 	spinlock_t		rules_mod_lock;
 
 	unsigned int		dev_unreg_count;
@@ -245,7 +242,7 @@ void __put_net(struct net *net);
 
 static inline struct net *get_net(struct net *net)
 {
-	refcount_inc(&net->count);
+	refcount_inc(&net->ns.count);
 	return net;
 }
 
@@ -256,14 +253,14 @@ static inline struct net *maybe_get_net(struct net *net)
 	 * exists.  If the reference count is zero this
 	 * function fails and returns NULL.
 	 */
-	if (!refcount_inc_not_zero(&net->count))
+	if (!refcount_inc_not_zero(&net->ns.count))
 		net = NULL;
 	return net;
 }
 
 static inline void put_net(struct net *net)
 {
-	if (refcount_dec_and_test(&net->count))
+	if (refcount_dec_and_test(&net->ns.count))
 		__put_net(net);
 }
 
@@ -275,7 +272,7 @@ int net_eq(const struct net *net1, const struct net *net2)
 
 static inline int check_net(const struct net *net)
 {
-	return refcount_read(&net->count) != 0;
+	return refcount_read(&net->ns.count) != 0;
 }
 
 void net_drop_ns(void *);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 9de33b594ff2..655a88b0071c 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1025,7 +1025,7 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 	while (--i >= new_num) {
 		struct kobject *kobj = &dev->_rx[i].kobj;
 
-		if (!refcount_read(&dev_net(dev)->count))
+		if (!refcount_read(&dev_net(dev)->ns.count))
 			kobj->uevent_suppress = 1;
 		if (dev->sysfs_rx_queue_group)
 			sysfs_remove_group(kobj, dev->sysfs_rx_queue_group);
@@ -1603,7 +1603,7 @@ netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 	while (--i >= new_num) {
 		struct netdev_queue *queue = dev->_tx + i;
 
-		if (!refcount_read(&dev_net(dev)->count))
+		if (!refcount_read(&dev_net(dev)->ns.count))
 			queue->kobj.uevent_suppress = 1;
 #ifdef CONFIG_BQL
 		sysfs_remove_group(&queue->kobj, &dql_group);
@@ -1850,7 +1850,7 @@ void netdev_unregister_kobject(struct net_device *ndev)
 {
 	struct device *dev = &ndev->dev;
 
-	if (!refcount_read(&dev_net(ndev)->count))
+	if (!refcount_read(&dev_net(ndev)->ns.count))
 		dev_set_uevent_suppress(dev, 1);
 
 	kobject_get(&dev->kobj);
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index dcd61aca343e..5f658cbedd34 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -44,7 +44,7 @@ static struct key_tag init_net_key_domain = { .usage = REFCOUNT_INIT(1) };
 #endif
 
 struct net init_net = {
-	.count		= REFCOUNT_INIT(1),
+	.ns.count	= REFCOUNT_INIT(1),
 	.dev_base_head	= LIST_HEAD_INIT(init_net.dev_base_head),
 #ifdef CONFIG_KEYS
 	.key_domain	= &init_net_key_domain,
@@ -248,7 +248,7 @@ int peernet2id_alloc(struct net *net, struct net *peer, gfp_t gfp)
 {
 	int id;
 
-	if (refcount_read(&net->count) == 0)
+	if (refcount_read(&net->ns.count) == 0)
 		return NETNSA_NSID_NOT_ASSIGNED;
 
 	spin_lock(&net->nsid_lock);
@@ -328,7 +328,7 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
 	int error = 0;
 	LIST_HEAD(net_exit_list);
 
-	refcount_set(&net->count, 1);
+	refcount_set(&net->ns.count, 1);
 	refcount_set(&net->passive, 1);
 	get_random_bytes(&net->hash_mix, sizeof(u32));
 	net->dev_base_seq = 1;
diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index c411c87ae865..437afe392e66 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -272,14 +272,14 @@ void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family)
 				continue;
 			tw = inet_twsk(sk);
 			if ((tw->tw_family != family) ||
-				refcount_read(&twsk_net(tw)->count))
+				refcount_read(&twsk_net(tw)->ns.count))
 				continue;
 
 			if (unlikely(!refcount_inc_not_zero(&tw->tw_refcnt)))
 				continue;
 
 			if (unlikely((tw->tw_family != family) ||
-				     refcount_read(&twsk_net(tw)->count))) {
+				     refcount_read(&twsk_net(tw)->ns.count))) {
 				inet_twsk_put(tw);
 				goto restart;
 			}
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 279db8822439..39710c417565 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -887,7 +887,7 @@ static void tcp_metrics_flush_all(struct net *net)
 		pp = &hb->chain;
 		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 			match = net ? net_eq(tm_net(tm), net) :
-				!refcount_read(&tm_net(tm)->count);
+				!refcount_read(&tm_net(tm)->ns.count);
 			if (match) {
 				*pp = tm->tcpm_next;
 				kfree_rcu(tm, rcu_head);

Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

[Eric W. Biederman]

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> Currently, every type of namespaces has its own counter,
> which is stored in ns-specific part. Say, @net has
> struct net::count, @pid has struct pid_namespace::kref, etc.
>
> This patchset introduces unified counter for all types
> of namespaces, and converts net namespace to use it first.

And the other refcounts on struct net?

How do they play into what you are trying to do?

For the lack of an explanation.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>


> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  include/linux/ns_common.h     |    3 +++
>  include/net/net_namespace.h   |   11 ++++-------
>  net/core/net-sysfs.c          |    6 +++---
>  net/core/net_namespace.c      |    6 +++---
>  net/ipv4/inet_timewait_sock.c |    4 ++--
>  net/ipv4/tcp_metrics.c        |    2 +-
>  6 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/ns_common.h b/include/linux/ns_common.h
> index 5fbc4000358f..0f1d024bd958 100644
> --- a/include/linux/ns_common.h
> +++ b/include/linux/ns_common.h
> @@ -2,12 +2,15 @@
>  #ifndef _LINUX_NS_COMMON_H
>  #define _LINUX_NS_COMMON_H
>  
> +#include <linux/refcount.h>
> +
>  struct proc_ns_operations;
>  
>  struct ns_common {
>  	atomic_long_t stashed;
>  	const struct proc_ns_operations *ops;
>  	unsigned int inum;
> +	refcount_t count;
>  };
>  
>  #endif
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 2ee5901bec7a..cb4b33d7834b 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -60,9 +60,6 @@ struct net {
>  	refcount_t		passive;	/* To decide when the network
>  						 * namespace should be freed.
>  						 */
> -	refcount_t		count;		/* To decided when the network
> -						 *  namespace should be shut down.
> -						 */
>  	spinlock_t		rules_mod_lock;
>  
>  	unsigned int		dev_unreg_count;
> @@ -245,7 +242,7 @@ void __put_net(struct net *net);
>  
>  static inline struct net *get_net(struct net *net)
>  {
> -	refcount_inc(&net->count);
> +	refcount_inc(&net->ns.count);
>  	return net;
>  }
>  
> @@ -256,14 +253,14 @@ static inline struct net *maybe_get_net(struct net *net)
>  	 * exists.  If the reference count is zero this
>  	 * function fails and returns NULL.
>  	 */
> -	if (!refcount_inc_not_zero(&net->count))
> +	if (!refcount_inc_not_zero(&net->ns.count))
>  		net = NULL;
>  	return net;
>  }
>  
>  static inline void put_net(struct net *net)
>  {
> -	if (refcount_dec_and_test(&net->count))
> +	if (refcount_dec_and_test(&net->ns.count))
>  		__put_net(net);
>  }
>  
> @@ -275,7 +272,7 @@ int net_eq(const struct net *net1, const struct net *net2)
>  
>  static inline int check_net(const struct net *net)
>  {
> -	return refcount_read(&net->count) != 0;
> +	return refcount_read(&net->ns.count) != 0;
>  }
>  
>  void net_drop_ns(void *);
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 9de33b594ff2..655a88b0071c 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1025,7 +1025,7 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
>  	while (--i >= new_num) {
>  		struct kobject *kobj = &dev->_rx[i].kobj;
>  
> -		if (!refcount_read(&dev_net(dev)->count))
> +		if (!refcount_read(&dev_net(dev)->ns.count))
>  			kobj->uevent_suppress = 1;
>  		if (dev->sysfs_rx_queue_group)
>  			sysfs_remove_group(kobj, dev->sysfs_rx_queue_group);
> @@ -1603,7 +1603,7 @@ netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
>  	while (--i >= new_num) {
>  		struct netdev_queue *queue = dev->_tx + i;
>  
> -		if (!refcount_read(&dev_net(dev)->count))
> +		if (!refcount_read(&dev_net(dev)->ns.count))
>  			queue->kobj.uevent_suppress = 1;
>  #ifdef CONFIG_BQL
>  		sysfs_remove_group(&queue->kobj, &dql_group);
> @@ -1850,7 +1850,7 @@ void netdev_unregister_kobject(struct net_device *ndev)
>  {
>  	struct device *dev = &ndev->dev;
>  
> -	if (!refcount_read(&dev_net(ndev)->count))
> +	if (!refcount_read(&dev_net(ndev)->ns.count))
>  		dev_set_uevent_suppress(dev, 1);
>  
>  	kobject_get(&dev->kobj);
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index dcd61aca343e..5f658cbedd34 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -44,7 +44,7 @@ static struct key_tag init_net_key_domain = { .usage = REFCOUNT_INIT(1) };
>  #endif
>  
>  struct net init_net = {
> -	.count		= REFCOUNT_INIT(1),
> +	.ns.count	= REFCOUNT_INIT(1),
>  	.dev_base_head	= LIST_HEAD_INIT(init_net.dev_base_head),
>  #ifdef CONFIG_KEYS
>  	.key_domain	= &init_net_key_domain,
> @@ -248,7 +248,7 @@ int peernet2id_alloc(struct net *net, struct net *peer, gfp_t gfp)
>  {
>  	int id;
>  
> -	if (refcount_read(&net->count) == 0)
> +	if (refcount_read(&net->ns.count) == 0)
>  		return NETNSA_NSID_NOT_ASSIGNED;
>  
>  	spin_lock(&net->nsid_lock);
> @@ -328,7 +328,7 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>  	int error = 0;
>  	LIST_HEAD(net_exit_list);
>  
> -	refcount_set(&net->count, 1);
> +	refcount_set(&net->ns.count, 1);
>  	refcount_set(&net->passive, 1);
>  	get_random_bytes(&net->hash_mix, sizeof(u32));
>  	net->dev_base_seq = 1;
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index c411c87ae865..437afe392e66 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -272,14 +272,14 @@ void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family)
>  				continue;
>  			tw = inet_twsk(sk);
>  			if ((tw->tw_family != family) ||
> -				refcount_read(&twsk_net(tw)->count))
> +				refcount_read(&twsk_net(tw)->ns.count))
>  				continue;
>  
>  			if (unlikely(!refcount_inc_not_zero(&tw->tw_refcnt)))
>  				continue;
>  
>  			if (unlikely((tw->tw_family != family) ||
> -				     refcount_read(&twsk_net(tw)->count))) {
> +				     refcount_read(&twsk_net(tw)->ns.count))) {
>  				inet_twsk_put(tw);
>  				goto restart;
>  			}
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 279db8822439..39710c417565 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -887,7 +887,7 @@ static void tcp_metrics_flush_all(struct net *net)
>  		pp = &hb->chain;
>  		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
>  			match = net ? net_eq(tm_net(tm), net) :
> -				!refcount_read(&tm_net(tm)->count);
> +				!refcount_read(&tm_net(tm)->ns.count);
>  			if (match) {
>  				*pp = tm->tcpm_next;
>  				kfree_rcu(tm, rcu_head);

Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

[Kirill Tkhai]

On 04.08.2020 15:21, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> Currently, every type of namespaces has its own counter,
>> which is stored in ns-specific part. Say, @net has
>> struct net::count, @pid has struct pid_namespace::kref, etc.
>>
>> This patchset introduces unified counter for all types
>> of namespaces, and converts net namespace to use it first.
> 
> And the other refcounts on struct net?
> 
> How do they play into what you are trying to do?

I just don't understand you. Different refcounters are related to different
problems, they are introduced to solve. This patchset changes only one refcounter,
and it does not touch other of them. What do you want to know about others?

> For the lack of an explanation.

What is unclear for you?

> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> 
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>> ---
>>  include/linux/ns_common.h     |    3 +++
>>  include/net/net_namespace.h   |   11 ++++-------
>>  net/core/net-sysfs.c          |    6 +++---
>>  net/core/net_namespace.c      |    6 +++---
>>  net/ipv4/inet_timewait_sock.c |    4 ++--
>>  net/ipv4/tcp_metrics.c        |    2 +-
>>  6 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/ns_common.h b/include/linux/ns_common.h
>> index 5fbc4000358f..0f1d024bd958 100644
>> --- a/include/linux/ns_common.h
>> +++ b/include/linux/ns_common.h
>> @@ -2,12 +2,15 @@
>>  #ifndef _LINUX_NS_COMMON_H
>>  #define _LINUX_NS_COMMON_H
>>  
>> +#include <linux/refcount.h>
>> +
>>  struct proc_ns_operations;
>>  
>>  struct ns_common {
>>  	atomic_long_t stashed;
>>  	const struct proc_ns_operations *ops;
>>  	unsigned int inum;
>> +	refcount_t count;
>>  };
>>  
>>  #endif
>> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>> index 2ee5901bec7a..cb4b33d7834b 100644
>> --- a/include/net/net_namespace.h
>> +++ b/include/net/net_namespace.h
>> @@ -60,9 +60,6 @@ struct net {
>>  	refcount_t		passive;	/* To decide when the network
>>  						 * namespace should be freed.
>>  						 */
>> -	refcount_t		count;		/* To decided when the network
>> -						 *  namespace should be shut down.
>> -						 */
>>  	spinlock_t		rules_mod_lock;
>>  
>>  	unsigned int		dev_unreg_count;
>> @@ -245,7 +242,7 @@ void __put_net(struct net *net);
>>  
>>  static inline struct net *get_net(struct net *net)
>>  {
>> -	refcount_inc(&net->count);
>> +	refcount_inc(&net->ns.count);
>>  	return net;
>>  }
>>  
>> @@ -256,14 +253,14 @@ static inline struct net *maybe_get_net(struct net *net)
>>  	 * exists.  If the reference count is zero this
>>  	 * function fails and returns NULL.
>>  	 */
>> -	if (!refcount_inc_not_zero(&net->count))
>> +	if (!refcount_inc_not_zero(&net->ns.count))
>>  		net = NULL;
>>  	return net;
>>  }
>>  
>>  static inline void put_net(struct net *net)
>>  {
>> -	if (refcount_dec_and_test(&net->count))
>> +	if (refcount_dec_and_test(&net->ns.count))
>>  		__put_net(net);
>>  }
>>  
>> @@ -275,7 +272,7 @@ int net_eq(const struct net *net1, const struct net *net2)
>>  
>>  static inline int check_net(const struct net *net)
>>  {
>> -	return refcount_read(&net->count) != 0;
>> +	return refcount_read(&net->ns.count) != 0;
>>  }
>>  
>>  void net_drop_ns(void *);
>> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>> index 9de33b594ff2..655a88b0071c 100644
>> --- a/net/core/net-sysfs.c
>> +++ b/net/core/net-sysfs.c
>> @@ -1025,7 +1025,7 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
>>  	while (--i >= new_num) {
>>  		struct kobject *kobj = &dev->_rx[i].kobj;
>>  
>> -		if (!refcount_read(&dev_net(dev)->count))
>> +		if (!refcount_read(&dev_net(dev)->ns.count))
>>  			kobj->uevent_suppress = 1;
>>  		if (dev->sysfs_rx_queue_group)
>>  			sysfs_remove_group(kobj, dev->sysfs_rx_queue_group);
>> @@ -1603,7 +1603,7 @@ netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
>>  	while (--i >= new_num) {
>>  		struct netdev_queue *queue = dev->_tx + i;
>>  
>> -		if (!refcount_read(&dev_net(dev)->count))
>> +		if (!refcount_read(&dev_net(dev)->ns.count))
>>  			queue->kobj.uevent_suppress = 1;
>>  #ifdef CONFIG_BQL
>>  		sysfs_remove_group(&queue->kobj, &dql_group);
>> @@ -1850,7 +1850,7 @@ void netdev_unregister_kobject(struct net_device *ndev)
>>  {
>>  	struct device *dev = &ndev->dev;
>>  
>> -	if (!refcount_read(&dev_net(ndev)->count))
>> +	if (!refcount_read(&dev_net(ndev)->ns.count))
>>  		dev_set_uevent_suppress(dev, 1);
>>  
>>  	kobject_get(&dev->kobj);
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index dcd61aca343e..5f658cbedd34 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -44,7 +44,7 @@ static struct key_tag init_net_key_domain = { .usage = REFCOUNT_INIT(1) };
>>  #endif
>>  
>>  struct net init_net = {
>> -	.count		= REFCOUNT_INIT(1),
>> +	.ns.count	= REFCOUNT_INIT(1),
>>  	.dev_base_head	= LIST_HEAD_INIT(init_net.dev_base_head),
>>  #ifdef CONFIG_KEYS
>>  	.key_domain	= &init_net_key_domain,
>> @@ -248,7 +248,7 @@ int peernet2id_alloc(struct net *net, struct net *peer, gfp_t gfp)
>>  {
>>  	int id;
>>  
>> -	if (refcount_read(&net->count) == 0)
>> +	if (refcount_read(&net->ns.count) == 0)
>>  		return NETNSA_NSID_NOT_ASSIGNED;
>>  
>>  	spin_lock(&net->nsid_lock);
>> @@ -328,7 +328,7 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>>  	int error = 0;
>>  	LIST_HEAD(net_exit_list);
>>  
>> -	refcount_set(&net->count, 1);
>> +	refcount_set(&net->ns.count, 1);
>>  	refcount_set(&net->passive, 1);
>>  	get_random_bytes(&net->hash_mix, sizeof(u32));
>>  	net->dev_base_seq = 1;
>> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
>> index c411c87ae865..437afe392e66 100644
>> --- a/net/ipv4/inet_timewait_sock.c
>> +++ b/net/ipv4/inet_timewait_sock.c
>> @@ -272,14 +272,14 @@ void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family)
>>  				continue;
>>  			tw = inet_twsk(sk);
>>  			if ((tw->tw_family != family) ||
>> -				refcount_read(&twsk_net(tw)->count))
>> +				refcount_read(&twsk_net(tw)->ns.count))
>>  				continue;
>>  
>>  			if (unlikely(!refcount_inc_not_zero(&tw->tw_refcnt)))
>>  				continue;
>>  
>>  			if (unlikely((tw->tw_family != family) ||
>> -				     refcount_read(&twsk_net(tw)->count))) {
>> +				     refcount_read(&twsk_net(tw)->ns.count))) {
>>  				inet_twsk_put(tw);
>>  				goto restart;
>>  			}
>> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
>> index 279db8822439..39710c417565 100644
>> --- a/net/ipv4/tcp_metrics.c
>> +++ b/net/ipv4/tcp_metrics.c
>> @@ -887,7 +887,7 @@ static void tcp_metrics_flush_all(struct net *net)
>>  		pp = &hb->chain;
>>  		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
>>  			match = net ? net_eq(tm_net(tm), net) :
>> -				!refcount_read(&tm_net(tm)->count);
>> +				!refcount_read(&tm_net(tm)->ns.count);
>>  			if (match) {
>>  				*pp = tm->tcpm_next;
>>  				kfree_rcu(tm, rcu_head);

Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

[Eric W. Biederman]

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 04.08.2020 15:21, Eric W. Biederman wrote:
>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>> 
>>> Currently, every type of namespaces has its own counter,
>>> which is stored in ns-specific part. Say, @net has
>>> struct net::count, @pid has struct pid_namespace::kref, etc.
>>>
>>> This patchset introduces unified counter for all types
>>> of namespaces, and converts net namespace to use it first.
>> 
>> And the other refcounts on struct net?
>> 
>> How do they play into what you are trying to do?
>
> I just don't understand you. Different refcounters are related to different
> problems, they are introduced to solve. This patchset changes only one refcounter,
> and it does not touch other of them. What do you want to know about others?
>

Why net::count not net::passive?  What problem are you trying to solve?

They both are reference counts on the network namespace.

I don't understand what you are trying to do, other than take a bunch of
things that look similar and squash them all together.

What semantics does this magical common reference count have?
Why is it better for the count to live in ns_common rather than it
it's own dedicated field of struct net?

Given that decrementing still requires code per namespace to handle
the count going to zero.  How does this benefit anyone?

Has the effect of cache line placement of the move of the reference
count been considered?

All I see in the patch in question is switching a location that the
count lives.  Which does not seem useful to me.

I am not fundamentally oppossed but I don't see the point.  At this
point it looks like you are making things harder to maintain by making
things common that are merely similar.

Eric

Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

[Kirill Tkhai]

On 04.08.2020 16:52, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 04.08.2020 15:21, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> Currently, every type of namespaces has its own counter,
>>>> which is stored in ns-specific part. Say, @net has
>>>> struct net::count, @pid has struct pid_namespace::kref, etc.
>>>>
>>>> This patchset introduces unified counter for all types
>>>> of namespaces, and converts net namespace to use it first.
>>>
>>> And the other refcounts on struct net?
>>>
>>> How do they play into what you are trying to do?
>>
>> I just don't understand you. Different refcounters are related to different
>> problems, they are introduced to solve. This patchset changes only one refcounter,
>> and it does not touch other of them. What do you want to know about others?
>>
> 
> Why net::count not net::passive?  What problem are you trying to solve?

net::count is a counter, which indicates whether net is alive and still
initilized. net::passive does not guarantees that net has not been
deinitialized yet.

The same with another namespaces. All of merged counters indicate
that a namespace is alive and initialized. So, we merge them by this property.

> They both are reference counts on the network namespace.
> 
> I don't understand what you are trying to do, other than take a bunch of
> things that look similar and squash them all together.
> 
> What semantics does this magical common reference count have?
> Why is it better for the count to live in ns_common rather than it
> it's own dedicated field of struct net?

The only visible current effect is better readability and better
debugability with, say, crash on kernel dump.

> Given that decrementing still requires code per namespace to handle
> the count going to zero.  How does this benefit anyone?

Since namespaces are different by type, they require different destructors.
Generic counter can't solve any problem, the namespaces can address.

> Has the effect of cache line placement of the move of the reference
> count been considered?

I don't see any performance impact during namespace creation/destruction
test.

> All I see in the patch in question is switching a location that the
> count lives.  Which does not seem useful to me.
> 
> I am not fundamentally oppossed but I don't see the point.  At this
> point it looks like you are making things harder to maintain by making
> things common that are merely similar

[PATCH 2/8] uts: Use generic ns_common::count

[Kirill Tkhai]

Convert uts namespace to use generic counter instead of kref.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/utsname.h |    9 ++++-----
 init/version.c          |    2 +-
 kernel/utsname.c        |    7 ++-----
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/linux/utsname.h b/include/linux/utsname.h
index 44429d9142ca..2b1737c9b244 100644
--- a/include/linux/utsname.h
+++ b/include/linux/utsname.h
@@ -4,7 +4,6 @@
 
 
 #include <linux/sched.h>
-#include <linux/kref.h>
 #include <linux/nsproxy.h>
 #include <linux/ns_common.h>
 #include <linux/err.h>
@@ -22,7 +21,6 @@ struct user_namespace;
 extern struct user_namespace init_user_ns;
 
 struct uts_namespace {
-	struct kref kref;
 	struct new_utsname name;
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;
@@ -33,16 +31,17 @@ extern struct uts_namespace init_uts_ns;
 #ifdef CONFIG_UTS_NS
 static inline void get_uts_ns(struct uts_namespace *ns)
 {
-	kref_get(&ns->kref);
+	refcount_inc(&ns->ns.count);
 }
 
 extern struct uts_namespace *copy_utsname(unsigned long flags,
 	struct user_namespace *user_ns, struct uts_namespace *old_ns);
-extern void free_uts_ns(struct kref *kref);
+extern void free_uts_ns(struct uts_namespace *ns);
 
 static inline void put_uts_ns(struct uts_namespace *ns)
 {
-	kref_put(&ns->kref, free_uts_ns);
+	if (refcount_dec_and_test(&ns->ns.count))
+		free_uts_ns(ns);
 }
 
 void uts_ns_init(void);
diff --git a/init/version.c b/init/version.c
index cba341161b58..80d2b7566b39 100644
--- a/init/version.c
+++ b/init/version.c
@@ -25,7 +25,7 @@ int version_string(LINUX_VERSION_CODE);
 #endif
 
 struct uts_namespace init_uts_ns = {
-	.kref = KREF_INIT(2),
+	.ns.count = REFCOUNT_INIT(2),
 	.name = {
 		.sysname	= UTS_SYSNAME,
 		.nodename	= UTS_NODENAME,
diff --git a/kernel/utsname.c b/kernel/utsname.c
index e488d0e2ab45..b1ac3ca870f2 100644
--- a/kernel/utsname.c
+++ b/kernel/utsname.c
@@ -33,7 +33,7 @@ static struct uts_namespace *create_uts_ns(void)
 
 	uts_ns = kmem_cache_alloc(uts_ns_cache, GFP_KERNEL);
 	if (uts_ns)
-		kref_init(&uts_ns->kref);
+		refcount_set(&uts_ns->ns.count, 1);
 	return uts_ns;
 }
 
@@ -103,11 +103,8 @@ struct uts_namespace *copy_utsname(unsigned long flags,
 	return new_ns;
 }
 
-void free_uts_ns(struct kref *kref)
+void free_uts_ns(struct uts_namespace *ns)
 {
-	struct uts_namespace *ns;
-
-	ns = container_of(kref, struct uts_namespace, kref);
 	dec_uts_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);
 	ns_free_inum(&ns->ns);

[PATCH 3/8] ipc: Use generic ns_common::count

[Kirill Tkhai]

Convert uts namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/ipc_namespace.h |    3 +--
 ipc/msgutil.c                 |    2 +-
 ipc/namespace.c               |    4 ++--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index a06a78c67f19..05e22770af51 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -27,7 +27,6 @@ struct ipc_ids {
 };
 
 struct ipc_namespace {
-	refcount_t	count;
 	struct ipc_ids	ids[3];
 
 	int		sem_ctls[4];
@@ -128,7 +127,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags,
 static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
 {
 	if (ns)
-		refcount_inc(&ns->count);
+		refcount_inc(&ns->ns.count);
 	return ns;
 }
 
diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 3149b4a379de..d0a0e877cadd 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -26,7 +26,7 @@ DEFINE_SPINLOCK(mq_lock);
  * and not CONFIG_IPC_NS.
  */
 struct ipc_namespace init_ipc_ns = {
-	.count		= REFCOUNT_INIT(1),
+	.ns.count = REFCOUNT_INIT(1),
 	.user_ns = &init_user_ns,
 	.ns.inum = PROC_IPC_INIT_INO,
 #ifdef CONFIG_IPC_NS
diff --git a/ipc/namespace.c b/ipc/namespace.c
index 24e7b45320f7..7bd0766ddc3b 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -51,7 +51,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 		goto fail_free;
 	ns->ns.ops = &ipcns_operations;
 
-	refcount_set(&ns->count, 1);
+	refcount_set(&ns->ns.count, 1);
 	ns->user_ns = get_user_ns(user_ns);
 	ns->ucounts = ucounts;
 
@@ -164,7 +164,7 @@ static DECLARE_WORK(free_ipc_work, free_ipc);
  */
 void put_ipc_ns(struct ipc_namespace *ns)
 {
-	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
+	if (refcount_dec_and_lock(&ns->ns.count, &mq_lock)) {
 		mq_clear_sbinfo(ns);
 		spin_unlock(&mq_lock);
 

[PATCH 4/8] pid: Use generic ns_common::count

[Kirill Tkhai]

Convert pid namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/pid_namespace.h |    4 +---
 kernel/pid.c                  |    2 +-
 kernel/pid_namespace.c        |   13 +++----------
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 5a5cb45ac57e..7c7e627503d2 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -8,7 +8,6 @@
 #include <linux/workqueue.h>
 #include <linux/threads.h>
 #include <linux/nsproxy.h>
-#include <linux/kref.h>
 #include <linux/ns_common.h>
 #include <linux/idr.h>
 
@@ -18,7 +17,6 @@
 struct fs_pin;
 
 struct pid_namespace {
-	struct kref kref;
 	struct idr idr;
 	struct rcu_head rcu;
 	unsigned int pid_allocated;
@@ -43,7 +41,7 @@ extern struct pid_namespace init_pid_ns;
 static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
 {
 	if (ns != &init_pid_ns)
-		kref_get(&ns->kref);
+		refcount_inc(&ns->ns.count);
 	return ns;
 }
 
diff --git a/kernel/pid.c b/kernel/pid.c
index de9d29c41d77..3b9e67736ef4 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -72,7 +72,7 @@ int pid_max_max = PID_MAX_LIMIT;
  * the scheme scales to up to 4 million PIDs, runtime.
  */
 struct pid_namespace init_pid_ns = {
-	.kref = KREF_INIT(2),
+	.ns.count = REFCOUNT_INIT(2),
 	.idr = IDR_INIT(init_pid_ns.idr),
 	.pid_allocated = PIDNS_ADDING,
 	.level = 0,
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 0e5ac162c3a8..d02dc1696edf 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -102,7 +102,7 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
 		goto out_free_idr;
 	ns->ns.ops = &pidns_operations;
 
-	kref_init(&ns->kref);
+	refcount_set(&ns->ns.count, 1);
 	ns->level = level;
 	ns->parent = get_pid_ns(parent_pid_ns);
 	ns->user_ns = get_user_ns(user_ns);
@@ -148,22 +148,15 @@ struct pid_namespace *copy_pid_ns(unsigned long flags,
 	return create_pid_namespace(user_ns, old_ns);
 }
 
-static void free_pid_ns(struct kref *kref)
-{
-	struct pid_namespace *ns;
-
-	ns = container_of(kref, struct pid_namespace, kref);
-	destroy_pid_namespace(ns);
-}
-
 void put_pid_ns(struct pid_namespace *ns)
 {
 	struct pid_namespace *parent;
 
 	while (ns != &init_pid_ns) {
 		parent = ns->parent;
-		if (!kref_put(&ns->kref, free_pid_ns))
+		if (!refcount_dec_and_test(&ns->ns.count))
 			break;
+		destroy_pid_namespace(ns);
 		ns = parent;
 	}
 }

[PATCH 5/8] user: Use generic ns_common::count

[Kirill Tkhai]

Convert user namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/user_namespace.h |    5 ++---
 kernel/user.c                  |    2 +-
 kernel/user_namespace.c        |    4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 6ef1c7109fc4..64cf8ebdc4ec 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -57,7 +57,6 @@ struct user_namespace {
 	struct uid_gid_map	uid_map;
 	struct uid_gid_map	gid_map;
 	struct uid_gid_map	projid_map;
-	atomic_t		count;
 	struct user_namespace	*parent;
 	int			level;
 	kuid_t			owner;
@@ -109,7 +108,7 @@ void dec_ucount(struct ucounts *ucounts, enum ucount_type type);
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
 {
 	if (ns)
-		atomic_inc(&ns->count);
+		refcount_inc(&ns->ns.count);
 	return ns;
 }
 
@@ -119,7 +118,7 @@ extern void __put_user_ns(struct user_namespace *ns);
 
 static inline void put_user_ns(struct user_namespace *ns)
 {
-	if (ns && atomic_dec_and_test(&ns->count))
+	if (ns && refcount_dec_and_test(&ns->ns.count))
 		__put_user_ns(ns);
 }
 
diff --git a/kernel/user.c b/kernel/user.c
index b1635d94a1f2..a2478cddf536 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -55,7 +55,7 @@ struct user_namespace init_user_ns = {
 			},
 		},
 	},
-	.count = ATOMIC_INIT(3),
+	.ns.count = REFCOUNT_INIT(3),
 	.owner = GLOBAL_ROOT_UID,
 	.group = GLOBAL_ROOT_GID,
 	.ns.inum = PROC_USER_INIT_INO,
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 87804e0371fe..7c2bbe8f3e45 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -111,7 +111,7 @@ int create_user_ns(struct cred *new)
 		goto fail_free;
 	ns->ns.ops = &userns_operations;
 
-	atomic_set(&ns->count, 1);
+	refcount_set(&ns->ns.count, 1);
 	/* Leave the new->user_ns reference with the new user namespace. */
 	ns->parent = parent_ns;
 	ns->level = parent_ns->level + 1;
@@ -197,7 +197,7 @@ static void free_user_ns(struct work_struct *work)
 		kmem_cache_free(user_ns_cachep, ns);
 		dec_user_namespaces(ucounts);
 		ns = parent;
-	} while (atomic_dec_and_test(&parent->count));
+	} while (refcount_dec_and_test(&parent->ns.count));
 }
 
 void __put_user_ns(struct user_namespace *ns)

[PATCH 6/8] mnt: Use generic ns_common::count

[Kirill Tkhai]

Convert mount namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/mount.h     |    3 +--
 fs/namespace.c |    4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index c3e0bb6e5782..f296862032ec 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -7,7 +7,6 @@
 #include <linux/watch_queue.h>
 
 struct mnt_namespace {
-	atomic_t		count;
 	struct ns_common	ns;
 	struct mount *	root;
 	/*
@@ -130,7 +129,7 @@ static inline void detach_mounts(struct dentry *dentry)
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {
-	atomic_inc(&ns->count);
+	refcount_inc(&ns->ns.count);
 }
 
 extern seqlock_t mount_lock;
diff --git a/fs/namespace.c b/fs/namespace.c
index 31c387794fbd..8c39810e6ec3 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3296,7 +3296,7 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns, bool a
 	new_ns->ns.ops = &mntns_operations;
 	if (!anon)
 		new_ns->seq = atomic64_add_return(1, &mnt_ns_seq);
-	atomic_set(&new_ns->count, 1);
+	refcount_set(&new_ns->ns.count, 1);
 	INIT_LIST_HEAD(&new_ns->list);
 	init_waitqueue_head(&new_ns->poll);
 	spin_lock_init(&new_ns->ns_lock);
@@ -3870,7 +3870,7 @@ void __init mnt_init(void)
 
 void put_mnt_ns(struct mnt_namespace *ns)
 {
-	if (!atomic_dec_and_test(&ns->count))
+	if (!refcount_dec_and_test(&ns->ns.count))
 		return;
 	drop_collected_mounts(&ns->root->mnt);
 	free_mnt_ns(ns);

[PATCH 7/8] cgroup: Use generic ns_common::count

[Kirill Tkhai]

Convert cgroup namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/cgroup.h    |    5 ++---
 kernel/cgroup/cgroup.c    |    2 +-
 kernel/cgroup/namespace.c |    2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 618838c48313..451c2d26a5db 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -854,7 +854,6 @@ static inline void cgroup_sk_free(struct sock_cgroup_data *skcd) {}
 #endif	/* CONFIG_CGROUP_DATA */
 
 struct cgroup_namespace {
-	refcount_t		count;
 	struct ns_common	ns;
 	struct user_namespace	*user_ns;
 	struct ucounts		*ucounts;
@@ -889,12 +888,12 @@ copy_cgroup_ns(unsigned long flags, struct user_namespace *user_ns,
 static inline void get_cgroup_ns(struct cgroup_namespace *ns)
 {
 	if (ns)
-		refcount_inc(&ns->count);
+		refcount_inc(&ns->ns.count);
 }
 
 static inline void put_cgroup_ns(struct cgroup_namespace *ns)
 {
-	if (ns && refcount_dec_and_test(&ns->count))
+	if (ns && refcount_dec_and_test(&ns->ns.count))
 		free_cgroup_ns(ns);
 }
 
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index dd247747ec14..22e466926853 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -199,7 +199,7 @@ static u16 have_canfork_callback __read_mostly;
 
 /* cgroup namespace for init task */
 struct cgroup_namespace init_cgroup_ns = {
-	.count		= REFCOUNT_INIT(2),
+	.ns.count	= REFCOUNT_INIT(2),
 	.user_ns	= &init_user_ns,
 	.ns.ops		= &cgroupns_operations,
 	.ns.inum	= PROC_CGROUP_INIT_INO,
diff --git a/kernel/cgroup/namespace.c b/kernel/cgroup/namespace.c
index 812a61afd538..f5e8828c109c 100644
--- a/kernel/cgroup/namespace.c
+++ b/kernel/cgroup/namespace.c
@@ -32,7 +32,7 @@ static struct cgroup_namespace *alloc_cgroup_ns(void)
 		kfree(new_ns);
 		return ERR_PTR(ret);
 	}
-	refcount_set(&new_ns->count, 1);
+	refcount_set(&new_ns->ns.count, 1);
 	new_ns->ns.ops = &cgroupns_operations;
 	return new_ns;
 }

[PATCH 8/8] time: Use generic ns_common::count

[Kirill Tkhai]

Convert time namespace to use generic counter.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/time_namespace.h |    9 ++++-----
 kernel/time/namespace.c        |    9 +++------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/linux/time_namespace.h b/include/linux/time_namespace.h
index 5b6031385db0..a51ffc089219 100644
--- a/include/linux/time_namespace.h
+++ b/include/linux/time_namespace.h
@@ -4,7 +4,6 @@
 
 
 #include <linux/sched.h>
-#include <linux/kref.h>
 #include <linux/nsproxy.h>
 #include <linux/ns_common.h>
 #include <linux/err.h>
@@ -18,7 +17,6 @@ struct timens_offsets {
 };
 
 struct time_namespace {
-	struct kref		kref;
 	struct user_namespace	*user_ns;
 	struct ucounts		*ucounts;
 	struct ns_common	ns;
@@ -37,20 +35,21 @@ extern void timens_commit(struct task_struct *tsk, struct time_namespace *ns);
 
 static inline struct time_namespace *get_time_ns(struct time_namespace *ns)
 {
-	kref_get(&ns->kref);
+	refcount_inc(&ns->ns.count);
 	return ns;
 }
 
 struct time_namespace *copy_time_ns(unsigned long flags,
 				    struct user_namespace *user_ns,
 				    struct time_namespace *old_ns);
-void free_time_ns(struct kref *kref);
+void free_time_ns(struct time_namespace *ns);
 int timens_on_fork(struct nsproxy *nsproxy, struct task_struct *tsk);
 struct vdso_data *arch_get_vdso_data(void *vvar_page);
 
 static inline void put_time_ns(struct time_namespace *ns)
 {
-	kref_put(&ns->kref, free_time_ns);
+	if (refcount_dec_and_test(&ns->ns.count))
+		free_time_ns(ns);
 }
 
 void proc_timens_show_offsets(struct task_struct *p, struct seq_file *m);
diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c
index afc65e6be33e..c4c829eb3511 100644
--- a/kernel/time/namespace.c
+++ b/kernel/time/namespace.c
@@ -92,7 +92,7 @@ static struct time_namespace *clone_time_ns(struct user_namespace *user_ns,
 	if (!ns)
 		goto fail_dec;
 
-	kref_init(&ns->kref);
+	refcount_set(&ns->ns.count, 1);
 
 	ns->vvar_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 	if (!ns->vvar_page)
@@ -226,11 +226,8 @@ static void timens_set_vvar_page(struct task_struct *task,
 	mutex_unlock(&offset_lock);
 }
 
-void free_time_ns(struct kref *kref)
+void free_time_ns(struct time_namespace *ns)
 {
-	struct time_namespace *ns;
-
-	ns = container_of(kref, struct time_namespace, kref);
 	dec_time_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);
 	ns_free_inum(&ns->ns);
@@ -464,7 +461,7 @@ const struct proc_ns_operations timens_for_children_operations = {
 };
 
 struct time_namespace init_time_ns = {
-	.kref		= KREF_INIT(3),
+	.ns.count	= REFCOUNT_INIT(3),
 	.user_ns	= &init_user_ns,
 	.ns.inum	= PROC_TIME_INIT_INO,
 	.ns.ops		= &timens_operations,

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Christian Brauner]

On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
> Every namespace type has its own counter. Some of them are
> of refcount_t, some of them are of kref.
> 
> This patchset introduces generic ns_common::count for any
> type of namespaces instead of them.
> 
> ---

I was wondering why that series never made it to me turns out there's
some weird bug in my (neo)mutt where it sometimes marks messages as read
when I'm deleting completely unrelated messages. That has already cost
me a talk slot for an event I really wanted to attend and now it seems
to start costing me patches... I need to figure this out.

Anyway, thanks for sending this. I pulled this into my tree now.

Thanks!
Christian

> 
> Kirill Tkhai (8):
>       ns: Add common refcount into ns_common add use it as counter for net_ns
>       uts: Use generic ns_common::count
>       ipc: Use generic ns_common::count
>       pid: Use generic ns_common::count
>       user: Use generic ns_common::count
>       mnt: Use generic ns_common::count
>       cgroup: Use generic ns_common::count
>       time: Use generic ns_common::count
> 
> 
>  fs/mount.h                     |    3 +--
>  fs/namespace.c                 |    4 ++--
>  include/linux/cgroup.h         |    5 ++---
>  include/linux/ipc_namespace.h  |    3 +--
>  include/linux/ns_common.h      |    3 +++
>  include/linux/pid_namespace.h  |    4 +---
>  include/linux/time_namespace.h |    9 ++++-----
>  include/linux/user_namespace.h |    5 ++---
>  include/linux/utsname.h        |    9 ++++-----
>  include/net/net_namespace.h    |   11 ++++-------
>  init/version.c                 |    2 +-
>  ipc/msgutil.c                  |    2 +-
>  ipc/namespace.c                |    4 ++--
>  kernel/cgroup/cgroup.c         |    2 +-
>  kernel/cgroup/namespace.c      |    2 +-
>  kernel/pid.c                   |    2 +-
>  kernel/pid_namespace.c         |   13 +++----------
>  kernel/time/namespace.c        |    9 +++------
>  kernel/user.c                  |    2 +-
>  kernel/user_namespace.c        |    4 ++--
>  kernel/utsname.c               |    7 ++-----
>  net/core/net-sysfs.c           |    6 +++---
>  net/core/net_namespace.c       |    6 +++---
>  net/ipv4/inet_timewait_sock.c  |    4 ++--
>  net/ipv4/tcp_metrics.c         |    2 +-
>  25 files changed, 51 insertions(+), 72 deletions(-)
> 
> --
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> 

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Eric W. Biederman]

Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
>> Every namespace type has its own counter. Some of them are
>> of refcount_t, some of them are of kref.
>> 
>> This patchset introduces generic ns_common::count for any
>> type of namespaces instead of them.
>> 
>> ---
>
> I was wondering why that series never made it to me turns out there's
> some weird bug in my (neo)mutt where it sometimes marks messages as read
> when I'm deleting completely unrelated messages. That has already cost
> me a talk slot for an event I really wanted to attend and now it seems
> to start costing me patches... I need to figure this out.
>
> Anyway, thanks for sending this. I pulled this into my tree now.

Actually why in the world should the reference count be generic?

What is the point of this patchset?

What problem does it solve.  Name spaces are not the same, and
their refcounting needs are not the same so I don't have a clue how it
helps anything to have a reference count in ns_common.

Eric

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Christian Brauner]

On Tue, Aug 04, 2020 at 07:11:59AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
> > On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
> >> Every namespace type has its own counter. Some of them are
> >> of refcount_t, some of them are of kref.
> >> 
> >> This patchset introduces generic ns_common::count for any
> >> type of namespaces instead of them.
> >> 
> >> ---
> >
> > I was wondering why that series never made it to me turns out there's
> > some weird bug in my (neo)mutt where it sometimes marks messages as read
> > when I'm deleting completely unrelated messages. That has already cost
> > me a talk slot for an event I really wanted to attend and now it seems
> > to start costing me patches... I need to figure this out.
> >
> > Anyway, thanks for sending this. I pulled this into my tree now.
> 
> Actually why in the world should the reference count be generic?
> 
> What is the point of this patchset?
> 
> What problem does it solve.  Name spaces are not the same, and
> their refcounting needs are not the same so I don't have a clue how it
> helps anything to have a reference count in ns_common.

What is the point of this opposition to this cleanup?

It unifies reference counting across namespaces and gets rid of
inconsistencices. Over the years none of the namespaces seem to have
deviated enough from each that they really have needed separate
reference counting mechanisms.

Christian

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Eric W. Biederman]

Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Tue, Aug 04, 2020 at 07:11:59AM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brauner@ubuntu.com> writes:
>> 
>> > On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
>> >> Every namespace type has its own counter. Some of them are
>> >> of refcount_t, some of them are of kref.
>> >> 
>> >> This patchset introduces generic ns_common::count for any
>> >> type of namespaces instead of them.
>> >> 
>> >> ---
>> >
>> > I was wondering why that series never made it to me turns out there's
>> > some weird bug in my (neo)mutt where it sometimes marks messages as read
>> > when I'm deleting completely unrelated messages. That has already cost
>> > me a talk slot for an event I really wanted to attend and now it seems
>> > to start costing me patches... I need to figure this out.
>> >
>> > Anyway, thanks for sending this. I pulled this into my tree now.
>> 
>> Actually why in the world should the reference count be generic?
>> 
>> What is the point of this patchset?
>> 
>> What problem does it solve.  Name spaces are not the same, and
>> their refcounting needs are not the same so I don't have a clue how it
>> helps anything to have a reference count in ns_common.
>
> What is the point of this opposition to this cleanup?
>
> It unifies reference counting across namespaces and gets rid of
> inconsistencices. Over the years none of the namespaces seem to have
> deviated enough from each that they really have needed separate
> reference counting mechanisms.

First this posting is the first I have seen of it, unless it was a
subset of the weird /proc/namespaces/ patchset that has design problems.
In which case I never got this far.

Second I don't see a motivation for this.  The only point to place a
reference count in ns_common is if it makes something easier.  What
does it make easier and what does it make harder?

For a pure cleanup the questions are what are the trade offs.
There are potential performance differences between refcount_t and
kfref.

From a practical matter it makes absolutely no sense in the least to
talk about the reference count, when some of the namespaces have more
than one reference count, with difference semantics and they interrelate
in somewhat subtle ways.

Further depending on what is happening sharing code that does not
have a fundamental reason to be shared, can make maintenance more
difficult as the entire generic infrastructure will need to be updated
instead of just that the part that focuses on the one thing.

So I am opposed because the patchset does not explain at all why it
makes sense to do, nor what tradeoffs were considered, nor what
testing was done.

This change is not as trivial as a spelling change so it is not ok to
say it is just a cleanup and move on.  A change in the reference
counting can be noticable.  This needs at least to be acknowledged in
the change log and at a minimum a hand wavy reason put forth why it is
ok.


Instead what I am seeing as justification is this is a trivial cleanup
and no one will notice or care.   And it is not that trivial so I
object to the patchset.

Eric

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Christian Brauner]

On Tue, Aug 04, 2020 at 08:21:51AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
> > On Tue, Aug 04, 2020 at 07:11:59AM -0500, Eric W. Biederman wrote:
> >> Christian Brauner <christian.brauner@ubuntu.com> writes:
> >> 
> >> > On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
> >> >> Every namespace type has its own counter. Some of them are
> >> >> of refcount_t, some of them are of kref.
> >> >> 
> >> >> This patchset introduces generic ns_common::count for any
> >> >> type of namespaces instead of them.
> >> >> 
> >> >> ---
> >> >
> >> > I was wondering why that series never made it to me turns out there's
> >> > some weird bug in my (neo)mutt where it sometimes marks messages as read
> >> > when I'm deleting completely unrelated messages. That has already cost
> >> > me a talk slot for an event I really wanted to attend and now it seems
> >> > to start costing me patches... I need to figure this out.
> >> >
> >> > Anyway, thanks for sending this. I pulled this into my tree now.
> >> 
> >> Actually why in the world should the reference count be generic?
> >> 
> >> What is the point of this patchset?
> >> 
> >> What problem does it solve.  Name spaces are not the same, and
> >> their refcounting needs are not the same so I don't have a clue how it
> >> helps anything to have a reference count in ns_common.
> >
> > What is the point of this opposition to this cleanup?
> >
> > It unifies reference counting across namespaces and gets rid of
> > inconsistencices. Over the years none of the namespaces seem to have
> > deviated enough from each that they really have needed separate
> > reference counting mechanisms.
> 
> First this posting is the first I have seen of it, unless it was a
> subset of the weird /proc/namespaces/ patchset that has design problems.
> In which case I never got this far.
> 
> Second I don't see a motivation for this.  The only point to place a
> reference count in ns_common is if it makes something easier.  What
> does it make easier and what does it make harder?
> 
> For a pure cleanup the questions are what are the trade offs.
> There are potential performance differences between refcount_t and
> kfref.
> 
> From a practical matter it makes absolutely no sense in the least to
> talk about the reference count, when some of the namespaces have more
> than one reference count, with difference semantics and they interrelate
> in somewhat subtle ways.
> 
> Further depending on what is happening sharing code that does not
> have a fundamental reason to be shared, can make maintenance more
> difficult as the entire generic infrastructure will need to be updated
> instead of just that the part that focuses on the one thing.
> 
> So I am opposed because the patchset does not explain at all why it
> makes sense to do, nor what tradeoffs were considered, nor what
> testing was done.
> 
> This change is not as trivial as a spelling change so it is not ok to
> say it is just a cleanup and move on.  A change in the reference
> counting can be noticable.  This needs at least to be acknowledged in
> the change log and at a minimum a hand wavy reason put forth why it is
> ok.
> 
> 
> Instead what I am seeing as justification is this is a trivial cleanup
> and no one will notice or care.   And it is not that trivial so I
> object to the patchset.

The "not seeing a motivation for this" is a suprising argument to me to
which I honestly can only respond that I don't see a motivation for not
doing this. It unifies and simplifies code and removes variance.

There can't be performance differences between kref and refcount_t since
kref is implemented on top of refcount_t. All functions that Kirill is
replacing are static inline wrappers around refcount_t helpers.

I have no idea why it is wrong talking about reference counts just
because something can have multiple counters or reference counters.

Another way of looking at this is that this patchset removes maintenance
by moving the shared parts of reference counting into a single place.
They are literally shared across all namespaces. I don't see a specific
worry other than the very hand wavy "if we ever have to change something
we have to update the whole generic infrastructure". That could be used
to shoot down 30 cleanup patchsets each development cycle that
significantly contribute to code legibility and maintenance. This is
also how it should work and one of the great benefits of the namespace
infra along with other parts of the kernel is that it is nice and
generic.

If you have issues with the patch descriptions than I'm sure Kirill will
be happy to adapt them to include more detail. If he can't I'm happy to.

Since past experience tells me that we're unlikely to settle this
dispute let's bring in a few more people to take a look at this.

Christian

Re: [PATCH 0/8] namespaces: Introduce generic refcount

[Kees Cook]

On Tue, Aug 04, 2020 at 04:57:14PM +0200, Christian Brauner wrote:
> On Tue, Aug 04, 2020 at 08:21:51AM -0500, Eric W. Biederman wrote:
> > This change is not as trivial as a spelling change so it is not ok to
> > say it is just a cleanup and move on.  A change in the reference
> > counting can be noticable.  This needs at least to be acknowledged in
> > the change log and at a minimum a hand wavy reason put forth why it is
> > ok.
> > 
> > 
> > Instead what I am seeing as justification is this is a trivial cleanup
> > and no one will notice or care.   And it is not that trivial so I
> > object to the patchset.
> 
> The "not seeing a motivation for this" is a suprising argument to me to
> which I honestly can only respond that I don't see a motivation for not
> doing this. It unifies and simplifies code and removes variance.

Thanks for the CC! Ironically I had noticed this series yesterday at the
end of my day and thought to myself, "that's great; I need to go Ack
that tomorrow" and added it to my TODO list. :P

> > So I am opposed because the patchset does not explain at all why it
> > makes sense to do, nor what tradeoffs were considered, nor what
> > testing was done.

This is a reasonable concern; to me the rationale is clear, but you have
a point that it's not very well spelled out. From my perspective I see
the following benefits:

- The code is simplified because now there is a single lifetime counter
  and it goes in the right place: ns_common ... because it's common to
  all of the namespaces. (Yes there are other counters, but the lifetime
  count is common to all of them.) And the stats on the refactoring
  seems to support it with "51 insertions(+), 72 deletions(-)" which,
  while not "proof" that it's correct, is a positive signal that it's
  going in the right direction, implying a reduced maintenance cost.

- refcount_t is safer than atomic_t, and just as fast[1].

- This reduces the atomic_t -> refcount_t delta[2] that is still pending
  from Elena's efforts started in 2017.

So, while the commit logs could perhaps be clarified (and maybe include
a "in preparation for showing namespaces in ..." prefix describing the
work that triggered this clean-up), I think this is a good idea. Please
consider the whole series:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

[1] https://git.kernel.org/linus/dcb786493f3e48da3272b710028d42ec608cfda1
[2] https://lore.kernel.org/lkml/1511954324-14414-1-git-send-email-elena.reshetova@intel.com/

-- 
Kees Cook