2020-08-20 13:06:15

by Waiman Long

Subject: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()

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 <[email protected]>
---
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;
}
return max;
}
--
2.18.1

2020-08-20 17:38:47

by Johannes Weiner

Subject: Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()

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 <[email protected]>
> ---
> 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.

2020-08-20 21:56:15

by Waiman Long

Subject: Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()

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 <[email protected]>
>> ---
>> 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.

Cheers,
Longman

2020-08-20 22:05:36

by Shakeel Butt

Subject: Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()

On Thu, Aug 20, 2020 at 1:29 PM Waiman Long <[email protected]> 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 <[email protected]>
> >> ---
> >> 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()?

2020-09-14 00:50:38

by Waiman Long

Subject: Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()

On 8/20/20 5:25 PM, Shakeel Butt wrote:
> On Thu, Aug 20, 2020 at 1:29 PM Waiman Long <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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