On Wed, Mar 21, 2018 at 04:22:51PM +0300, Kirill Tkhai wrote:
> Using the preparations made in previous patches, in case of memcg
> shrink, we may avoid shrinkers, which are not set in memcg's shrinkers
> bitmap. To do that, we separate iterations over memcg-aware and
> !memcg-aware shrinkers, and memcg-aware shrinkers are chosen
> via for_each_set_bit() from the bitmap. In case of big nodes,
> having many isolated environments, this gives significant
> performance growth. See next patch for the details.
>
> Note, that the patch does not respect to empty memcg shrinkers,
> since we never clear the bitmap bits after we set it once.
> Their shrinkers will be called again, with no shrinked objects
> as result. This functionality is provided by next patch.
>
> Signed-off-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
> ---
> mm/vmscan.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 265cf069b470..e1fd16bc7a9b 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -327,6 +327,8 @@ static int alloc_shrinker_id(struct shrinker *shrinker)
>
> if (!(shrinker->flags & SHRINKER_MEMCG_AWARE))
> return 0;
> + BUG_ON(!(shrinker->flags & SHRINKER_NUMA_AWARE));
> +
> retry:
> ida_pre_get(&bitmap_id_ida, GFP_KERNEL);
> down_write(&bitmap_rwsem);
> @@ -366,7 +368,8 @@ static void add_shrinker(struct shrinker *shrinker)
> down_write(&shrinker_rwsem);
> if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> mcg_shrinkers[shrinker->id] = shrinker;
> - list_add_tail(&shrinker->list, &shrinker_list);
> + else
> + list_add_tail(&shrinker->list, &shrinker_list);
I don't think we should remove per-memcg shrinkers from the global
shrinker list - this is confusing. It won't be critical if we iterate
over all shrinkers on global reclaim, will it?
> up_write(&shrinker_rwsem);
> }
>
> @@ -701,6 +705,39 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
> if (!down_read_trylock(&shrinker_rwsem))
> goto out;
>
> +#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
> + if (!memcg_kmem_enabled() || memcg) {
> + struct shrinkers_map *map;
> + int i;
> +
> + map = rcu_dereference_protected(SHRINKERS_MAP(memcg), true);
> + if (map) {
> + for_each_set_bit(i, map->map[nid], bitmap_nr_ids) {
> + struct shrink_control sc = {
> + .gfp_mask = gfp_mask,
> + .nid = nid,
> + .memcg = memcg,
> + };
> +
> + shrinker = mcg_shrinkers[i];
> + if (!shrinker) {
> + clear_bit(i, map->map[nid]);
> + continue;
> + }
> + freed += do_shrink_slab(&sc, shrinker, priority);
> +
> + if (rwsem_is_contended(&shrinker_rwsem)) {
> + freed = freed ? : 1;
> + goto unlock;
> + }
> + }
> + }
> +
> + if (memcg_kmem_enabled() && memcg)
> + goto unlock;
May be, factor this out to a separate function, say shrink_slab_memcg?
Just for the sake of code legibility.
