On 2023/6/24 19:08, Qi Zheng wrote:

Hi Dave,

On 2023/6/24 06:19, Dave Chinner wrote:

quoted

On Fri, Jun 23, 2023 at 09:10:57PM +0800, Qi Zheng wrote:

quoted

On 2023/6/23 14:29, Dave Chinner wrote:

quoted

On Thu, Jun 22, 2023 at 05:12:02PM +0200, Vlastimil Babka wrote:

quoted

On 6/22/23 10:53, Qi Zheng wrote:

Yes, I suggested the IDR route because radix tree lookups under RCU
with reference counted objects are a known safe pattern that we can
easily confirm is correct or not.  Hence I suggested the unification
+ IDR route because it makes the life of reviewers so, so much
easier...

In fact, I originally planned to try the unification + IDR method you
suggested at the beginning. But in the case of CONFIG_MEMCG disabled,
the struct mem_cgroup is not even defined, and root_mem_cgroup and
shrinker_info will not be allocated.  This required more code 
changes, so
I ended up keeping the shrinker_list and implementing the above pattern.

Yes. Go back and read what I originally said needed to be done
first. In the case of CONFIG_MEMCG=n, a dummy root memcg still needs
to exist that holds all of the global shrinkers. Then shrink_slab()
is only ever passed a memcg that should be iterated.

Yes, it needs changes external to the shrinker code itself to be
made to work. And even if memcg's are not enabled, we can still use
the memcg structures to ensure a common abstraction is used for the
shrinker tracking infrastructure....

Yeah, what I imagined before was to define a more concise struct
mem_cgroup in the case of CONFIG_MEMCG=n, then allocate a dummy root
memcg on system boot:

#ifdef !CONFIG_MEMCG

struct shrinker_info {
     struct rcu_head rcu;
     atomic_long_t *nr_deferred;
     unsigned long *map;
     int map_nr_max;
};

struct mem_cgroup_per_node {
     struct shrinker_info __rcu    *shrinker_info;
};

struct mem_cgroup {
     struct mem_cgroup_per_node *nodeinfo[];
};

#endif

But I have a concern: if all global shrinkers are tracking with the
info->map of root memcg, a shrinker->id needs to be assigned to them,
which will cause info->map_nr_max to become larger than before, then
making the traversal of info->map slower.

But most of the system is 'sb-xxx' shrinker instances, they all have
the SHRINKER_MEMCG_AWARE flag, so it should have little impact on the
speed of traversing info->map. ;)

quoted

quoted

If the above pattern is not safe, I will go back to the unification +
IDR method.

And that is exactly how we got into this mess in the first place....

I only found one similar pattern in the kernel:

fs/smb/server/oplock.c:find_same_lease_key/smb_break_all_levII_oplock/lookup_lease_in_table

But IIUC, the refcount here needs to be decremented after holding
rcu lock as I did above.

So regardless of whether we choose unification + IDR in the end, I still
want to confirm whether the pattern I implemented above is safe. :)

Also + RCU mailing list.

Thanks,
Qi

quoted

-Dave