netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v0 0/4] Rework net_cls update socket logic
@ 2012-10-17 13:04 Daniel Wagner
       [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
  2012-10-17 13:04 ` [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid() Daniel Wagner
  0 siblings, 2 replies; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 13:04 UTC (permalink / raw)
  To: netdev, cgroups
  Cc: Daniel Wagner, Li Zefan, David S. Miller, Michael S. Tsirkin,
	Eric Dumazet, Glauber Costa, Jamal Hadi Salim, Joe Perches,
	John Fastabend, Neil Horman, Stanislav Kinsbursky, Tejun Heo

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

Hi,

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.

cheers,
daniel

Cc:  Li Zefan <lizefan@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Joe Perches <joe@perches.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: <cgroups@vger.kernel.org>
Cc: <netdev@vger.kernel.org>

Daniel Wagner (4):
  cgroup: net_prio: Mark local used function static
  cgroup: net_cls: Fix local variable type decleration
  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           |  6 +++---
 net/sched/cls_cgroup.c    | 38 ++++++++++++++++++++++++++++++++++++++
 net/socket.c              |  8 --------
 6 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.11.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v0 1/4] cgroup: net_prio: Mark local used function static
       [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
@ 2012-10-17 13:04   ` Daniel Wagner
  2012-10-17 13:04   ` [PATCH v0 2/4] cgroup: net_cls: Fix local variable type decleration Daniel Wagner
  2012-10-17 13:04   ` [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic Daniel Wagner
  2 siblings, 0 replies; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 13:04 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA
  Cc: Daniel Wagner, David S. Miller, John Fastabend, Li Zefan,
	Neil Horman, Tejun Heo

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 4a83fb3..5ac384e 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -239,7 +239,7 @@ out_free_devname:
 	return ret;
 }
 
-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;
 
-- 
1.7.11.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v0 2/4] cgroup: net_cls: Fix local variable type decleration
       [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
  2012-10-17 13:04   ` [PATCH v0 1/4] cgroup: net_prio: Mark local used function static Daniel Wagner
@ 2012-10-17 13:04   ` Daniel Wagner
  2012-10-17 13:04   ` [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic Daniel Wagner
  2 siblings, 0 replies; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 13:04 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA
  Cc: Daniel Wagner, David S. Miller, Li Zefan, Tejun Heo

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>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@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>
---
 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.7.11.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid()
  2012-10-17 13:04 [PATCH v0 0/4] Rework net_cls update socket logic Daniel Wagner
       [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
@ 2012-10-17 13:04 ` Daniel Wagner
  2012-10-17 13:37   ` Eric Dumazet
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 13:04 UTC (permalink / raw)
  To: netdev, cgroups
  Cc: Daniel Wagner, David S. Miller, Michael S. Tsirkin, Eric Dumazet,
	Glauber Costa, Joe Perches, Neil Horman, Stanislav Kinsbursky,
	Tejun Heo

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

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@bmw-carit.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Glauber Costa <glommer@parallels.com>
Cc: Joe Perches <joe@perches.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        | 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 8a146cf..68c2f3b 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1214,12 +1214,12 @@ 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;
 
 	rcu_read_lock();  /* doing current task, which cannot vanish. */
-	classid = task_cls_classid(current);
+	classid = task_cls_classid(task);
 	rcu_read_unlock();
 	if (classid != sk->sk_classid)
 		sk->sk_classid = classid;
@@ -1263,7 +1263,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 80dc7e8..b38aff9 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -625,7 +625,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;
@@ -789,7 +789,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;
@@ -901,7 +901,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);
 }
@@ -3421,7 +3421,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.7.11.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic
       [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
  2012-10-17 13:04   ` [PATCH v0 1/4] cgroup: net_prio: Mark local used function static Daniel Wagner
  2012-10-17 13:04   ` [PATCH v0 2/4] cgroup: net_cls: Fix local variable type decleration Daniel Wagner
@ 2012-10-17 13:04   ` Daniel Wagner
       [not found]     ` <1350479078-29361-5-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
  2 siblings, 1 reply; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 13:04 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA
  Cc: Daniel Wagner, Li Zefan, David S. Miller, Michael S. Tsirkin,
	Jamal Hadi Salim, Joe Perches, John Fastabend, Neil Horman,
	Stanislav Kinsbursky, Tejun Heo

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.

Signed-off-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Cc:  Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@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: Jamal Hadi Salim <jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Cc: John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@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      |  3 ---
 net/sched/cls_cgroup.c | 38 ++++++++++++++++++++++++++++++++++++++
 net/socket.c           |  8 --------
 3 files changed, 38 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..789edb8 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,42 @@ static void cgrp_destroy(struct cgroup *cgrp)
 	kfree(cgrp_cls_state(cgrp));
 }
 
+static void cgrp_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+{
+	struct task_struct *p;
+
+	cgroup_taskset_for_each(p, cgrp, tset) {
+		unsigned int fd;
+		struct fdtable *fdt;
+		struct files_struct *files;
+
+		task_lock(p);
+		files = p->files;
+		if (!files) {
+			task_unlock(p);
+			continue;
+		}
+
+		spin_lock(&files->file_lock);
+		fdt = files_fdtable(files);
+		for (fd = 0; fd < fdt->max_fds; fd++) {
+			struct file *file;
+			struct socket *sock;
+			int err;
+
+			file = fcheck_files(files, fd);
+			if (!file)
+				continue;
+
+			sock = sock_from_file(file, &err);
+			if (sock)
+				sock_update_classid(sock->sk, p);
+		}
+		spin_unlock(&files->file_lock);
+		task_unlock(p);
+	}
+}
+
 static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 {
 	return cgrp_cls_state(cgrp)->classid;
@@ -77,6 +114,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 b38aff9..d296648 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -625,8 +625,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;
@@ -789,8 +787,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;
@@ -901,8 +897,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);
 }
 
@@ -3421,8 +3415,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.7.11.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid()
  2012-10-17 13:04 ` [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid() Daniel Wagner
@ 2012-10-17 13:37   ` Eric Dumazet
  2012-10-17 15:54     ` Daniel Wagner
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2012-10-17 13:37 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: netdev, cgroups, Daniel Wagner, David S. Miller,
	Michael S. Tsirkin, Eric Dumazet, Glauber Costa, Joe Perches,
	Neil Horman, Stanislav Kinsbursky, Tejun Heo

On Wed, 2012-10-17 at 15:04 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> 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.
> 
...

>  
> -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 8a146cf..68c2f3b 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1214,12 +1214,12 @@ 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;
>  
>  	rcu_read_lock();  /* doing current task, which cannot vanish. */
> -	classid = task_cls_classid(current);
> +	classid = task_cls_classid(task);
>  	rcu_read_unlock();


Comment is now misleading (since task might not be current) and
rcu_read_lock()/rcu_read_unlock() protects nothing here, but avoid a
lockdep splat...

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)

(BTW net-next is not opened yet, its content outdated, although I like
your patch series !)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic
       [not found]     ` <1350479078-29361-5-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
@ 2012-10-17 14:02       ` John Fastabend
       [not found]         ` <507EBA59.9080307-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: John Fastabend @ 2012-10-17 14:02 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Daniel Wagner, Li Zefan, David S. Miller, Michael S. Tsirkin,
	Jamal Hadi Salim, Joe Perches, Neil Horman, Stanislav Kinsbursky,
	Tejun Heo

On 10/17/2012 6:04 AM, 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.
>

[...]


>
> +static void cgrp_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
> +{
> +	struct task_struct *p;
> +
> +	cgroup_taskset_for_each(p, cgrp, tset) {
> +		unsigned int fd;
> +		struct fdtable *fdt;
> +		struct files_struct *files;
> +
> +		task_lock(p);
> +		files = p->files;
> +		if (!files) {
> +			task_unlock(p);
> +			continue;
> +		}
> +
> +		spin_lock(&files->file_lock);
> +		fdt = files_fdtable(files);
> +		for (fd = 0; fd < fdt->max_fds; fd++) {
> +			struct file *file;
> +			struct socket *sock;
> +			int err;
> +
> +			file = fcheck_files(files, fd);
> +			if (!file)
> +				continue;
> +
> +			sock = sock_from_file(file, &err);
> +			if (sock)
> +				sock_update_classid(sock->sk, p);
> +		}
> +		spin_unlock(&files->file_lock);

This block should probably use iterate_fd() instead of open coding
this. See the latest net_prio_attach().

.John

> +		task_unlock(p);
> +	}
> +}
> +

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid()
  2012-10-17 13:37   ` Eric Dumazet
@ 2012-10-17 15:54     ` Daniel Wagner
       [not found]       ` <507ED4C0.7070408-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 15:54 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Daniel Wagner, David S. Miller, Michael S. Tsirkin, Eric Dumazet,
	Glauber Costa, Joe Perches, Neil Horman, Stanislav Kinsbursky,
	Tejun Heo

On 17.10.2012 15:37, Eric Dumazet wrote:
> On Wed, 2012-10-17 at 15:04 +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.
>>
> ...
>
>>
>> -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 8a146cf..68c2f3b 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -1214,12 +1214,12 @@ 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;
>>
>>   	rcu_read_lock();  /* doing current task, which cannot vanish. */
>> -	classid = task_cls_classid(current);
>> +	classid = task_cls_classid(task);
>>   	rcu_read_unlock();
>
>
> Comment is now misleading (since task might not be current) and
> rcu_read_lock()/rcu_read_unlock() protects nothing here, but avoid a
> lockdep splat...
>
> 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)

Good point. Will update the series accordingly.

> (BTW net-next is not opened yet, its content outdated, although I like
> your patch series !)

I still have not figured all details on the patch submission procedure. 
Sorry about that. The time to send patches for net-next will be annonced 
by David, is this correct?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic
       [not found]         ` <507EBA59.9080307-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2012-10-17 15:56           ` Daniel Wagner
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Wagner @ 2012-10-17 15:56 UTC (permalink / raw)
  To: John Fastabend
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Daniel Wagner, Li Zefan, David S. Miller, Michael S. Tsirkin,
	Jamal Hadi Salim, Joe Perches, Neil Horman, Stanislav Kinsbursky,
	Tejun Heo

On 17.10.2012 16:02, John Fastabend wrote:
> On 10/17/2012 6:04 AM, 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.

[...]

> This block should probably use iterate_fd() instead of open coding
> this. See the latest net_prio_attach().

Thanks for the heads up. Will update the patch accordingly.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid()
       [not found]       ` <507ED4C0.7070408-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
@ 2012-10-17 16:15         ` Eric Dumazet
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2012-10-17 16:15 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Daniel Wagner, David S. Miller, Michael S. Tsirkin, Eric Dumazet,
	Glauber Costa, Joe Perches, Neil Horman, Stanislav Kinsbursky,
	Tejun Heo

On Wed, 2012-10-17 at 17:54 +0200, Daniel Wagner wrote:

> I still have not figured all details on the patch submission procedure. 
> Sorry about that. The time to send patches for net-next will be annonced 
> by David, is this correct?
> 

Exactly. David sent a mail yesterday :

On Mon, 2012-10-15 at 23:19 -0400, David Miller wrote:
> Please stop sending feature patches and cleanups.
> 
> I'll be sure to let the list know when net-next is
> ready for submissions again, thanks.
> 
> Thank you.
> -- 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-10-17 16:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-17 13:04 [PATCH v0 0/4] Rework net_cls update socket logic Daniel Wagner
     [not found] ` <1350479078-29361-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
2012-10-17 13:04   ` [PATCH v0 1/4] cgroup: net_prio: Mark local used function static Daniel Wagner
2012-10-17 13:04   ` [PATCH v0 2/4] cgroup: net_cls: Fix local variable type decleration Daniel Wagner
2012-10-17 13:04   ` [PATCH v0 4/4] cgroup: net_cls: Rework update socket logic Daniel Wagner
     [not found]     ` <1350479078-29361-5-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
2012-10-17 14:02       ` John Fastabend
     [not found]         ` <507EBA59.9080307-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2012-10-17 15:56           ` Daniel Wagner
2012-10-17 13:04 ` [PATCH v0 3/4] cgroup: net_cls: Pass in task to sock_update_classid() Daniel Wagner
2012-10-17 13:37   ` Eric Dumazet
2012-10-17 15:54     ` Daniel Wagner
     [not found]       ` <507ED4C0.7070408-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
2012-10-17 16:15         ` Eric Dumazet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).