Messages in this thread | | | Subject | Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max() | From | Waiman Long <> | Date | Sun, 13 Sep 2020 20:49:01 -0400 |
| |
On 8/20/20 5:25 PM, Shakeel Butt wrote: > On Thu, Aug 20, 2020 at 1:29 PM Waiman Long <longman@redhat.com> wrote: >> On 8/20/20 1:35 PM, Johannes Weiner wrote: >>> On Thu, Aug 20, 2020 at 09:03:49AM -0400, Waiman Long wrote: >>>> The mem_cgroup_get_max() function used to get memory+swap max from >>>> both the v1 memsw and v2 memory+swap page counters & return the maximum >>>> of these 2 values. This is redundant and it is more efficient to just >>>> get either the v1 or the v2 values depending on which one is currently >>>> in use. >>>> >>>> Signed-off-by: Waiman Long <longman@redhat.com> >>>> --- >>>> mm/memcontrol.c | 14 +++++--------- >>>> 1 file changed, 5 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>>> index 26b7a48d3afb..d219dca5239f 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg) >>>> */ >>>> unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg) >>>> { >>>> - unsigned long max; >>>> + unsigned long max = READ_ONCE(memcg->memory.max); >>>> >>>> - max = READ_ONCE(memcg->memory.max); >>>> if (mem_cgroup_swappiness(memcg)) { >>>> - unsigned long memsw_max; >>>> - unsigned long swap_max; >>>> - >>>> - memsw_max = memcg->memsw.max; >>>> - swap_max = READ_ONCE(memcg->swap.max); >>>> - swap_max = min(swap_max, (unsigned long)total_swap_pages); >>>> - max = min(max + swap_max, memsw_max); >>>> + if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) >>>> + max += READ_ONCE(memcg->swap.max); >>>> + else >>>> + max = memcg->memsw.max; >>> I agree with the premise of the patch, but v1 and v2 have sufficiently >>> different logic, and the way v1 overrides max from the innermost >>> branch again also doesn't help in understanding what's going on. >>> >>> Can you please split out the v1 and v2 code? >>> >>> if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) { >>> max = READ_ONCE(memcg->memory.max); >>> if (mem_cgroup_swappiness(memcg)) >>> max += READ_ONCE(memcg->swap.max); >>> } else { >>> if (mem_cgroup_swappiness(memcg)) >>> max = memcg->memsw.max; >>> else >>> max = READ_ONCE(memcg->memory.max); >>> } >>> >>> It's slightly repetitive, but IMO much more readable. >>> >> Sure. That makes it even better. >> > Can you please also add in the commit message why it is ok to drop > total_swap_pages comparison from mem_cgroup_get_max()? > My bad. I accidentally skipped the total_swap_pages check. Will add it back in v2.
Cheers, Longman
| |