Hi Tejun, On 08/24/2016 11:54 PM, Tejun Heo wrote: > On Wed, Aug 24, 2016 at 10:24:19PM +0200, Daniel Mack wrote: >> +void cgroup_bpf_free(struct cgroup *cgrp) >> +{ >> + unsigned int type; >> + >> + rcu_read_lock(); >> + >> + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { >> + if (!cgrp->bpf.prog[type]) >> + continue; >> + >> + bpf_prog_put(cgrp->bpf.prog[type]); >> + static_branch_dec(&cgroup_bpf_enabled_key); >> + } >> + >> + rcu_read_unlock(); > > These rcu locking seem suspicious to me. RCU locking on writer side > is usually bogus. We sometimes do it to work around locking > assertions in accessors but it's a better idea to make the assertions > better in those cases - e.g. sth like assert_mylock_or_rcu_locked().
Right, in this case, it is unnecessary, as the bpf.prog[] is not under RCU. >> +void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent) >> +{ >> + unsigned int type; >> + >> + rcu_read_lock(); > > Ditto. > >> + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) >> + rcu_assign_pointer(cgrp->bpf.prog_effective[type], >> + rcu_dereference(parent->bpf.prog_effective[type])); Okay, yes. We're under cgroup_mutex write-path protection here, so that's unnecessary too. >> +void __cgroup_bpf_update(struct cgroup *cgrp, >> + struct cgroup *parent, >> + struct bpf_prog *prog, >> + enum bpf_attach_type type) >> +{ >> + struct bpf_prog *old_prog, *effective; >> + struct cgroup_subsys_state *pos; >> + >> + rcu_read_lock(); > > Ditto. Yes, agreed, as above. >> + old_prog = xchg(cgrp->bpf.prog + type, prog); >> + if (old_prog) { >> + bpf_prog_put(old_prog); >> + static_branch_dec(&cgroup_bpf_enabled_key); >> + } >> + >> + if (prog) >> + static_branch_inc(&cgroup_bpf_enabled_key); > > Minor but probably better to inc first and then dec so that you can > avoid unnecessary enabled -> disabled -> enabled sequence. Good point. Will fix. >> + rcu_read_unlock(); >> + >> + css_for_each_descendant_pre(pos, &cgrp->self) { > > On the other hand, this walk actually requires rcu read locking unless > you're holding cgroup_mutex. I am - this function is always called with cgroup_mutex held through the wrapper in kernel/cgroup.c. Thanks a lot - will put all that changes in v3. Daniel