On Tue, Apr 28, 2020 at 1:05 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Sat, Apr 25, 2020 at 11:24:15AM -0400, Yafang Shao wrote:
> > Since proportional memory.{min, low} reclaim is introduced in
> > commit 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim"),
> > it have been proved that the proportional reclaim is hard to understand and
> > the issues caused by it is harder to understand.[1]. That dilemma faced by
> > us is caused by that the proportional reclaim mixed up memcg and the
> > reclaim context.
> >
> > In proportional reclaim, the whole reclaim context - includes the memcg
> > to be reclaimed and the reclaimer, should be considered, rather than
> > memcg only.
> >
> > To make it clear, a new member 'protection' is introduced in the reclaim
> > context (struct shrink_control) to replace mem_cgroup_protection(). This
> > one is set when we check whether the memcg is protected or not.
>
> This patch series makes the code harder to understand.
>
I don't think so.
With this patchset, you don't need to care about what the emin of this
memcg and its parent should be when you calculate the scan count.
Furthermore, you don't need to worry about the potential race when you
are getting the scan count.
'Protection' is calculated when we check whether the memcg is
'protected' or not, that is really easy to understand.
> It's already tricky to understand ownership and lifetime/validity of
> struct scan_control members when it's just vmscan.c functions touching
> them.
Note that the reclaimers can call the funtions existing in vmscan.c,
compaction.c, slub.c, filemap.c, oom_kill.c, workingset.c,
page-writeback.c, swap.c, khugepaged.c and etc. IOW, it may be used in
these functions sooner or later.
> Expanding the scope to the entire MM is a bad idea.
>
Well, mem_cgroup_protection() is only used in memcontrol.c and vmscan.c.
So expanding the scope to the entire LINUX is a MORE bad idea.
There may be some other functions doing this as well.
> Splitting up sc->memcg_low_reclaim handling and the associated restart
> logic makes control flow harder to follow as well.
>
Placing sc->memcg_low_reclaim in lots of callsites make the control
flow more than harder to follow.
> The problem with the current code is a suboptimal interface between
> the memcg protection calculation and the reclaim control flow. We need
> to clean that up, not remove the interface and thereby all clarity on
> who handles which information.
'remove the interface ' in the CURRENT CALLSITE is really a good 'clean up'.
Seems we can't get an agreement on how to improve current code.
So I will submit a patch to revert the commit 9783aa9917f8 ("mm,
memcg: proportional memory.{low,min} reclaim") first.
Then let's talk about the good solution to implement it.
BTW, I can't image why such a bad and ugly implementation got merged
in the first place.
The quantity of memory cgroup is becoming more and more bad.
--
Thanks
Yafang
