* [PATCH] mm/memcg: Skip high limit check in root memcg @ 2023-02-10 9:45 Haifeng Xu 2023-02-14 15:56 ` Michal Hocko 0 siblings, 1 reply; 6+ messages in thread From: Haifeng Xu @ 2023-02-10 9:45 UTC (permalink / raw) To: hannes Cc: mhocko, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel, Haifeng Xu The high limit checks the memory usage from given memcg to root memcg. However, there is no limit in root memcg. So this check makes no sense and we can ignore it. Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> --- mm/memcontrol.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 73afff8062f9..a31a56598f29 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2780,6 +2780,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, do { bool mem_high, swap_high; + /* There is no need for root memcg to check high limit */ + if (mem_cgroup_is_root(memcg)) + break; + mem_high = page_counter_read(&memcg->memory) > READ_ONCE(memcg->memory.high); swap_high = page_counter_read(&memcg->swap) > -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/memcg: Skip high limit check in root memcg 2023-02-10 9:45 [PATCH] mm/memcg: Skip high limit check in root memcg Haifeng Xu @ 2023-02-14 15:56 ` Michal Hocko 2023-02-21 10:29 ` Haifeng Xu 0 siblings, 1 reply; 6+ messages in thread From: Michal Hocko @ 2023-02-14 15:56 UTC (permalink / raw) To: Haifeng Xu Cc: hannes, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel On Fri 10-02-23 09:45:50, Haifeng Xu wrote: > The high limit checks the memory usage from given memcg to root memcg. > However, there is no limit in root memcg. So this check makes no sense > and we can ignore it. Is this check actually addining any benefit? Have you measured aby performance gains by this change? > Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> > --- > mm/memcontrol.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 73afff8062f9..a31a56598f29 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2780,6 +2780,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, > do { > bool mem_high, swap_high; > > + /* There is no need for root memcg to check high limit */ > + if (mem_cgroup_is_root(memcg)) > + break; > + > mem_high = page_counter_read(&memcg->memory) > > READ_ONCE(memcg->memory.high); > swap_high = page_counter_read(&memcg->swap) > > -- > 2.25.1 -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/memcg: Skip high limit check in root memcg 2023-02-14 15:56 ` Michal Hocko @ 2023-02-21 10:29 ` Haifeng Xu 2023-02-21 12:20 ` Michal Hocko 0 siblings, 1 reply; 6+ messages in thread From: Haifeng Xu @ 2023-02-21 10:29 UTC (permalink / raw) To: Michal Hocko Cc: hannes, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1277 bytes --] On 2023/2/14 23:56, Michal Hocko wrote: > On Fri 10-02-23 09:45:50, Haifeng Xu wrote: >> The high limit checks the memory usage from given memcg to root memcg. >> However, there is no limit in root memcg. So this check makes no sense >> and we can ignore it. > > Is this check actually addining any benefit? Have you measured aby > performance gains by this change? > >> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> >> --- >> mm/memcontrol.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 73afff8062f9..a31a56598f29 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -2780,6 +2780,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, >> do { >> bool mem_high, swap_high; >> >> + /* There is no need for root memcg to check high limit */ >> + if (mem_cgroup_is_root(memcg)) >> + break; >> + >> mem_high = page_counter_read(&memcg->memory) > >> READ_ONCE(memcg->memory.high); >> swap_high = page_counter_read(&memcg->swap) > >> -- >> 2.25.1 > test steps: 1. mkdir -p /sys/fs/cgroup/memory/test 2. echo $$ > /sys/fs/cgroup/memory/test/cgroup.procs 3. ./mmap_test The test result show that with or without the patch, the time taken is almost the same. [-- Attachment #2: mmap_test.c --] [-- Type: text/plain, Size: 927 bytes --] #include <sys/mman.h> #include <sys/types.h> #include <unistd.h> #include <stdlib.h> #include <stdio.h> #include <fcntl.h> #include <ctype.h> #include <string.h> #include <inttypes.h> #define SIZE (5 * 1024 * 1024 * 1024) int64_t current_time_ms() { struct timeval time; gettimeofday(&time, NULL); int64_t s1 = (int64_t)(time.tv_sec) * 1000; int64_t s2 = (time.tv_usec / 1000); return s1 + s2; } int main(int argc, char* argv[]) { void * buf; size_t size = SIZE; int64_t start, cost; buf = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, 0, 0); if (buf < 0 ) { printf("mmap failed\n"); exit(-1); } start = current_time_ms(); mlock(buf, size); cost = current_time_ms() - start; printf("cost: %" PRId64 " ms\n", cost); munmap(buf, size); return 0; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/memcg: Skip high limit check in root memcg 2023-02-21 10:29 ` Haifeng Xu @ 2023-02-21 12:20 ` Michal Hocko 2023-02-21 14:21 ` Haifeng Xu 0 siblings, 1 reply; 6+ messages in thread From: Michal Hocko @ 2023-02-21 12:20 UTC (permalink / raw) To: Haifeng Xu Cc: hannes, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel On Tue 21-02-23 18:29:39, Haifeng Xu wrote: > > > On 2023/2/14 23:56, Michal Hocko wrote: > > On Fri 10-02-23 09:45:50, Haifeng Xu wrote: > >> The high limit checks the memory usage from given memcg to root memcg. > >> However, there is no limit in root memcg. So this check makes no sense > >> and we can ignore it. > > > > Is this check actually addining any benefit? Have you measured aby > > performance gains by this change? > > > >> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> > >> --- > >> mm/memcontrol.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >> index 73afff8062f9..a31a56598f29 100644 > >> --- a/mm/memcontrol.c > >> +++ b/mm/memcontrol.c > >> @@ -2780,6 +2780,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, > >> do { > >> bool mem_high, swap_high; > >> > >> + /* There is no need for root memcg to check high limit */ > >> + if (mem_cgroup_is_root(memcg)) > >> + break; > >> + > >> mem_high = page_counter_read(&memcg->memory) > > >> READ_ONCE(memcg->memory.high); > >> swap_high = page_counter_read(&memcg->swap) > > >> -- > >> 2.25.1 > > > > test steps: > 1. mkdir -p /sys/fs/cgroup/memory/test > 2. echo $$ > /sys/fs/cgroup/memory/test/cgroup.procs > 3. ./mmap_test > > The test result show that with or without the patch, the time taken is almost the same. This is in line with my expectation. So the question is whether the additional check is really worth it. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/memcg: Skip high limit check in root memcg 2023-02-21 12:20 ` Michal Hocko @ 2023-02-21 14:21 ` Haifeng Xu 2023-02-21 15:21 ` Michal Hocko 0 siblings, 1 reply; 6+ messages in thread From: Haifeng Xu @ 2023-02-21 14:21 UTC (permalink / raw) To: Michal Hocko Cc: hannes, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel On 2023/2/21 20:20, Michal Hocko wrote: > On Tue 21-02-23 18:29:39, Haifeng Xu wrote: >> >> >> On 2023/2/14 23:56, Michal Hocko wrote: >>> On Fri 10-02-23 09:45:50, Haifeng Xu wrote: >>>> The high limit checks the memory usage from given memcg to root memcg. >>>> However, there is no limit in root memcg. So this check makes no sense >>>> and we can ignore it. >>> >>> Is this check actually addining any benefit? Have you measured aby >>> performance gains by this change? >>> >>>> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> >>>> --- >>>> mm/memcontrol.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>>> index 73afff8062f9..a31a56598f29 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -2780,6 +2780,10 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, >>>> do { >>>> bool mem_high, swap_high; >>>> >>>> + /* There is no need for root memcg to check high limit */ >>>> + if (mem_cgroup_is_root(memcg)) >>>> + break; >>>> + >>>> mem_high = page_counter_read(&memcg->memory) > >>>> READ_ONCE(memcg->memory.high); >>>> swap_high = page_counter_read(&memcg->swap) > >>>> -- >>>> 2.25.1 >>> >> >> test steps: >> 1. mkdir -p /sys/fs/cgroup/memory/test >> 2. echo $$ > /sys/fs/cgroup/memory/test/cgroup.procs >> 3. ./mmap_test >> >> The test result show that with or without the patch, the time taken is almost the same. > > This is in line with my expectation. So the question is whether the > additional check is really worth it. This patch doesn't bring any obvious benifit or harm, but the high limit check in root memcg seems a little weird. Maybe we can add this check？It all depends on your viewpoint. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/memcg: Skip high limit check in root memcg 2023-02-21 14:21 ` Haifeng Xu @ 2023-02-21 15:21 ` Michal Hocko 0 siblings, 0 replies; 6+ messages in thread From: Michal Hocko @ 2023-02-21 15:21 UTC (permalink / raw) To: Haifeng Xu Cc: hannes, shakeelb, muchun.song, akpm, cgroups, linux-mm, linux-kernel On Tue 21-02-23 22:21:45, Haifeng Xu wrote: [...] > >> The test result show that with or without the patch, the time taken is almost the same. > > > > This is in line with my expectation. So the question is whether the > > additional check is really worth it. > > This patch doesn't bring any obvious benifit or harm, but the high > limit check in root memcg seems a little weird. Maybe we can add this > check Well, I do not see the code to look weird TBH. There is nothing wrong in doing the check for the root memcg. It is a bit pointless but it is not incorrect. > It all depends on your viewpoint. From my POV, I prefer changes that either fix something (correctness issue or a performance issue/improvement) or improve readbility. The check doesn't fix anything and I am not convinced about an improved readabilit either. Thanks for the patch anyway! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-21 15:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-10 9:45 [PATCH] mm/memcg: Skip high limit check in root memcg Haifeng Xu 2023-02-14 15:56 ` Michal Hocko 2023-02-21 10:29 ` Haifeng Xu 2023-02-21 12:20 ` Michal Hocko 2023-02-21 14:21 ` Haifeng Xu 2023-02-21 15:21 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox