Re: [PATCH 2/2] drm/msm/gem: Add modparam to disable shrinker blocking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The shrinker scan function `msm_gem_shrinker_scan` loses count of freed objects and aborts prematurely by returning `SHRINK_STOP` when all scanned objects are successfully freed.
--

commit 1da508dba68cbcbae647ca4f3177e66e5a19e756
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Add modparam to disable shrinker blocking

This commit adds a module parameter, eviction_can_block, to disable
blocking for GEM buffers to become idle during shrinker eviction. This
helps prevent the shrinker from waiting on idle BOs during high memory
pressure, which is undesirable in latency-sensitive use-cases.

> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 9d2788f79acee..db57609046d1b 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -19,6 +19,10 @@ static bool enable_eviction = true;
>  MODULE_PARM_DESC(enable_eviction, "Enable swappable GEM buffers");
>  module_param(enable_eviction, bool, 0600);
>  
> +static bool eviction_can_block = true;
> +MODULE_PARM_DESC(eviction_can_block, "Enable blocking for GEM buffer to become idle for eviction");
> +module_param(eviction_can_block, bool, 0600);
> +
>  static bool can_swap(void)
>  {
>  	return enable_eviction && get_nr_swap_pages() > 0;
> @@ -26,6 +30,8 @@ static bool can_swap(void)
>  
>  static bool can_block(struct shrink_control *sc)
>  {
> +	if (!eviction_can_block)
> +		return false;

[Severity: High]
This is a pre-existing issue, but I noticed the shrinker scan function can
lose count of freed objects and abort prematurely.

When the VM calls msm_gem_shrinker_scan(), it loops over LRU stages calling
drm_gem_lru_scan():

drivers/gpu/drm/msm/msm_gem_shrinker.c:msm_gem_shrinker_scan() {
    ...
    for (unsigned i = 0; (nr > 0) && (i < ARRAY_SIZE(stages)); i++) {
        ...
        stages[i].freed =
            drm_gem_lru_scan(priv->dev, stages[i].lru, nr,
                     &stages[i].remaining,
                     stages[i].shrink,
                     NULL);
        nr -= stages[i].freed;
        freed += stages[i].freed;
        remaining += stages[i].remaining;
    }
    ...
    return (freed > 0 && remaining > 0) ? freed : SHRINK_STOP;
}

In drm_gem_lru_scan(), the remaining parameter is only populated with objects
that failed to be shrunk:

drivers/gpu/drm/drm_gem.c:drm_gem_lru_scan() {
    ...
        if (!ww_mutex_trylock(&obj->resv->lock, ticket)) {
            *remaining += obj->size >> PAGE_SHIFT;
            goto tail;
        }
    ...
}

If all scanned objects in a batch are successfully freed without encountering
a locked object, remaining will be 0. The return expression
(freed > 0 && remaining > 0) ? freed : SHRINK_STOP; then evaluates to
SHRINK_STOP instead of the number of freed items.

Does this prematurely abort the core memory management shrinker
(do_shrink_slab) and under-report the amount of reclaimed memory,
potentially degrading memory reclaim efficiency and triggering unnecessary
OOM kills?

>  	return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) ||
>  	       (current_is_kswapd() && (sc->gfp_mask & __GFP_KSWAPD_RECLAIM));
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903183725.15835-1-robin.clark@oss.qualcomm.com?part=2