Re: [PATCH] mm, memcg: do full scan initially in force_empty

[Yafang Shao <laoar.shao@gmail.com>]

On Mon, Aug 3, 2020 at 6:12 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 31-07-20 09:50:04, Yafang Shao wrote:
> > On Thu, Jul 30, 2020 at 7:26 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Tue 28-07-20 03:40:32, Yafang Shao wrote:
> > > > Sometimes we use memory.force_empty to drop pages in a memcg to work
> > > > around some memory pressure issues. When we use force_empty, we want the
> > > > pages can be reclaimed ASAP, however force_empty reclaims pages as a
> > > > regular reclaimer which scans the page cache LRUs from DEF_PRIORITY
> > > > priority and finally it will drop to 0 to do full scan. That is a waste
> > > > of time, we'd better do full scan initially in force_empty.
> > >
> > > Do you have any numbers please?
> > >
> >
> > Unfortunately the number doesn't improve obviously, while it is
> > directly proportional to the numbers of total pages to be scanned.
>
> Your changelog claims an optimization and that should be backed by some
> numbers. It is true that reclaim at a higher priority behaves slightly
> and subtly differently but that urge for even more details in the
> changelog.
>

With the below addition change (nr_to_scan also changed), the elapsed
time of force_empty can be reduced by 10%.

@@ -3208,6 +3211,7 @@ static inline bool memcg_has_children(struct
mem_cgroup *memcg)
 static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 {
        int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
+       unsigned long size;

        /* we call try-to-free pages for make this cgroup empty */
        lru_add_drain_all();
@@ -3215,14 +3219,15 @@ static int mem_cgroup_force_empty(struct
mem_cgroup *memcg)
        drain_all_stock(memcg);
        /* try to free all pages in this cgroup */
-       while (nr_retries && page_counter_read(&memcg->memory)) {
+       while (nr_retries && (size = page_counter_read(&memcg->memory))) {
                int progress;

                if (signal_pending(current))
                        return -EINTR;
-               progress = try_to_free_mem_cgroup_pages(memcg, 1,
-                                                       GFP_KERNEL, true);
+               progress = try_to_free_mem_cgroup_pages(memcg, size,
+                                                       GFP_KERNEL, true,
+                                                       0);

Below are the numbers for a 16G memcg with full clean pagecache.
Without these change,
$ time echo 1 > /sys/fs/cgroup/memory/foo/memory.force_empty
real    0m2.247s
user    0m0.000s
sys     0m1.722s

With these change,
$ time echo 1 > /sys/fs/cgroup/memory/foo/memory.force_empty
real    0m2.053s
user    0m0.000s
sys     0m1.529s

But I'm not sure whether we should make this improvement, because
force_empty is not a critical path.


> > But then I notice that force_empty will try to write dirty pages, that
> > is not expected by us, because this behavior may be dangerous in the
> > production environment.
>
> I do not understand your claim here. Direct reclaim doesn't write dirty
> page cache pages directly.

It will write dirty pages once the sc->priority drops to a very low number.
if (sc->priority < DEF_PRIORITY - 2)
    sc->may_writepage = 1;

>  And it is even less clear why that would be
> dangerous if it did.
>

It will generate many IOs, which may block the others.

> > What do you think introducing per memcg drop_cache ?
>
> I do not like the global drop_cache and per memcg is not very much
> different. This all shouldn't be really necessary because we do have
> means to reclaim memory in a memcg.
> --

We used to find an issue that there are many negative  dentries in some memcgs.
These negative dentries were introduced by some specific workload in
these memcgs,  and we want to drop them as soon as possible.
But unfortunately there is no good way to drop them except the
force_empy or global drop_caches.
The force_empty will also drop the pagecache pages, which is not
expected by us.
The global drop_caches can't work either because it will drop slabs in
other memcgs.
That is why I want to introduce per memcg drop_caches.


-- 
Thanks
Yafang