[PATCH v1 0/5] Rework net_cls update socket logic

[PATCH v1 0/5] Rework net_cls update socket logic

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

Hi,

I updated this series according the feedback I got. I am not sure
if I did the 'revert' patch correctly (patch #3). 

cheers,
daniel

orignal cover letter:

This series updates net_cls to use the same socket update logic as
net_prio. With this change sock_update_classid() is not called from
recvmsg, sendmsg and friends anymore. This seems to be a good idea.

Credit goes to John Fastabend for this work.

The patches are against net-next.

Daniel Wagner (5):
  cgroup: net_prio: Mark local used function static
  cgroup: net_cls: Fix local variable type decleration
  cgroup: net_cls: Remove rcu_read_lock/unlock
  cgroup: net_cls: Pass in task to sock_update_classid()
  cgroup: net_cls: Rework update socket logic

 drivers/net/tun.c         |  3 ---
 include/net/cls_cgroup.h  |  4 ++--
 net/core/netprio_cgroup.c |  2 +-
 net/core/sock.c           |  8 +++-----
 net/sched/cls_cgroup.c    | 24 ++++++++++++++++++++++++
 net/socket.c              |  8 --------
 6 files changed, 30 insertions(+), 19 deletions(-)

-- 
1.8.0.rc0

[PATCH v1 1/5] cgroup: net_prio: Mark local used function static

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

net_prio_attach() is only access via cgroup_subsys callbacks,
therefore we can reduce the visibility of this function.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 net/core/netprio_cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 79285a3..847c02b 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -248,7 +248,7 @@ static int update_netprio(const void *v, struct file *file, unsigned n)
 	return 0;
 }
 
-void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+static void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 {
 	struct task_struct *p;
 	void *v;
-- 
1.8.0.rc0

[PATCH v1 2/5] cgroup: net_cls: Fix local variable type decleration

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

The classid type used throughout the kernel is u32.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: <netdev@vger.kernel.org>
Cc: <cgroups@vger.kernel.org>
---
 include/net/cls_cgroup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index b6a6eeb..0fee061 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -29,7 +29,7 @@ extern void sock_update_classid(struct sock *sk);
 #if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 static inline u32 task_cls_classid(struct task_struct *p)
 {
-	int classid;
+	u32 classid;
 
 	if (in_interrupt())
 		return 0;
-- 
1.8.0.rc0

[PATCH v1 3/5] cgroup: net_cls: Remove rcu_read_lock/unlock

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

As Eric pointed out:
"Hey task_cls_classid() has its own rcu protection since commit
3fb5a991916091a908d (cls_cgroup: Fix rcu lockdep warning)

So we can safely revert Paul commit (1144182a8757f2a1)
(We no longer need rcu_read_lock/unlock here)"

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: netdev@vger.kernel.org
Cc: cgroups@vger.kernel.org
---
 net/core/sock.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index c49412c..9fedbbf 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1221,9 +1221,7 @@ void sock_update_classid(struct sock *sk)
 {
 	u32 classid;
 
-	rcu_read_lock();  /* doing current task, which cannot vanish. */
 	classid = task_cls_classid(current);
-	rcu_read_unlock();
 	if (classid != sk->sk_classid)
 		sk->sk_classid = classid;
 }
-- 
1.8.0.rc0

[PATCH v1 4/5] cgroup: net_cls: Pass in task to sock_update_classid()

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

sock_update_classid() assumes that the update operation always are
applied on the current task. sock_update_classid() needs to know on
which tasks to work on in order to be able to migrate task between
cgroups using the struct cgroup_subsys attach() callback.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/net/tun.c        | 2 +-
 include/net/cls_cgroup.h | 2 +-
 net/core/sock.c          | 6 +++---
 net/socket.c             | 8 ++++----
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 0873cdc..e4858b2 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -587,7 +587,7 @@ static struct sk_buff *tun_alloc_skb(struct tun_struct *tun,
 	struct sk_buff *skb;
 	int err;
 
-	sock_update_classid(sk);
+	sock_update_classid(sk, current);
 
 	/* Under a page?  Don't bother with paged skb. */
 	if (prepad + len < PAGE_SIZE || !linear)
diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index 0fee061..571de6e 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -24,7 +24,7 @@ struct cgroup_cls_state
 	u32 classid;
 };
 
-extern void sock_update_classid(struct sock *sk);
+extern void sock_update_classid(struct sock *sk, struct task_struct *task);
 
 #if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 static inline u32 task_cls_classid(struct task_struct *p)
diff --git a/net/core/sock.c b/net/core/sock.c
index 9fedbbf..0a023b8 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1217,11 +1217,11 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 
 #ifdef CONFIG_CGROUPS
 #if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
-void sock_update_classid(struct sock *sk)
+void sock_update_classid(struct sock *sk, struct task_struct *task)
 {
 	u32 classid;
 
-	classid = task_cls_classid(current);
+	classid = task_cls_classid(task);
 	if (classid != sk->sk_classid)
 		sk->sk_classid = classid;
 }
@@ -1264,7 +1264,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sock_net_set(sk, get_net(net));
 		atomic_set(&sk->sk_wmem_alloc, 1);
 
-		sock_update_classid(sk);
+		sock_update_classid(sk, current);
 		sock_update_netprioidx(sk, current);
 	}
 
diff --git a/net/socket.c b/net/socket.c
index d92c490..f1a481a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -620,7 +620,7 @@ static inline int __sock_sendmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
+	sock_update_classid(sock->sk, current);
 
 	si->sock = sock;
 	si->scm = NULL;
@@ -784,7 +784,7 @@ static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
+	sock_update_classid(sock->sk, current);
 
 	si->sock = sock;
 	si->scm = NULL;
@@ -896,7 +896,7 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 	if (unlikely(!sock->ops->splice_read))
 		return -EINVAL;
 
-	sock_update_classid(sock->sk);
+	sock_update_classid(sock->sk, current);
 
 	return sock->ops->splice_read(sock, ppos, pipe, len, flags);
 }
@@ -3437,7 +3437,7 @@ EXPORT_SYMBOL(kernel_setsockopt);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	sock_update_classid(sock->sk);
+	sock_update_classid(sock->sk, current);
 
 	if (sock->ops->sendpage)
 		return sock->ops->sendpage(sock, page, offset, size, flags);
-- 
1.8.0.rc0

[PATCH v1 5/5] cgroup: net_cls: Rework update socket logic

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

The cgroup logic part of net_cls is very similar as the one in
net_prio. Let's stream line the net_cls logic with the net_prio one.

The net_prio update logic was changed by following commit (note there
were some changes necessary later on)

commit 406a3c638ce8b17d9704052c07955490f732c2b8
Author: John Fastabend <john.r.fastabend@intel.com>
Date:   Fri Jul 20 10:39:25 2012 +0000

    net: netprio_cgroup: rework update socket logic

    Instead of updating the sk_cgrp_prioidx struct field on every send
    this only updates the field when a task is moved via cgroup
    infrastructure.

    This allows sockets that may be used by a kernel worker thread
    to be managed. For example in the iscsi case today a user can
    put iscsid in a netprio cgroup and control traffic will be sent
    with the correct sk_cgrp_prioidx value set but as soon as data
    is sent the kernel worker thread isssues a send and sk_cgrp_prioidx
    is updated with the kernel worker threads value which is the
    default case.

    It seems more correct to only update the field when the user
    explicitly sets it via control group infrastructure. This allows
    the users to manage sockets that may be used with other threads.

Since classid is now updated when the task is moved between the
cgroups, we don't have to call sock_update_classid() from various
places to ensure we always using the latest classid value.

[v2: Use iterate_fd() instead of open coding]

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc:  Li Zefan <lizefan@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Joe Perches <joe@perches.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: <netdev@vger.kernel.org>
Cc: <cgroups@vger.kernel.org>
---
 drivers/net/tun.c      |  3 ---
 net/sched/cls_cgroup.c | 24 ++++++++++++++++++++++++
 net/socket.c           |  8 --------
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e4858b2..3157519 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -68,7 +68,6 @@
 #include <net/netns/generic.h>
 #include <net/rtnetlink.h>
 #include <net/sock.h>
-#include <net/cls_cgroup.h>
 
 #include <asm/uaccess.h>
 
@@ -587,8 +586,6 @@ static struct sk_buff *tun_alloc_skb(struct tun_struct *tun,
 	struct sk_buff *skb;
 	int err;
 
-	sock_update_classid(sk, current);
-
 	/* Under a page?  Don't bother with paged skb. */
 	if (prepad + len < PAGE_SIZE || !linear)
 		linear = len;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 2ecde22..709b0fb 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
+#include <linux/fdtable.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
@@ -53,6 +54,28 @@ static void cgrp_destroy(struct cgroup *cgrp)
 	kfree(cgrp_cls_state(cgrp));
 }
 
+static int update_classid(const void *v, struct file *file, unsigned n)
+{
+	int err;
+	struct socket *sock = sock_from_file(file, &err);
+	if (sock)
+		sock->sk->sk_classid = (u32)(unsigned long)v;
+	return 0;
+}
+
+static void cgrp_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+{
+	struct task_struct *p;
+	void *v;
+
+	cgroup_taskset_for_each(p, cgrp, tset) {
+		task_lock(p);
+		v = (void *)(unsigned long)task_cls_classid(p);
+		iterate_fd(p->files, 0, update_classid, v);
+		task_unlock(p);
+	}
+}
+
 static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 {
 	return cgrp_cls_state(cgrp)->classid;
@@ -77,6 +100,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
+	.attach		= cgrp_attach,
 	.subsys_id	= net_cls_subsys_id,
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
diff --git a/net/socket.c b/net/socket.c
index f1a481a..2ca51c7 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -620,8 +620,6 @@ static inline int __sock_sendmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk, current);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -784,8 +782,6 @@ static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk, current);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -896,8 +892,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 	if (unlikely(!sock->ops->splice_read))
 		return -EINVAL;
 
-	sock_update_classid(sock->sk, current);
-
 	return sock->ops->splice_read(sock, ppos, pipe, len, flags);
 }
 
@@ -3437,8 +3431,6 @@ EXPORT_SYMBOL(kernel_setsockopt);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	sock_update_classid(sock->sk, current);
-
 	if (sock->ops->sendpage)
 		return sock->ops->sendpage(sock, page, offset, size, flags);
 
-- 
1.8.0.rc0

Re: [PATCH v1 4/5] cgroup: net_cls: Pass in task to sock_update_classid()

[Tejun Heo]

On Thu, Oct 25, 2012 at 04:16:58PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> sock_update_classid() assumes that the update operation always are
> applied on the current task. sock_update_classid() needs to know on
> which tasks to work on in order to be able to migrate task between
> cgroups using the struct cgroup_subsys attach() callback.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

1-4 look good to me.  I have no idea about 5. :)

Thanks.

-- 
tejun

Re: [PATCH v1 1/5] cgroup: net_prio: Mark local used function static

[Neil Horman]

On Thu, Oct 25, 2012 at 04:16:55PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> net_prio_attach() is only access via cgroup_subsys callbacks,
> therefore we can reduce the visibility of this function.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  net/core/netprio_cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index 79285a3..847c02b 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -248,7 +248,7 @@ static int update_netprio(const void *v, struct file *file, unsigned n)
>  	return 0;
>  }
>  
> -void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
> +static void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
>  {
>  	struct task_struct *p;
>  	void *v;
> -- 
> 1.8.0.rc0
> 
> 
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Re: [PATCH v1 2/5] cgroup: net_cls: Fix local variable type decleration

[Neil Horman]

On Thu, Oct 25, 2012 at 04:16:56PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> The classid type used throughout the kernel is u32.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: <netdev@vger.kernel.org>
> Cc: <cgroups@vger.kernel.org>
> ---
>  include/net/cls_cgroup.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
> index b6a6eeb..0fee061 100644
> --- a/include/net/cls_cgroup.h
> +++ b/include/net/cls_cgroup.h
> @@ -29,7 +29,7 @@ extern void sock_update_classid(struct sock *sk);
>  #if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
>  static inline u32 task_cls_classid(struct task_struct *p)
>  {
> -	int classid;
> +	u32 classid;
>  
>  	if (in_interrupt())
>  		return 0;
> -- 
> 1.8.0.rc0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH v1 3/5] cgroup: net_cls: Remove rcu_read_lock/unlock

[Neil Horman]

On Thu, Oct 25, 2012 at 04:16:57PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> As Eric pointed out:
> "Hey task_cls_classid() has its own rcu protection since commit
> 3fb5a991916091a908d (cls_cgroup: Fix rcu lockdep warning)
> 
> So we can safely revert Paul commit (1144182a8757f2a1)
> (We no longer need rcu_read_lock/unlock here)"
> 
> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Paul E. McKenney <paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  net/core/sock.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index c49412c..9fedbbf 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1221,9 +1221,7 @@ void sock_update_classid(struct sock *sk)
>  {
>  	u32 classid;
>  
> -	rcu_read_lock();  /* doing current task, which cannot vanish. */
>  	classid = task_cls_classid(current);
> -	rcu_read_unlock();
>  	if (classid != sk->sk_classid)
>  		sk->sk_classid = classid;
>  }
> -- 
> 1.8.0.rc0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Re: [PATCH v1 4/5] cgroup: net_cls: Pass in task to sock_update_classid()

[Neil Horman]

On Thu, Oct 25, 2012 at 04:16:58PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> sock_update_classid() assumes that the update operation always are
> applied on the current task. sock_update_classid() needs to know on
> which tasks to work on in order to be able to migrate task between
> cgroups using the struct cgroup_subsys attach() callback.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/net/tun.c        | 2 +-
>  include/net/cls_cgroup.h | 2 +-
>  net/core/sock.c          | 6 +++---
>  net/socket.c             | 8 ++++----
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Re: [PATCH v1 5/5] cgroup: net_cls: Rework update socket logic

[Neil Horman]

On Thu, Oct 25, 2012 at 04:16:59PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
> 
> The cgroup logic part of net_cls is very similar as the one in
> net_prio. Let's stream line the net_cls logic with the net_prio one.
> 
> The net_prio update logic was changed by following commit (note there
> were some changes necessary later on)
> 
> commit 406a3c638ce8b17d9704052c07955490f732c2b8
> Author: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date:   Fri Jul 20 10:39:25 2012 +0000
> 
>     net: netprio_cgroup: rework update socket logic
> 
>     Instead of updating the sk_cgrp_prioidx struct field on every send
>     this only updates the field when a task is moved via cgroup
>     infrastructure.
> 
>     This allows sockets that may be used by a kernel worker thread
>     to be managed. For example in the iscsi case today a user can
>     put iscsid in a netprio cgroup and control traffic will be sent
>     with the correct sk_cgrp_prioidx value set but as soon as data
>     is sent the kernel worker thread isssues a send and sk_cgrp_prioidx
>     is updated with the kernel worker threads value which is the
>     default case.
> 
>     It seems more correct to only update the field when the user
>     explicitly sets it via control group infrastructure. This allows
>     the users to manage sockets that may be used with other threads.
> 
> Since classid is now updated when the task is moved between the
> cgroups, we don't have to call sock_update_classid() from various
> places to ensure we always using the latest classid value.
> 
> [v2: Use iterate_fd() instead of open coding]
> 
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Re: [PATCH v1 1/5] cgroup: net_prio: Mark local used function static

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Thu, 25 Oct 2012 14:28:58 -0400

> On Thu, Oct 25, 2012 at 04:16:55PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> 
>> net_prio_attach() is only access via cgroup_subsys callbacks,
>> therefore we can reduce the visibility of this function.
>> 
>> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
 ...
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Re: [PATCH v1 2/5] cgroup: net_cls: Fix local variable type decleration

[David Miller]

From: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Thu, 25 Oct 2012 14:35:16 -0400

> On Thu, Oct 25, 2012 at 04:16:56PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>> 
>> The classid type used throughout the kernel is u32.
>> 
>> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
 ...
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Applied.
 

Re: [PATCH v1 3/5] cgroup: net_cls: Remove rcu_read_lock/unlock

[David Miller]

From: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Thu, 25 Oct 2012 14:42:52 -0400

> On Thu, Oct 25, 2012 at 04:16:57PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>> 
>> As Eric pointed out:
>> "Hey task_cls_classid() has its own rcu protection since commit
>> 3fb5a991916091a908d (cls_cgroup: Fix rcu lockdep warning)
>> 
>> So we can safely revert Paul commit (1144182a8757f2a1)
>> (We no longer need rcu_read_lock/unlock here)"
>> 
>> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
 ...
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Applied.

Re: [PATCH v1 4/5] cgroup: net_cls: Pass in task to sock_update_classid()

[David Miller]

From: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Thu, 25 Oct 2012 14:43:17 -0400

> On Thu, Oct 25, 2012 at 04:16:58PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>> 
>> sock_update_classid() assumes that the update operation always are
>> applied on the current task. sock_update_classid() needs to know on
>> which tasks to work on in order to be able to migrate task between
>> cgroups using the struct cgroup_subsys attach() callback.
>> 
>> Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
 ...
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Applied.

Re: [PATCH v1 5/5] cgroup: net_cls: Rework update socket logic

[David Miller]

From: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Thu, 25 Oct 2012 15:07:34 -0400

> On Thu, Oct 25, 2012 at 04:16:59PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>> 
>> The cgroup logic part of net_cls is very similar as the one in
>> net_prio. Let's stream line the net_cls logic with the net_prio one.
>> 
>> The net_prio update logic was changed by following commit (note there
>> were some changes necessary later on)
>> 
>> commit 406a3c638ce8b17d9704052c07955490f732c2b8
>> Author: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Date:   Fri Jul 20 10:39:25 2012 +0000
>> 
>>     net: netprio_cgroup: rework update socket logic
>> 
>>     Instead of updating the sk_cgrp_prioidx struct field on every send
>>     this only updates the field when a task is moved via cgroup
>>     infrastructure.
>> 
>>     This allows sockets that may be used by a kernel worker thread
>>     to be managed. For example in the iscsi case today a user can
>>     put iscsid in a netprio cgroup and control traffic will be sent
>>     with the correct sk_cgrp_prioidx value set but as soon as data
>>     is sent the kernel worker thread isssues a send and sk_cgrp_prioidx
>>     is updated with the kernel worker threads value which is the
>>     default case.
>> 
>>     It seems more correct to only update the field when the user
>>     explicitly sets it via control group infrastructure. This allows
>>     the users to manage sockets that may be used with other threads.
>> 
>> Since classid is now updated when the task is moved between the
>> cgroups, we don't have to call sock_update_classid() from various
>> places to ensure we always using the latest classid value.
>> 
>> [v2: Use iterate_fd() instead of open coding]
>> 
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Applied.