| Message ID | 20210710003626.3549282-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] mm, memcg: add mem_cgroup_disabled checks in vmpressure and swap-related functions | expand |
On 2021/7/10 8:36, Suren Baghdasaryan wrote: > Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and > cgroup_throttle_swaprate functions. This minimizes the memcg overhead in > the pagefault and exit_mmap paths when memcgs are disabled using > cgroup_disable=memory command-line option. > This change results in ~2.1% overhead reduction when running PFT test > comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, > CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core > ARM64 Android device. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reviewed-by: Shakeel Butt <shakeelb@google.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/memcontrol.c | 3 +++ > mm/swapfile.c | 3 +++ > mm/vmpressure.c | 7 ++++++- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index ae1f5d0cb581..a228cd51c4bd 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) > struct mem_cgroup *memcg; > unsigned short id; > > + if (mem_cgroup_disabled()) > + return; > + > id = swap_cgroup_record(entry, 0, nr_pages); > rcu_read_lock(); > memcg = mem_cgroup_from_id(id); > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 1e07d1c776f2..707fa0481bb4 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) > struct swap_info_struct *si, *next; > int nid = page_to_nid(page); > > + if (mem_cgroup_disabled()) > + return; > + Many thanks for your patch. But I'am somewhat confused about this change. IMO, cgroup_throttle_swaprate() is only related to blk_cgroup and it seems it's irrelevant to mem_cgroup. Could you please have a explanation for me? Thanks! > if (!(gfp_mask & __GFP_IO)) > return; > > diff --git a/mm/vmpressure.c b/mm/vmpressure.c > index d69019fc3789..9b172561fded 100644 > --- a/mm/vmpressure.c > +++ b/mm/vmpressure.c > @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) > void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, > unsigned long scanned, unsigned long reclaimed) > { > - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); > + struct vmpressure *vmpr; > + > + if (mem_cgroup_disabled()) > + return; > + > + vmpr = memcg_to_vmpressure(memcg); > > /* > * Here we only want to account pressure that userland is able to >
On Fri, Jul 9, 2021 at 6:52 PM Miaohe Lin <linmiaohe@huawei.com> wrote: > > On 2021/7/10 8:36, Suren Baghdasaryan wrote: > > Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and > > cgroup_throttle_swaprate functions. This minimizes the memcg overhead in > > the pagefault and exit_mmap paths when memcgs are disabled using > > cgroup_disable=memory command-line option. > > This change results in ~2.1% overhead reduction when running PFT test > > comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, > > CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core > > ARM64 Android device. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Reviewed-by: Shakeel Butt <shakeelb@google.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > --- > > mm/memcontrol.c | 3 +++ > > mm/swapfile.c | 3 +++ > > mm/vmpressure.c | 7 ++++++- > > 3 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index ae1f5d0cb581..a228cd51c4bd 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) > > struct mem_cgroup *memcg; > > unsigned short id; > > > > + if (mem_cgroup_disabled()) > > + return; > > + > > id = swap_cgroup_record(entry, 0, nr_pages); > > rcu_read_lock(); > > memcg = mem_cgroup_from_id(id); > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 1e07d1c776f2..707fa0481bb4 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) > > struct swap_info_struct *si, *next; > > int nid = page_to_nid(page); > > > > + if (mem_cgroup_disabled()) > > + return; > > + > > Many thanks for your patch. But I'am somewhat confused about this change. > IMO, cgroup_throttle_swaprate() is only related to blk_cgroup and it seems > it's irrelevant to mem_cgroup. Could you please have a explanation for me? cgroup_throttle_swaprate() is a NoOp when CONFIG_MEMCG=n (see: https://elixir.bootlin.com/linux/latest/source/include/linux/swap.h#L699), therefore I assume we can safely skip it when memcgs are disabled via "cgroup_disable=memory". From perf results I also see no hits on this function when CONFIG_MEMCG=n. However, looking into the code, I'm not sure why it should depend on CONFIG_MEMCG. But it's Friday night and I might be missing some details here... > > Thanks! > > > if (!(gfp_mask & __GFP_IO)) > > return; > > > > diff --git a/mm/vmpressure.c b/mm/vmpressure.c > > index d69019fc3789..9b172561fded 100644 > > --- a/mm/vmpressure.c > > +++ b/mm/vmpressure.c > > @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) > > void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, > > unsigned long scanned, unsigned long reclaimed) > > { > > - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); > > + struct vmpressure *vmpr; > > + > > + if (mem_cgroup_disabled()) > > + return; > > + > > + vmpr = memcg_to_vmpressure(memcg); > > > > /* > > * Here we only want to account pressure that userland is able to > > >
On 2021/7/10 10:40, Suren Baghdasaryan wrote: > On Fri, Jul 9, 2021 at 6:52 PM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> On 2021/7/10 8:36, Suren Baghdasaryan wrote: >>> Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and >>> cgroup_throttle_swaprate functions. This minimizes the memcg overhead in >>> the pagefault and exit_mmap paths when memcgs are disabled using >>> cgroup_disable=memory command-line option. >>> This change results in ~2.1% overhead reduction when running PFT test >>> comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, >>> CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core >>> ARM64 Android device. >>> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>> Reviewed-by: Shakeel Butt <shakeelb@google.com> >>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> >>> --- >>> mm/memcontrol.c | 3 +++ >>> mm/swapfile.c | 3 +++ >>> mm/vmpressure.c | 7 ++++++- >>> 3 files changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>> index ae1f5d0cb581..a228cd51c4bd 100644 >>> --- a/mm/memcontrol.c >>> +++ b/mm/memcontrol.c >>> @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) >>> struct mem_cgroup *memcg; >>> unsigned short id; >>> >>> + if (mem_cgroup_disabled()) >>> + return; >>> + >>> id = swap_cgroup_record(entry, 0, nr_pages); >>> rcu_read_lock(); >>> memcg = mem_cgroup_from_id(id); >>> diff --git a/mm/swapfile.c b/mm/swapfile.c >>> index 1e07d1c776f2..707fa0481bb4 100644 >>> --- a/mm/swapfile.c >>> +++ b/mm/swapfile.c >>> @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) >>> struct swap_info_struct *si, *next; >>> int nid = page_to_nid(page); >>> >>> + if (mem_cgroup_disabled()) >>> + return; >>> + >> >> Many thanks for your patch. But I'am somewhat confused about this change. >> IMO, cgroup_throttle_swaprate() is only related to blk_cgroup and it seems >> it's irrelevant to mem_cgroup. Could you please have a explanation for me? > > cgroup_throttle_swaprate() is a NoOp when CONFIG_MEMCG=n (see: > https://elixir.bootlin.com/linux/latest/source/include/linux/swap.h#L699), I browsed the git history related to cgroup_throttle_swaprate() and found this: """ mm: memcontrol: move out cgroup swaprate throttling The cgroup swaprate throttling is about matching new anon allocations to the rate of available IO when that is being throttled. It's the io controller hooking into the VM, rather than a memory controller thing. """ It seems cgroup_throttle_swaprate() is working with memory allocations. So mem_cgroup matters this way. But I'am not sure... > therefore I assume we can safely skip it when memcgs are disabled via > "cgroup_disable=memory". From perf results I also see no hits on this > function when CONFIG_MEMCG=n. > However, looking into the code, I'm not sure why it should depend on > CONFIG_MEMCG. But it's Friday night and I might be missing some > details here... Many thanks for your replay at Friday night. :) > >> >> Thanks! >> >>> if (!(gfp_mask & __GFP_IO)) >>> return; >>> >>> diff --git a/mm/vmpressure.c b/mm/vmpressure.c >>> index d69019fc3789..9b172561fded 100644 >>> --- a/mm/vmpressure.c >>> +++ b/mm/vmpressure.c >>> @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) >>> void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, >>> unsigned long scanned, unsigned long reclaimed) >>> { >>> - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); >>> + struct vmpressure *vmpr; >>> + >>> + if (mem_cgroup_disabled()) >>> + return; >>> + >>> + vmpr = memcg_to_vmpressure(memcg); >>> >>> /* >>> * Here we only want to account pressure that userland is able to >>> >> > . >
On Sat, Jul 10, 2021 at 8:36 AM Suren Baghdasaryan <surenb@google.com> wrote: > > Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and > cgroup_throttle_swaprate functions. This minimizes the memcg overhead in > the pagefault and exit_mmap paths when memcgs are disabled using > cgroup_disable=memory command-line option. > This change results in ~2.1% overhead reduction when running PFT test > comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, > CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core > ARM64 Android device. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reviewed-by: Shakeel Butt <shakeelb@google.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> The changes are straightforward. LGTM. Reviewed-by: Muchun Song <songmuchun@bytedance.com>
On Fri 09-07-21 17:36:24, Suren Baghdasaryan wrote: > Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and > cgroup_throttle_swaprate functions. This minimizes the memcg overhead in > the pagefault and exit_mmap paths when memcgs are disabled using > cgroup_disable=memory command-line option. > This change results in ~2.1% overhead reduction when running PFT test What is PFT test? > comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, > CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core > ARM64 Android device. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reviewed-by: Shakeel Butt <shakeelb@google.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/memcontrol.c | 3 +++ > mm/swapfile.c | 3 +++ > mm/vmpressure.c | 7 ++++++- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index ae1f5d0cb581..a228cd51c4bd 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) > struct mem_cgroup *memcg; > unsigned short id; > > + if (mem_cgroup_disabled()) > + return; > + > id = swap_cgroup_record(entry, 0, nr_pages); > rcu_read_lock(); > memcg = mem_cgroup_from_id(id); > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 1e07d1c776f2..707fa0481bb4 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) > struct swap_info_struct *si, *next; > int nid = page_to_nid(page); > > + if (mem_cgroup_disabled()) > + return; > + > if (!(gfp_mask & __GFP_IO)) > return; > > diff --git a/mm/vmpressure.c b/mm/vmpressure.c > index d69019fc3789..9b172561fded 100644 > --- a/mm/vmpressure.c > +++ b/mm/vmpressure.c > @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) > void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, > unsigned long scanned, unsigned long reclaimed) > { > - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); > + struct vmpressure *vmpr; > + > + if (mem_cgroup_disabled()) > + return; > + > + vmpr = memcg_to_vmpressure(memcg); > > /* > * Here we only want to account pressure that userland is able to > -- > 2.32.0.93.g670b81a890-goog
On Mon, Jul 12, 2021 at 12:11 AM Michal Hocko <mhocko@suse.com> wrote: > > On Fri 09-07-21 17:36:24, Suren Baghdasaryan wrote: > > Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and > > cgroup_throttle_swaprate functions. This minimizes the memcg overhead in > > the pagefault and exit_mmap paths when memcgs are disabled using > > cgroup_disable=memory command-line option. > > This change results in ~2.1% overhead reduction when running PFT test > > What is PFT test? Christoph Lamenter’s pagefault tool (https://lkml.org/lkml/2006/8/29/294). I'll add the link in the description for clarity. > > > comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, > > CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core > > ARM64 Android device. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Reviewed-by: Shakeel Butt <shakeelb@google.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > > Thanks! > > > --- > > mm/memcontrol.c | 3 +++ > > mm/swapfile.c | 3 +++ > > mm/vmpressure.c | 7 ++++++- > > 3 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index ae1f5d0cb581..a228cd51c4bd 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) > > struct mem_cgroup *memcg; > > unsigned short id; > > > > + if (mem_cgroup_disabled()) > > + return; > > + > > id = swap_cgroup_record(entry, 0, nr_pages); > > rcu_read_lock(); > > memcg = mem_cgroup_from_id(id); > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 1e07d1c776f2..707fa0481bb4 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) > > struct swap_info_struct *si, *next; > > int nid = page_to_nid(page); > > > > + if (mem_cgroup_disabled()) > > + return; > > + > > if (!(gfp_mask & __GFP_IO)) > > return; > > > > diff --git a/mm/vmpressure.c b/mm/vmpressure.c > > index d69019fc3789..9b172561fded 100644 > > --- a/mm/vmpressure.c > > +++ b/mm/vmpressure.c > > @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) > > void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, > > unsigned long scanned, unsigned long reclaimed) > > { > > - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); > > + struct vmpressure *vmpr; > > + > > + if (mem_cgroup_disabled()) > > + return; > > + > > + vmpr = memcg_to_vmpressure(memcg); > > > > /* > > * Here we only want to account pressure that userland is able to > > -- > > 2.32.0.93.g670b81a890-goog > > -- > Michal Hocko > SUSE Labs
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index ae1f5d0cb581..a228cd51c4bd 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -7305,6 +7305,9 @@ void mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages) struct mem_cgroup *memcg; unsigned short id; + if (mem_cgroup_disabled()) + return; + id = swap_cgroup_record(entry, 0, nr_pages); rcu_read_lock(); memcg = mem_cgroup_from_id(id); diff --git a/mm/swapfile.c b/mm/swapfile.c index 1e07d1c776f2..707fa0481bb4 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3778,6 +3778,9 @@ void cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) struct swap_info_struct *si, *next; int nid = page_to_nid(page); + if (mem_cgroup_disabled()) + return; + if (!(gfp_mask & __GFP_IO)) return; diff --git a/mm/vmpressure.c b/mm/vmpressure.c index d69019fc3789..9b172561fded 100644 --- a/mm/vmpressure.c +++ b/mm/vmpressure.c @@ -240,7 +240,12 @@ static void vmpressure_work_fn(struct work_struct *work) void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, unsigned long scanned, unsigned long reclaimed) { - struct vmpressure *vmpr = memcg_to_vmpressure(memcg); + struct vmpressure *vmpr; + + if (mem_cgroup_disabled()) + return; + + vmpr = memcg_to_vmpressure(memcg); /* * Here we only want to account pressure that userland is able to
Add mem_cgroup_disabled check in vmpressure, mem_cgroup_uncharge_swap and cgroup_throttle_swaprate functions. This minimizes the memcg overhead in the pagefault and exit_mmap paths when memcgs are disabled using cgroup_disable=memory command-line option. This change results in ~2.1% overhead reduction when running PFT test comparing {CONFIG_MEMCG=n, CONFIG_MEMCG_SWAP=n} against {CONFIG_MEMCG=y, CONFIG_MEMCG_SWAP=y, cgroup_disable=memory} configuration on an 8-core ARM64 Android device. Signed-off-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/memcontrol.c | 3 +++ mm/swapfile.c | 3 +++ mm/vmpressure.c | 7 ++++++- 3 files changed, 12 insertions(+), 1 deletion(-)