From: Quanyang Wang <quanyang.wang@windriver.com> To: "Michal Koutný" <mkoutny@suse.com>, "Roman Gushchin" <guro@fb.com> Cc: Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH] cgroup: fix memory leak caused by missing cgroup_bpf_offline Date: Tue, 12 Oct 2021 14:22:13 +0800 [thread overview] Message-ID: <6d76de0b-9de7-adbe-834b-c49ed991559d@windriver.com> (raw) In-Reply-To: <20211011162128.GC61605@blackbody.suse.cz> Hi Michal & Roman, Thank you for your review. On 10/12/21 12:21 AM, Michal Koutný wrote: > Hello. > > On Thu, Oct 07, 2021 at 08:16:03PM +0800, quanyang.wang@windriver.com wrote: >> This is because that root_cgrp->bpf.refcnt.data is allocated by the >> function percpu_ref_init in cgroup_bpf_inherit which is called by >> cgroup_setup_root when mounting, but not freed along with root_cgrp >> when umounting. > > Good catch! > >> Adding cgroup_bpf_offline which calls percpu_ref_kill to >> cgroup_kill_sb can free root_cgrp->bpf.refcnt.data in umount path. > > That is sensible. > >> Fixes: 2b0d3d3e4fcfb ("percpu_ref: reduce memory footprint of percpu_ref in fast path") > > Why this Fixes:? Is the leak absent before the percpu_ref refactoring? Before this commit, percpu_ref is embedded in cgroup, it can be freed along with cgroup, so there is no memory leak. Since this commit, it causes the memory leak. Should I change it to "Fixes: 4bfc0bb2c60e ("bpf: decouple the lifetime of cgroup_bpf from cgroup itself")"? > I guess the embedded data are free'd together with cgroup. Makes me > wonder why struct cgroup_bpf has a separate percpu_ref counter from > struct cgroup... > >> +++ b/kernel/cgroup/cgroup.c >> @@ -2147,8 +2147,10 @@ static void cgroup_kill_sb(struct super_block *sb) >> * And don't kill the default root. >> */ >> if (list_empty(&root->cgrp.self.children) && root != &cgrp_dfl_root && >> - !percpu_ref_is_dying(&root->cgrp.self.refcnt)) >> + !percpu_ref_is_dying(&root->cgrp.self.refcnt)) { >> + cgroup_bpf_offline(&root->cgrp); > > (You made some unnecessary whitespace here breaking indention :-) Thanks for pointing it out. I will send a V2 to fix this. Thanks, Quanyang > >> percpu_ref_kill(&root->cgrp.self.refcnt); >> + } >> cgroup_put(&root->cgrp); >> kernfs_kill_sb(sb); >> }
WARNING: multiple messages have this Message-ID (diff)
From: Quanyang Wang <quanyang.wang@windriver.com> To: "Michal Koutný" <mkoutny@suse.com>, "Roman Gushchin" <guro@fb.com> Cc: Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH] cgroup: fix memory leak caused by missing cgroup_bpf_offline Date: Tue, 12 Oct 2021 14:22:13 +0800 [thread overview] Message-ID: <6d76de0b-9de7-adbe-834b-c49ed991559d@windriver.com> (raw) In-Reply-To: <20211011162128.GC61605@blackbody.suse.cz> Hi Michal & Roman, Thank you for your review. On 10/12/21 12:21 AM, Michal Koutný wrote: > Hello. > > On Thu, Oct 07, 2021 at 08:16:03PM +0800, quanyang.wang@windriver.com wrote: >> This is because that root_cgrp->bpf.refcnt.data is allocated by the >> function percpu_ref_init in cgroup_bpf_inherit which is called by >> cgroup_setup_root when mounting, but not freed along with root_cgrp >> when umounting. > > Good catch! > >> Adding cgroup_bpf_offline which calls percpu_ref_kill to >> cgroup_kill_sb can free root_cgrp->bpf.refcnt.data in umount path. > > That is sensible. > >> Fixes: 2b0d3d3e4fcfb ("percpu_ref: reduce memory footprint of percpu_ref in fast path") > > Why this Fixes:? Is the leak absent before the percpu_ref refactoring? Before this commit, percpu_ref is embedded in cgroup, it can be freed along with cgroup, so there is no memory leak. Since this commit, it causes the memory leak. Should I change it to "Fixes: 4bfc0bb2c60e ("bpf: decouple the lifetime of cgroup_bpf from cgroup itself")"? > I guess the embedded data are free'd together with cgroup. Makes me > wonder why struct cgroup_bpf has a separate percpu_ref counter from > struct cgroup... > >> +++ b/kernel/cgroup/cgroup.c >> @@ -2147,8 +2147,10 @@ static void cgroup_kill_sb(struct super_block *sb) >> * And don't kill the default root. >> */ >> if (list_empty(&root->cgrp.self.children) && root != &cgrp_dfl_root && >> - !percpu_ref_is_dying(&root->cgrp.self.refcnt)) >> + !percpu_ref_is_dying(&root->cgrp.self.refcnt)) { >> + cgroup_bpf_offline(&root->cgrp); > > (You made some unnecessary whitespace here breaking indention :-) Thanks for pointing it out. I will send a V2 to fix this. Thanks, Quanyang > >> percpu_ref_kill(&root->cgrp.self.refcnt); >> + } >> cgroup_put(&root->cgrp); >> kernfs_kill_sb(sb); >> }
next prev parent reply other threads:[~2021-10-12 6:22 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-07 12:16 [PATCH] cgroup: fix memory leak caused by missing cgroup_bpf_offline quanyang.wang 2021-10-11 16:21 ` Michal Koutný 2021-10-11 16:29 ` Roman Gushchin 2021-10-11 16:29 ` Roman Gushchin 2021-10-22 15:50 ` John Fastabend 2021-10-12 6:22 ` Quanyang Wang [this message] 2021-10-12 6:22 ` Quanyang Wang 2021-10-12 9:32 ` Michal Koutný
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6d76de0b-9de7-adbe-834b-c49ed991559d@windriver.com \ --to=quanyang.wang@windriver.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=axboe@kernel.dk \ --cc=bpf@vger.kernel.org \ --cc=cgroups@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=guro@fb.com \ --cc=hannes@cmpxchg.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan.x@bytedance.com \ --cc=ming.lei@redhat.com \ --cc=mkoutny@suse.com \ --cc=netdev@vger.kernel.org \ --cc=tj@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.