Re: [v2 PATCH 3/9] mm: vmscan: guarantee shrinker_slab_memcg() sees valid shrinker_maps for online memcg

I think Johannes's point makes sense to me. If the shrinker_maps is
not initialized yet it means the memcg is too young to have a number
of reclaimable slab caches. It sounds fine to just skip it.

And, with consolidating shrinker_maps and shrinker_deferred into one
struct, we could just check the pointer of the struct. So, it seems
this patch is not necessary anymore. This patch will be dropped in v3.

On Tue, Dec 15, 2020 at 12:31 PM Yang Shi [off-list ref] wrote:

On Tue, Dec 15, 2020 at 9:16 AM Johannes Weiner [off-list ref] wrote:

quoted

On Mon, Dec 14, 2020 at 02:37:16PM -0800, Yang Shi wrote:

quoted

The shrink_slab_memcg() races with mem_cgroup_css_online(). A visibility of CSS_ONLINE flag
in shrink_slab_memcg()->mem_cgroup_online() does not guarantee that we will see
memcg->nodeinfo[nid]->shrinker_maps != NULL.  This may occur because of processor reordering
on !x86.

This seems like the below case:

           CPU A          CPU B
store shrinker_map      load CSS_ONLINE
store CSS_ONLINE        load shrinker_map

But we have a separate check on shrinker_maps, so it doesn't matter
that it isn't guaranteed, no?

IIUC, yes. Checking shrinker_maps is the alternative way to detect the
reordering to prevent from seeing NULL shrinker_maps per Kirill.

We could check shrinker_deferred too, then just walk away if it is NULL.

quoted

The only downside I can see is when CSS_ONLINE isn't visible yet and
we bail even though we'd be ready to shrink. Although it's probably
unlikely that there would be any objects allocated already...

Yes, it seems so.

quoted

Can somebody remind me why we check mem_cgroup_online() at all?

IIUC it should be mainly used to skip offlined memcgs since there is
nothing on offlined memcgs' LRU because all objects have been
reparented. But shrinker_map won't be freed until .css_free is called.
So the shrinkers might be called in vain.

quoted

If shrinker_map is set, we can shrink: .css_alloc is guaranteed to be
complete, and by using RCU for the shrinker_map pointer, the map is
also guaranteed to be initialized. There is nothing else happening
during onlining that you may depend on.

If shrinker_map isn't set, we cannot iterate the bitmap. It does not
really matter whether CSS_ONLINE is reordered and visible already.

As I mentioned above it should be used to skip offlined memcgs, but it
also opens the race condition due to memory reordering. As Kirill
explained in the earlier email, we could either check the pointer or
use memory barriers.

If the memory barriers seems overkilling, I could definitely switch
back to NULL pointer check approach.

quoted

Agreed with Dave: if we need that synchronization around onlining, it
needs to happen inside the cgroup core. But I wouldn't add that until
somebody actually required it.