Message ID | 1345816904-21745-4-git-send-email-wagi@monom.org |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote: > @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void) > synchronize_rcu(); > #endif > > +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > + static_key_slow_dec(&cgroup_cls_enabled); > + rcu_barrier(); Why is this rcu_barrier() necessary? In general, please explain what synchronization is going on when using sync constructs which aren't obvious - e.g. memory barriers, rcu barriers. Thanks.
On 25.08.2012 01:26, Tejun Heo wrote: > On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote: >> @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void) >> synchronize_rcu(); >> #endif >> >> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) >> + static_key_slow_dec(&cgroup_cls_enabled); >> + rcu_barrier(); > > Why is this rcu_barrier() necessary? I have read the rcubarrier.txt document and I got from that that an rcu_barrier() is needed when unloading a module. But maybe I got it wrong. So the idea after disabling the jump lables all pending readers in task_cls_classid() have left. THe same thing is done in the old code with the dynamic id part. With the difference that synchronize_rcu() is used. > In general, please explain what > synchronization is going on when using sync constructs which aren't > obvious - e.g. memory barriers, rcu barriers. Sure, I will keep this in mind. -- 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
On Sat, Aug 25, 2012 at 06:56:29PM +0200, Daniel Wagner wrote: > On 25.08.2012 01:26, Tejun Heo wrote: > >On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote: > >>@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void) > >> synchronize_rcu(); > >> #endif > >> > >>+#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > >>+ static_key_slow_dec(&cgroup_cls_enabled); > >>+ rcu_barrier(); > > > >Why is this rcu_barrier() necessary? > > I have read the rcubarrier.txt document and I got from that that an > rcu_barrier() is needed when unloading a module. But maybe I got it > wrong. > > So the idea after disabling the jump lables all pending readers in > task_cls_classid() have left. THe same thing is done in the old code > with the dynamic id part. With the difference that synchronize_rcu() > is used. FWIW, the rcu_barrier() is needed only if the module uses call_rcu(). In that case it is required to ensure that all the resulting callbacks execute before the module's .text is freed up. Thanx, Paul > >In general, please explain what > >synchronization is going on when using sync constructs which aren't > >obvious - e.g. memory barriers, rcu barriers. > > Sure, I will keep this in mind. > > -- > 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 > -- 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
On Tue, Aug 28, 2012 at 07:47:25AM -0700, Paul E. McKenney wrote: > On Sat, Aug 25, 2012 at 06:56:29PM +0200, Daniel Wagner wrote: > > On 25.08.2012 01:26, Tejun Heo wrote: > > >On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote: > > >>@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void) > > >> synchronize_rcu(); > > >> #endif > > >> > > >>+#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > > >>+ static_key_slow_dec(&cgroup_cls_enabled); > > >>+ rcu_barrier(); > > > > > >Why is this rcu_barrier() necessary? > > > > I have read the rcubarrier.txt document and I got from that that an > > rcu_barrier() is needed when unloading a module. But maybe I got it > > wrong. > > > > So the idea after disabling the jump lables all pending readers in > > task_cls_classid() have left. THe same thing is done in the old code > > with the dynamic id part. With the difference that synchronize_rcu() > > is used. > > FWIW, the rcu_barrier() is needed only if the module uses call_rcu(). > In that case it is required to ensure that all the resulting callbacks > execute before the module's .text is freed up. Thanks for the clarification. After reading the documentations again and I think this here should do the trick: static inline u32 task_cls_classid(struct task_struct *p) { struct cgroup_cls_state *cs; u32 classid = 0; if (!clscg_enabled || in_interrupt()) return 0; rcu_read_lock(); cs = container_of(task_subsys_state(p, net_cls_subsys_id), struct cgroup_cls_state, css); if (cs) classid = cs->classid; rcu_read_unlock(); return classid; } static void __exit exit_cgroup_cls(void) { unregister_tcf_proto_ops(&cls_cgroup_ops); #if IS_MODULE(CONFIG_NET_CLS_CGROUP) static_key_slow_dec(&cgroup_cls_enabled); synchronize_rcu(); #endif cgroup_unload_subsys(&net_cls_subsys); } So when unloading the module, we first disable the static branch, then we wait for all old readers leaving the reader side issuing a synchronize_rcu(). New readers might already have passed the static branch and now entering the reader side. So we need to test if the pointer we retrieve is valid. Basically, this change is using the same approach we had before. Instead looking at the id is valid we look at the pointer if it is valid. -- 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
diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h index 401672c..24443d2 100644 --- a/include/net/cls_cgroup.h +++ b/include/net/cls_cgroup.h @@ -16,6 +16,7 @@ #include <linux/cgroup.h> #include <linux/hardirq.h> #include <linux/rcupdate.h> +#include <linux/jump_label.h> #ifdef CONFIG_CGROUPS struct cgroup_cls_state @@ -45,6 +46,9 @@ static inline u32 task_cls_classid(struct task_struct *p) #elif IS_MODULE(CONFIG_NET_CLS_CGROUP) +extern struct static_key cgroup_cls_enabled; +#define clscg_enabled static_key_false(&cgroup_cls_enabled) + extern int net_cls_subsys_id; static inline u32 task_cls_classid(struct task_struct *p) @@ -52,7 +56,7 @@ static inline u32 task_cls_classid(struct task_struct *p) int id; u32 classid = 0; - if (in_interrupt()) + if (!clscg_enabled || in_interrupt()) return 0; rcu_read_lock(); diff --git a/net/core/sock.c b/net/core/sock.c index 8f67ced..8106e77 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -327,6 +327,11 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb) EXPORT_SYMBOL(__sk_backlog_rcv); #if defined(CONFIG_CGROUPS) +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) +struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE; +EXPORT_SYMBOL_GPL(cgroup_cls_enabled); +#endif + #if !defined(CONFIG_NET_CLS_CGROUP) int net_cls_subsys_id = -1; EXPORT_SYMBOL_GPL(net_cls_subsys_id); diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c index 7743ea8..554dc5b 100644 --- a/net/sched/cls_cgroup.c +++ b/net/sched/cls_cgroup.c @@ -293,6 +293,12 @@ static int __init init_cgroup_cls(void) if (ret) cgroup_unload_subsys(&net_cls_subsys); +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) + if (ret) + goto out; + static_key_slow_inc(&cgroup_cls_enabled); +#endif + out: return ret; } @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void) synchronize_rcu(); #endif +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) + static_key_slow_dec(&cgroup_cls_enabled); + rcu_barrier(); +#endif + cgroup_unload_subsys(&net_cls_subsys); }