From: Michal Hocko <mhocko@suse.com>
To: Yafang Shao <laoar.shao@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>,
Andrew Morton <akpm@linux-foundation.org>,
Linux MM <linux-mm@kvack.org>
Subject: Re: [PATCH] mm, memcg: do full scan initially in force_empty
Date: Mon, 3 Aug 2020 15:56:36 +0200 [thread overview]
Message-ID: <20200803135636.GN5174@dhcp22.suse.cz> (raw)
In-Reply-To: <CALOAHbAcB_jdYQqYzvZw24FkjJR+rkyvpisYAZRASkZY8tgNsQ@mail.gmail.com>
On Mon 03-08-20 21:20:44, Yafang Shao wrote:
> 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);
Have you tried this change without changing the reclaim priority?
> 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.
Well, an isolated change to force_empty would be more acceptable but it
is worth noting that a very large reclaim target might affect the
userspace triggering this path because it will potentially increase
latency to process any signals. I do not expect this to be a huge
problem in practice because even reclaim for a smaller target can take
quite long if the memory is not really reclaimable and it has to take
the full world scan. Moreovere most userspace will simply do
echo 1 > $MEMCG_PAGE/force_empty
and only care about killing that if it takes too long.
> > > 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;
OK, I see what you mean now. Please have a look above that check:
/*
* Only kswapd can writeback filesystem pages
* to avoid risk of stack overflow. But avoid
* injecting inefficient single-page IO into
* flusher writeback as much as possible: only
* write pages when we've encountered many
* dirty pages, and when we've already scanned
* the rest of the LRU for clean pages and see
* the same dirty pages again (PageReclaim).
*/
> > 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.
Yes, negative dentries can build up but the memory reclaim should be
pretty effective reclaiming them.
> 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.
You can use memcg limits (e.g. memory high) to pro-actively reclaim
excess memory. Have you tried that?
> The force_empty will also drop the pagecache pages, which is not
> expected by us.
force_empty is intended to reclaim _all_ pages.
> 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.
Problems with negative dentries has been already discussed in the past.
I believe there was no conclusion so far. Please try to dig into
archives.
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2020-08-03 13:56 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 7:40 [PATCH] mm, memcg: do full scan initially in force_empty Yafang Shao
2020-07-30 11:26 ` Michal Hocko
2020-07-31 1:50 ` Yafang Shao
2020-08-03 10:12 ` Michal Hocko
2020-08-03 13:20 ` Yafang Shao
2020-08-03 13:56 ` Michal Hocko [this message]
2020-08-03 14:18 ` Yafang Shao
2020-08-03 14:26 ` Yafang Shao
2020-08-03 14:37 ` Michal Hocko
2020-08-03 14:34 ` Michal Hocko
2020-08-03 15:26 ` Waiman Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200803135636.GN5174@dhcp22.suse.cz \
--to=mhocko@suse.com \
--cc=akpm@linux-foundation.org \
--cc=hannes@cmpxchg.org \
--cc=laoar.shao@gmail.com \
--cc=linux-mm@kvack.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).