On Sat, Jan 04, 2020 at 03:26:13PM +0800, Yafang Shao wrote:
> On Sat, Jan 4, 2020 at 11:36 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, Dec 24, 2019 at 02:53:25AM -0500, Yafang Shao wrote:
> > > The lru walker isolation function may use this memcg to do something, e.g.
> > > the inode isolatation function will use the memcg to do inode protection in
> > > followup patch. So make memcg visible to the lru walker isolation function.
> > >
> > > Something should be emphasized in this patch is it replaces
> > > for_each_memcg_cache_index() with for_each_mem_cgroup() in
> > > list_lru_walk_node(). Because there's a gap between these two MACROs that
> > > for_each_mem_cgroup() depends on CONFIG_MEMCG while the other one depends
> > > on CONFIG_MEMCG_KMEM. But as list_lru_memcg_aware() returns false if
> > > CONFIG_MEMCG_KMEM is not configured, it is safe to this replacement.
> > >
> > > Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> > > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> >
> > ....
> >
> > > @@ -299,17 +299,15 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
> > > list_lru_walk_cb isolate, void *cb_arg,
> > > unsigned long *nr_to_walk)
> > > {
> > > + struct mem_cgroup *memcg;
> > > long isolated = 0;
> > > - int memcg_idx;
> > >
> > > - isolated += list_lru_walk_one(lru, nid, NULL, isolate, cb_arg,
> > > - nr_to_walk);
> > > - if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
> > > - for_each_memcg_cache_index(memcg_idx) {
> > > + if (list_lru_memcg_aware(lru)) {
> > > + for_each_mem_cgroup(memcg) {
> > > struct list_lru_node *nlru = &lru->node[nid];
> > >
> > > spin_lock(&nlru->lock);
> > > - isolated += __list_lru_walk_one(nlru, memcg_idx,
> > > + isolated += __list_lru_walk_one(nlru, memcg,
> > > isolate, cb_arg,
> > > nr_to_walk);
> > > spin_unlock(&nlru->lock);
> > > @@ -317,7 +315,11 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
> > > if (*nr_to_walk <= 0)
> > > break;
> > > }
> > > + } else {
> > > + isolated += list_lru_walk_one(lru, nid, NULL, isolate, cb_arg,
> > > + nr_to_walk);
> > > }
> > > +
> >
> > That's a change of behaviour. The old code always runs per-node
> > reclaim, then if the LRU is memcg aware it also runs the memcg
> > aware reclaim. The new code never runs global per-node reclaim
> > if the list is memcg aware, so shrinkers that are initialised
> > with the flags SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE seem
> > likely to have reclaim problems with mixed memcg/global memory
> > pressure scenarios.
> >
> > e.g. if all the memory is in the per-node lists, and the memcg needs
> > to reclaim memory because of a global shortage, it is now unable to
> > reclaim global memory.....
> >
>
> Hi Dave,
>
> Thanks for your detailed explanation.
> But I have different understanding.
> The difference between for_each_mem_cgroup(memcg) and
> for_each_memcg_cache_index(memcg_idx) is that the
> for_each_mem_cgroup() includes the root_mem_cgroup while the
> for_each_memcg_cache_index() excludes the root_mem_cgroup because the
> memcg_idx of it is -1.
Except that the "root" memcg that for_each_mem_cgroup() is not the
"global root" memcg - it is whatever memcg that is passed down in
the shrink_control, whereever that sits in the cgroup tree heirarchy.
do_shrink_slab() only ever passes down the global root memcg to the
shrinkers when the global root memcg is passed to shrink_slab(), and
that does not iterate the memcg heirarchy - it just wants to
reclaim from global caches an non-memcg aware shrinkers.
IOWs, there are multiple changes in behaviour here - memcg specific
reclaim won't do global reclaim, and global reclaim will now iterate
all memcgs instead of just the global root memcg.
> So it can reclaim global memory even if the list is memcg aware.
> Is that right ?
If the memcg passed to this fucntion is the root memcg, then yes,
it will behave as you suggest. But for the majority of memcg-context
reclaim, the memcg is not the root memcg and so they will not do
global reclaim anymore...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
