Re: [v2 PATCH 2/5] mm: memcontrol: do not try to do swap when force empty

[Shakeel Butt]

On Fri, Jan 4, 2019 at 4:21 PM Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx> wrote:
>
> The typical usecase of force empty is to try to reclaim as much as
> possible memory before offlining a memcg.  Since there should be no
> attached tasks to offlining memcg, the tasks anonymous pages would have
> already been freed or uncharged.  Even though anonymous pages get
> swapped out, but they still get charged to swap space.  So, it sounds
> pointless to do swap for force empty.
>
> I tried to dig into the history of this, it was introduced by
> commit 8c7c6e34a125 ("memcg: mem+swap controller core"), but there is
> not any clue about why it was done so at the first place.
>
> The below simple test script shows slight file cache reclaim improvement
> when swap is on.
>
> echo 3 > /proc/sys/vm/drop_caches
> mkdir /sys/fs/cgroup/memory/test
> echo 30 > /sys/fs/cgroup/memory/test/memory.swappiness
> echo $$ >/sys/fs/cgroup/memory/test/cgroup.procs
> cat /proc/meminfo | grep ^Cached|awk -F" " '{print $2}'
> dd if=/dev/zero of=/mnt/test bs=1M count=1024
> ping localhost > /dev/null &
> echo 1 > /sys/fs/cgroup/memory/test/memory.force_empty
> killall ping
> echo $$ >/sys/fs/cgroup/memory/cgroup.procs
> cat /proc/meminfo | grep ^Cached|awk -F" " '{print $2}'
> rmdir /sys/fs/cgroup/memory/test
> cat /proc/meminfo | grep ^Cached|awk -F" " '{print $2}'
>
> The number of page cache is:
>                         w/o             w/
> before force empty    1088792        1088784
> after force empty     41492          39428
> reclaimed             1047300        1049356
>
> Without doing swap, force empty can reclaim 2MB more memory in 1GB page
> cache.
>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index af7f18b..75208a2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2895,7 +2895,7 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
>                         return -EINTR;
>
>                 progress = try_to_free_mem_cgroup_pages(memcg, 1,
> -                                                       GFP_KERNEL, true);
> +                                                       GFP_KERNEL, false);

I think we agreed not to change the behavior of force_empty. You can
customize 'force_empty on wipe_on_offline' to not swapout.

>                 if (!progress) {
>                         nr_retries--;
>                         /* maybe some writeback is necessary */
> --
> 1.8.3.1
>