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 <[email protected]> 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/[email protected]?part=2
