[PATCH 1/6] netprio_cgroup: fix an off-by-one bug

[PATCH 1/6] netprio_cgroup: fix an off-by-one bug

[Li Zefan]

  # mount -t cgroup xxx /mnt
  # mkdir /mnt/tmp
  # cat /mnt/tmp/net_prio.ifpriomap
  lo 0
  eth0 0
  virbr0 0
  # echo 'lo 999' > /mnt/tmp/net_prio.ifpriomap
  # cat /mnt/tmp/net_prio.ifpriomap
  lo 999
  eth0 0
  virbr0 4101267344

We got weired output, because we exceeded the boundary of the array.
We may even crash the kernel..

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 net/core/netprio_cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 3a9fd48..a296cbb 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -107,7 +107,7 @@ static void extend_netdev_table(struct net_device *dev, u32 new_len)
 static void update_netdev_tables(void)
 {
 	struct net_device *dev;
-	u32 max_len = atomic_read(&max_prioidx);
+	u32 max_len = atomic_read(&max_prioidx) + 1;
 	struct netprio_map *map;
 
 	rtnl_lock();
-- 
1.7.3.1

[PATCH 3/6] netprio_cgroup: fix wrong memory access when NETPRIO_CGROUP=m

[Li Zefan]

When the netprio_cgroup module is not loaded, net_prio_subsys_id
is -1, and so sock_update_prioidx() accesses cgroup_subsys array
with negative index subsys[-1].

Make the code resembles cls_cgroup code, which is bug free.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/net/netprio_cgroup.h |   48 +++++++++++++++++++++++++++++++++++-------
 net/core/sock.c              |    7 +----
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index 7b2d431..d58fdec 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -37,19 +37,51 @@ extern int net_prio_subsys_id;
 
 extern void sock_update_netprioidx(struct sock *sk);
 
-static inline struct cgroup_netprio_state
-		*task_netprio_state(struct task_struct *p)
+#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
+
+static inline u32 task_netprioidx(struct task_struct *p)
 {
-#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
-	return container_of(task_subsys_state(p, net_prio_subsys_id),
-			    struct cgroup_netprio_state, css);
-#else
-	return NULL;
-#endif
+	struct cgroup_netprio_state *state;
+	u32 idx;
+
+	rcu_read_lock();
+	state = container_of(task_subsys_state(p, net_prio_subsys_id),
+			     struct cgroup_netprio_state, css);
+	idx = state->prioidx;
+	rcu_read_unlock();
+	return idx;
+}
+
+#elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
+
+static inline u32 task_netprioidx(struct task_struct *p)
+{
+	struct cgroup_netprio_state *state;
+	int subsys_id;
+	u32 idx = 0;
+
+	rcu_read_lock();
+	subsys_id = rcu_dereference_index_check(net_prio_subsys_id,
+						rcu_read_lock_held());
+	if (subsys_id >= 0) {
+		state = container_of(task_subsys_state(p, subsys_id),
+				     struct cgroup_netprio_state, css);
+		idx = state->prioidx;
+	}
+	rcu_read_unlock();
+	return idx;
 }
 
 #else
 
+static inline u32 task_netprioidx(struct task_struct *p)
+{
+	return 0;
+}
+
+#endif /* CONFIG_NETPRIO_CGROUP */
+
+#else
 #define sock_update_netprioidx(sk)
 #endif
 
diff --git a/net/core/sock.c b/net/core/sock.c
index 3e81fd2..02f8dfe 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1171,13 +1171,10 @@ EXPORT_SYMBOL(sock_update_classid);
 
 void sock_update_netprioidx(struct sock *sk)
 {
-	struct cgroup_netprio_state *state;
 	if (in_interrupt())
 		return;
-	rcu_read_lock();
-	state = task_netprio_state(current);
-	sk->sk_cgrp_prioidx = state ? state->prioidx : 0;
-	rcu_read_unlock();
+
+	sk->sk_cgrp_prioidx = task_netprioidx(current);
 }
 EXPORT_SYMBOL_GPL(sock_update_netprioidx);
 #endif
-- 
1.7.3.1

[PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[Li Zefan]

- remove unused net_prio_subsys_id and sock->sk_cgrp->prioidx
when CGROUPS=y and NETPRIO_CGROUP=n

- don't use "ifndef CONFIG_NETPRIO_CGROUP" to suggest it's
a module, use IS_MODULE().

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/net/netprio_cgroup.h |   23 ++++++-----------------
 include/net/sock.h           |    2 +-
 net/core/netprio_cgroup.c    |    7 +++----
 net/core/sock.c              |    7 +++++--
 4 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index d58fdec..8d09944 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -24,18 +24,16 @@ struct netprio_map {
 	u32 priomap[];
 };
 
-#ifdef CONFIG_CGROUPS
-
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 struct cgroup_netprio_state {
 	struct cgroup_subsys_state css;
 	u32 prioidx;
 };
 
-#ifndef CONFIG_NETPRIO_CGROUP
-extern int net_prio_subsys_id;
-#endif
-
 extern void sock_update_netprioidx(struct sock *sk);
+#else
+static inline void sock_update_netprioidx(struct sock *sk) {}
+#endif
 
 #if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 
@@ -54,6 +52,8 @@ static inline u32 task_netprioidx(struct task_struct *p)
 
 #elif IS_MODULE(CONFIG_NETPRIO_CGROUP)
 
+extern int net_prio_subsys_id;
+
 static inline u32 task_netprioidx(struct task_struct *p)
 {
 	struct cgroup_netprio_state *state;
@@ -72,17 +72,6 @@ static inline u32 task_netprioidx(struct task_struct *p)
 	return idx;
 }
 
-#else
-
-static inline u32 task_netprioidx(struct task_struct *p)
-{
-	return 0;
-}
-
 #endif /* CONFIG_NETPRIO_CGROUP */
 
-#else
-#define sock_update_netprioidx(sk)
-#endif
-
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/include/net/sock.h b/include/net/sock.h
index 91c1c8b..b08a44e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -342,7 +342,7 @@ struct sock {
 	unsigned short		sk_ack_backlog;
 	unsigned short		sk_max_ack_backlog;
 	__u32			sk_priority;
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 	__u32			sk_cgrp_prioidx;
 #endif
 	struct pid		*sk_peer_pid;
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 2edfa6b..95ab29a 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -33,7 +33,7 @@ struct cgroup_subsys net_prio_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.populate	= cgrp_populate,
-#ifdef CONFIG_NETPRIO_CGROUP
+#if IS_BUILTIN(CONFIG_NETPRIO_CGROUP)
 	.subsys_id	= net_prio_subsys_id,
 #endif
 	.module		= THIS_MODULE
@@ -298,7 +298,7 @@ static int __init init_cgroup_netprio(void)
 	ret = cgroup_load_subsys(&net_prio_subsys);
 	if (ret)
 		goto out;
-#ifndef CONFIG_NETPRIO_CGROUP
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	smp_wmb();
 	net_prio_subsys_id = net_prio_subsys.subsys_id;
 #endif
@@ -318,11 +318,10 @@ static void __exit exit_cgroup_netprio(void)
 
 	cgroup_unload_subsys(&net_prio_subsys);
 
-#ifndef CONFIG_NETPRIO_CGROUP
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 	net_prio_subsys_id = -1;
 	synchronize_rcu();
 #endif
-
 	rtnl_lock();
 	for_each_netdev(&init_net, dev) {
 		old = rtnl_dereference(dev->priomap);
diff --git a/net/core/sock.c b/net/core/sock.c
index 02f8dfe..76df203 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -272,11 +272,12 @@ EXPORT_SYMBOL(sysctl_optmem_max);
 int net_cls_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_cls_subsys_id);
 #endif
-#if !defined(CONFIG_NETPRIO_CGROUP)
+#endif
+
+#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 int net_prio_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_prio_subsys_id);
 #endif
-#endif
 
 static int sock_set_timeout(long *timeo_p, char __user *optval, int optlen)
 {
@@ -1168,7 +1169,9 @@ void sock_update_classid(struct sock *sk)
 		sk->sk_classid = classid;
 }
 EXPORT_SYMBOL(sock_update_classid);
+#endif
 
+#if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 void sock_update_netprioidx(struct sock *sk)
 {
 	if (in_interrupt())
-- 
1.7.3.1

Re: [PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[David Miller]

Do not mix genuine bug fixes and cleanups.

Otherwise I cannot apply your bug fixes to the 'net' tree.

Seperate things out, submit only pure bug fixes first, then
later once those changes propagate you can submit the cleanups.

Re: [PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[Li Zefan]

14:59, David Miller wrote:
> 
> Do not mix genuine bug fixes and cleanups.
> 
> Otherwise I cannot apply your bug fixes to the 'net' tree.
> 
> Seperate things out, submit only pure bug fixes first, then
> later once those changes propagate you can submit the cleanups.
> 

I did seperate them out, so the first three patches are fixes,
and others are cleanups. I can resend cleanup patches once
fixes hit mainline.

Re: [PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[David Miller]

From: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Wed, 01 Feb 2012 15:06:53 +0800

> 14:59, David Miller wrote:
>> 
>> Do not mix genuine bug fixes and cleanups.
>> 
>> Otherwise I cannot apply your bug fixes to the 'net' tree.
>> 
>> Seperate things out, submit only pure bug fixes first, then
>> later once those changes propagate you can submit the cleanups.
>> 
> 
> I did seperate them out, so the first three patches are fixes,
> and others are cleanups. I can resend cleanup patches once
> fixes hit mainline.

Seperate them in different groups, don't submit them as one
collection.  And definitely don't submit them all at the
same damn time!  You have to wait with the dependent cleanups
until the fixes go in, and subsequently show up in my net-next
tree.

I'm not applying this stuff until you submit it properly, in
two stages.

Re: [PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[Li Zefan]

>>> Do not mix genuine bug fixes and cleanups.
>>>
>>> Otherwise I cannot apply your bug fixes to the 'net' tree.
>>>
>>> Seperate things out, submit only pure bug fixes first, then
>>> later once those changes propagate you can submit the cleanups.
>>>
>>
>> I did seperate them out, so the first three patches are fixes,
>> and others are cleanups. I can resend cleanup patches once
>> fixes hit mainline.
> 
> Seperate them in different groups, don't submit them as one
> collection.  And definitely don't submit them all at the
> same damn time!  You have to wait with the dependent cleanups
> until the fixes go in, and subsequently show up in my net-next
> tree.
> 
> I'm not applying this stuff until you submit it properly, in
> two stages.
> 

I'll do this when I get some comments/acks.

Re: [PATCH 4/6] netprio_cgroup: use IS_ENABLED() and family

[Neil Horman]

On Wed, Feb 01, 2012 at 03:22:59PM +0800, Li Zefan wrote:
> >>> Do not mix genuine bug fixes and cleanups.
> >>>
> >>> Otherwise I cannot apply your bug fixes to the 'net' tree.
> >>>
> >>> Seperate things out, submit only pure bug fixes first, then
> >>> later once those changes propagate you can submit the cleanups.
> >>>
> >>
> >> I did seperate them out, so the first three patches are fixes,
> >> and others are cleanups. I can resend cleanup patches once
> >> fixes hit mainline.
> > 
> > Seperate them in different groups, don't submit them as one
> > collection.  And definitely don't submit them all at the
> > same damn time!  You have to wait with the dependent cleanups
> > until the fixes go in, and subsequently show up in my net-next
> > tree.
> > 
> > I'm not applying this stuff until you submit it properly, in
> > two stages.
> > 
> 
> I'll do this when I get some comments/acks.
> 

The changes look fine to me.
I'll ack them once you submit them properly.
Thanks
Neil

[PATCH 2/6] netprio_cgroup: don't allocate prio table when a device is registered

[Li Zefan]

So we delay the allocation till the priority is set through cgroup,
and this makes skb_update_priority() faster when it's not set.

This also eliminates an off-by-one bug similar with the one fixed
in the previous patch.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 net/core/netprio_cgroup.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index a296cbb..2edfa6b 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -270,7 +270,6 @@ static int netprio_device_event(struct notifier_block *unused,
 {
 	struct net_device *dev = ptr;
 	struct netprio_map *old;
-	u32 max_len = atomic_read(&max_prioidx);
 
 	/*
 	 * Note this is called with rtnl_lock held so we have update side
@@ -278,11 +277,6 @@ static int netprio_device_event(struct notifier_block *unused,
 	 */
 
 	switch (event) {
-
-	case NETDEV_REGISTER:
-		if (max_len)
-			extend_netdev_table(dev, max_len);
-		break;
 	case NETDEV_UNREGISTER:
 		old = rtnl_dereference(dev->priomap);
 		RCU_INIT_POINTER(dev->priomap, NULL);
-- 
1.7.3.1

[PATCH 5/6] cls_cgroup: use IS_ENABLED() and family

[Li Zefan]

- remove net_cls_subsys_id when CGROUPS=y && !NET_CLS_CGRUOP
- remove sock->classid when !NET_CLS_CGROUP
- don't use "ifndef CONFIG_NET_CLS_CGROUP" to sugguest it's
a module, use IS_MODULE().

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/net/cls_cgroup.h |   12 ++++--------
 include/net/sock.h       |    4 +++-
 net/core/sock.c          |    6 ++----
 net/sched/cls_cgroup.c   |    6 +++---
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index a4dc5b0..806b9a1 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -17,14 +17,14 @@
 #include <linux/hardirq.h>
 #include <linux/rcupdate.h>
 
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 struct cgroup_cls_state
 {
 	struct cgroup_subsys_state css;
 	u32 classid;
 };
 
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 static inline u32 task_cls_classid(struct task_struct *p)
 {
 	int classid;
@@ -39,7 +39,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
 
 	return classid;
 }
-#else
+#elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
 extern int net_cls_subsys_id;
 
 static inline u32 task_cls_classid(struct task_struct *p)
@@ -61,10 +61,6 @@ static inline u32 task_cls_classid(struct task_struct *p)
 	return classid;
 }
 #endif
-#else
-static inline u32 task_cls_classid(struct task_struct *p)
-{
-	return 0;
-}
+
 #endif
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/include/net/sock.h b/include/net/sock.h
index b08a44e..7b80d55 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -362,7 +362,9 @@ struct sock {
 	void			*sk_security;
 #endif
 	__u32			sk_mark;
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 	u32			sk_classid;
+#endif
 	struct cg_proto		*sk_cgrp;
 	void			(*sk_state_change)(struct sock *sk);
 	void			(*sk_data_ready)(struct sock *sk, int bytes);
@@ -1382,7 +1384,7 @@ extern void *sock_kmalloc(struct sock *sk, int size,
 extern void sock_kfree_s(struct sock *sk, void *mem, int size);
 extern void sk_send_sigurg(struct sock *sk);
 
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 extern void sock_update_classid(struct sock *sk);
 #else
 static inline void sock_update_classid(struct sock *sk)
diff --git a/net/core/sock.c b/net/core/sock.c
index 76df203..213c856 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -267,12 +267,10 @@ __u32 sysctl_rmem_default __read_mostly = SK_RMEM_MAX;
 int sysctl_optmem_max __read_mostly = sizeof(unsigned long)*(2*UIO_MAXIOV+512);
 EXPORT_SYMBOL(sysctl_optmem_max);
 
-#if defined(CONFIG_CGROUPS)
-#if !defined(CONFIG_NET_CLS_CGROUP)
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 int net_cls_subsys_id = -1;
 EXPORT_SYMBOL_GPL(net_cls_subsys_id);
 #endif
-#endif
 
 #if IS_MODULE(CONFIG_NETPRIO_CGROUP)
 int net_prio_subsys_id = -1;
@@ -1157,7 +1155,7 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 	module_put(owner);
 }
 
-#ifdef CONFIG_CGROUPS
+#if IS_ENABLED(CONFIG_NET_CLS_CGROUP)
 void sock_update_classid(struct sock *sk)
 {
 	u32 classid;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index f84fdc3..f57245c 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -32,7 +32,7 @@ struct cgroup_subsys net_cls_subsys = {
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.populate	= cgrp_populate,
-#ifdef CONFIG_NET_CLS_CGROUP
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
 	.subsys_id	= net_cls_subsys_id,
 #endif
 	.module		= THIS_MODULE,
@@ -294,7 +294,7 @@ static int __init init_cgroup_cls(void)
 	if (ret)
 		goto out;
 
-#ifndef CONFIG_NET_CLS_CGROUP
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 	/* We can't use rcu_assign_pointer because this is an int. */
 	smp_wmb();
 	net_cls_subsys_id = net_cls_subsys.subsys_id;
@@ -312,7 +312,7 @@ static void __exit exit_cgroup_cls(void)
 {
 	unregister_tcf_proto_ops(&cls_cgroup_ops);
 
-#ifndef CONFIG_NET_CLS_CGROUP
+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
 	net_cls_subsys_id = -1;
 	synchronize_rcu();
 #endif
-- 
1.7.3.1

[PATCH 6/6] cls_cgroup: remove redundant rcu_read_lock/unlock

[Li Zefan]

We've already used rcu_read_lock/unlock inside task_classid(),
so don't use the lock/unlock pair twice in this hot path.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 net/core/sock.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 213c856..c0bab23 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1160,9 +1160,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 && classid != sk->sk_classid)
 		sk->sk_classid = classid;
 }
-- 
1.7.3.1

Re: [PATCH 6/6] cls_cgroup: remove redundant rcu_read_lock/unlock

[Eric Dumazet]

Le mercredi 01 février 2012 à 14:56 +0800, Li Zefan a écrit :
> We've already used rcu_read_lock/unlock inside task_classid(),
> so don't use the lock/unlock pair twice in this hot path.
> 
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> ---
>  net/core/sock.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 213c856..c0bab23 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1160,9 +1160,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 && classid != sk->sk_classid)
>  		sk->sk_classid = classid;

Yes, this seems fine.

Then, I wonder why we do the "if (classid && classid != sk->sk_classid)"

before the :

	sk->sk_classid = classid;

This seems unnecessary checks.

Re: [PATCH 6/6] cls_cgroup: remove redundant rcu_read_lock/unlock

[David Miller]

From: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Wed, 01 Feb 2012 08:07:19 +0100

> Then, I wonder why we do the "if (classid && classid != sk->sk_classid)"
> 
> before the :
> 
> 	sk->sk_classid = classid;
> 
> This seems unnecessary checks.
> 

Avoiding dirtying the sk->sk_classid cache line unnecessarily?

I actually have no idea actually how often this routine can get
invoked in real world scenerios.

Re: [PATCH 6/6] cls_cgroup: remove redundant rcu_read_lock/unlock

[Li Zefan]

Cc: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>

>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 213c856..c0bab23 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -1160,9 +1160,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 && classid != sk->sk_classid)
>>  		sk->sk_classid = classid;
> 
> Yes, this seems fine.
> 
> Then, I wonder why we do the "if (classid && classid != sk->sk_classid)"
> 
> before the :
> 
> 	sk->sk_classid = classid;
> 
> This seems unnecessary checks.
> 

I was wondering about this too. He who added this may provide us with an
answer.

Re: [PATCH 6/6] cls_cgroup: remove redundant rcu_read_lock/unlock

[Herbert Xu]

On Wed, Feb 01, 2012 at 03:20:00PM +0800, Li Zefan wrote:
> Cc: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
> 
> >> diff --git a/net/core/sock.c b/net/core/sock.c
> >> index 213c856..c0bab23 100644
> >> --- a/net/core/sock.c
> >> +++ b/net/core/sock.c
> >> @@ -1160,9 +1160,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 && classid != sk->sk_classid)
> >>  		sk->sk_classid = classid;
> > 
> > Yes, this seems fine.
> > 
> > Then, I wonder why we do the "if (classid && classid != sk->sk_classid)"
> > 
> > before the :
> > 
> > 	sk->sk_classid = classid;
> > 
> > This seems unnecessary checks.
> > 
> 
> I was wondering about this too. He who added this may provide us with an
> answer.

Well writing a cache-line unnecessarily is bad.

Cheers,
-- 
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt