On 5/22/19 4:20 PM, Roman Gushchin wrote: > Currently the lifetime of bpf programs attached to a cgroup is bound > to the lifetime of the cgroup itself. It means that if a user > forgets (or intentionally avoids) to detach a bpf program before > removing the cgroup, it will stay attached up to the release of the > cgroup. Since the cgroup can stay in the dying state (the state > between being rmdir()'ed and being released) for a very long time, it > leads to a waste of memory. Also, it blocks a possibility to implement > the memcg-based memory accounting for bpf objects, because a circular > reference dependency will occur. Charged memory pages are pinning the > corresponding memory cgroup, and if the memory cgroup is pinning > the attached bpf program, nothing will be ever released. > > A dying cgroup can not contain any processes, so the only chance for > an attached bpf program to be executed is a live socket associated > with the cgroup. So in order to release all bpf data early, let's > count associated sockets using a new percpu refcounter. On cgroup > removal the counter is transitioned to the atomic mode, and as soon > as it reaches 0, all bpf programs are detached. > > The reference counter is not socket specific, and can be used for any > other types of programs, which can be executed from a cgroup-bpf hook > outside of the process context, had such a need arise in the future. > > Signed-off-by: Roman Gushchin <[email protected]> > Cc: [email protected]
The logic looks sound to me. With one nit below, Acked-by: Yonghong Song <[email protected]> > --- > include/linux/bpf-cgroup.h | 8 ++++++-- > include/linux/cgroup.h | 18 ++++++++++++++++++ > kernel/bpf/cgroup.c | 25 ++++++++++++++++++++++--- > kernel/cgroup/cgroup.c | 11 ++++++++--- > 4 files changed, 54 insertions(+), 8 deletions(-) > [...] > @@ -167,7 +178,12 @@ int cgroup_bpf_inherit(struct cgroup *cgrp) > */ > #define NR ARRAY_SIZE(cgrp->bpf.effective) > struct bpf_prog_array __rcu *arrays[NR] = {}; > - int i; > + int ret, i; > + > + ret = percpu_ref_init(&cgrp->bpf.refcnt, cgroup_bpf_release, 0, > + GFP_KERNEL); > + if (ret) > + return -ENOMEM; Maybe return "ret" here instead of -ENOMEM. Currently, percpu_ref_init only return error code is -ENOMEM. But in the future, it could change? > > for (i = 0; i < NR; i++) > INIT_LIST_HEAD(&cgrp->bpf.progs[i]); > @@ -183,6 +199,9 @@ int cgroup_bpf_inherit(struct cgroup *cgrp) > cleanup: > for (i = 0; i < NR; i++) > bpf_prog_array_free(arrays[i]); > + > + percpu_ref_exit(&cgrp->bpf.refcnt); > + > return -ENOMEM; > } > [...]
