Received-SPF: pass (google.com: domain of linux-kernel+bounces-142318-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <d78ec9ea-006a-4317-b0df-99ad31994b0d@bytedance.com>
Date: Fri, 12 Apr 2024 16:47:30 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/shrinker: add SHRINKER_NO_DIRECT_RECLAIM
Content-Language: en-US
To: lipeifeng@oppo.com
Cc: akpm@linux-foundation.org, david@fromorbit.com, roman.gushchin@linux.dev,
 muchun.song@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20240412080706.744-1-lipeifeng@oppo.com>
From: Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <20240412080706.744-1-lipeifeng@oppo.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi Peifeng,

On 2024/4/12 16:07, lipeifeng@oppo.com wrote:
> From: Peifeng Li <lipeifeng@oppo.com>
> 
> In the case of insufficient memory, threads will be in direct_reclaim to
> reclaim memory, direct_reclaim will call shrink_slab to run sequentially
> each shrinker callback. If there is a lock-contention in the shrinker
> callback,such as spinlock,mutex_lock and so on, threads may be likely to
> be stuck in direct_reclaim for a long time, even if the memfree has reached
> the high watermarks of the zone, resulting in poor performance of threads.
> 
> Example 1: shrinker callback may wait for spinlock
> static unsigned long mb_cache_shrink(struct mb_cache *cache,
>                                       unsigned long nr_to_scan)
> {
>          struct mb_cache_entry *entry;
>          unsigned long shrunk = 0;
> 
>          spin_lock(&cache->c_list_lock);
>          while (nr_to_scan-- && !list_empty(&cache->c_list)) {
>                  entry = list_first_entry(&cache->c_list,
>                                           struct mb_cache_entry, e_list);
>                  if (test_bit(MBE_REFERENCED_B, &entry->e_flags) ||
>                      atomic_cmpxchg(&entry->e_refcnt, 1, 0) != 1) {
>                          clear_bit(MBE_REFERENCED_B, &entry->e_flags);
>                          list_move_tail(&entry->e_list, &cache->c_list);
>                          continue;
>                  }
>                  list_del_init(&entry->e_list);
>                  cache->c_entry_count--;
>                  spin_unlock(&cache->c_list_lock);
>                  __mb_cache_entry_free(cache, entry);
>                  shrunk++;
>                  cond_resched();
>                  spin_lock(&cache->c_list_lock);
>          }
>          spin_unlock(&cache->c_list_lock);
> 
>          return shrunk;
> }
> Example 2: shrinker callback may wait for mutex lock
> static
> unsigned long kbase_mem_evictable_reclaim_scan_objects(struct shrinker *s,
> 		struct shrink_control *sc)
> {
> 	struct kbase_context *kctx;
> 	struct kbase_mem_phy_alloc *alloc;
> 	struct kbase_mem_phy_alloc *tmp;
> 	unsigned long freed = 0;
> 
> 	kctx = container_of(s, struct kbase_context, reclaim);
> 
> 	// MTK add to prevent false alarm
> 	lockdep_off();
> 
> 	mutex_lock(&kctx->jit_evict_lock);
> 
> 	list_for_each_entry_safe(alloc, tmp, &kctx->evict_list, evict_node) {
> 		int err;
> 
> 		err = kbase_mem_shrink_gpu_mapping(kctx, alloc->reg,
> 				0, alloc->nents);
> 		if (err != 0) {
> 			freed = -1;
> 			goto out_unlock;
> 		}
> 
> 		alloc->evicted = alloc->nents;
> 
> 		kbase_free_phy_pages_helper(alloc, alloc->evicted);
> 		freed += alloc->evicted;
> 		list_del_init(&alloc->evict_node);
> 
> 		kbase_jit_backing_lost(alloc->reg);
> 
> 		if (freed > sc->nr_to_scan)
> 			break;
> 	}
> out_unlock:
> 	mutex_unlock(&kctx->jit_evict_lock);
> 
> 	// MTK add to prevent false alarm
> 	lockdep_on();
> 
> 	return freed;
> }
> 
> In mobile-phone,threads are likely to be stuck in shrinker callback during
> direct_reclaim, with example like the following:
> <...>-2806    [004] ..... 866458.339840: mm_shrink_slab_start:
> 			dynamic_mem_shrink_scan+0x0/0xb8 ... priority 2
> <...>-2806    [004] ..... 866459.339933: mm_shrink_slab_end:
> 			dynamic_mem_shrink_scan+0x0/0xb8 ...
> 
> For the above reason, the patch introduces SHRINKER_NO_DIRECT_RECLAIM that
> allows driver to set shrinker callback not to be called in direct_reclaim
> unless sc->priority is 0.

Hmm, this is just a workaround, no shrinker will want to set this flag.
If a shrinker has a lock contention problem, then it needs to be fixed.

Perhaps executing do_shrink_slab() asynchronously may be a more
fundamental solution, but this may result in untimely reclamation.

> 
> The reason why sc->priority=0 allows shrinker callback to be called in
> direct_reclaim is for two reasons:
> 1.Always call all shrinker callback in drop_slab that priority is 0.
> 2.sc->priority is 0 during direct_reclaim, allow direct_reclaim to call
> shrinker callback, to reclaim memory timely.
> 
> Note:
> 1.Register_shrinker_prepared() default not to set

This API is no longer included in the latest upstream code. Please
submit a patch based on the latest code.

Thanks,
Qi

> SHRINKER_NO_DIRECT_RECLAIM, to maintain the current behavior of the code.
> 2.Logic of kswapd and drop_slab to call shrinker callback isn't affected.
> 
> Signed-off-by: Peifeng Li <lipeifeng@oppo.com>
> ---
>   include/linux/shrinker.h |  5 +++++
>   mm/shrinker.c            | 36 ++++++++++++++++++++++++++++++++++--
>   2 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 1a00be90d93a..2d5a8b3a720b 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -130,6 +130,11 @@ struct shrinker {
>    * non-MEMCG_AWARE shrinker should not have this flag set.
>    */
>   #define SHRINKER_NONSLAB	BIT(4)
> +/*
> + * Can shrinker callback be called in direct_relcaim unless
> + * sc->priority is 0?
> + */
> +#define SHRINKER_NO_DIRECT_RECLAIM	BIT(5)
>   
>   __printf(2, 3)
>   struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...);
> diff --git a/mm/shrinker.c b/mm/shrinker.c
> index dc5d2a6fcfc4..3ac50da72494 100644
> --- a/mm/shrinker.c
> +++ b/mm/shrinker.c
> @@ -544,7 +544,23 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>   			if (!memcg_kmem_online() &&
>   			    !(shrinker->flags & SHRINKER_NONSLAB))
>   				continue;
> -
> +			/*
> +			 * SHRINKER_NO_DIRECT_RECLAIM, mean that shrinker callback
> +			 * should not be called in direct_reclaim unless priority
> +			 * is 0.
> +			 */
> +			if ((shrinker->flags & SHRINKER_NO_DIRECT_RECLAIM) &&
> +					!current_is_kswapd()) {
> +				/*
> +				 * 1.Always call shrinker callback in drop_slab that
> +				 * priority is 0.
> +				 * 2.sc->priority is 0 during direct_reclaim, allow
> +				 * direct_reclaim to call shrinker callback, to reclaim
> +				 * memory timely.
> +				 */
> +				if (priority)
> +					continue;
> +			}
>   			ret = do_shrink_slab(&sc, shrinker, priority);
>   			if (ret == SHRINK_EMPTY) {
>   				clear_bit(offset, unit->map);
> @@ -658,7 +674,23 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
>   			continue;
>   
>   		rcu_read_unlock();
> -
> +		/*
> +		 * SHRINKER_NO_DIRECT_RECLAIM, mean that shrinker callback
> +		 * should not be called in direct_reclaim unless priority
> +		 * is 0.
> +		 */
> +		if ((shrinker->flags & SHRINKER_NO_DIRECT_RECLAIM) &&
> +				!current_is_kswapd()) {
> +			/*
> +			 * 1.Always call shrinker callback in drop_slab that
> +			 * priority is 0.
> +			 * 2.sc->priority is 0 during direct_reclaim, allow
> +			 * direct_reclaim to call shrinker callback, to reclaim
> +			 * memory timely.
> +			 */
> +			if (priority)
> +				continue;
> +		}
>   		ret = do_shrink_slab(&sc, shrinker, priority);
>   		if (ret == SHRINK_EMPTY)
>   			ret = 0;