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

[Yang Shi]

On 1/4/19 4:43 PM, Shakeel Butt wrote:
> 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.

OK, will keep force_empty intact.

Thanks,
Yang

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