Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] msm_gem_shrinker_scan incorrectly returns SHRINK_STOP when pages are successfully freed but the scanned LRU becomes empty, or when the scan yields no freed pages but pages remain. -- commit 9512c359aecfd18777a053ee1d115de39b75b08a 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 the shrinker's ability to block waiting for GEM buffers to become idle. This provides a way to avoid latency issues during memory pressure for sensitive use-cases. > diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c > b/drivers/gpu/drm/msm/msm_gem_shrinker.c > index 3514d5c849896..83ee032cb21d7 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; > return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) || > (current_is_kswapd() && (sc->gfp_mask & __GFP_KSWAPD_RECLAIM)); > } [Severity: High] This is a pre-existing issue, but does msm_gem_shrinker_scan() correctly report reclaimed memory? In drivers/gpu/drm/msm/msm_gem_shrinker.c:msm_gem_shrinker_scan(), the return value uses a ternary operation: return (freed > 0 && remaining > 0) ? freed : SHRINK_STOP; If the scan successfully frees all remaining pages in the LRU (remaining becomes 0, freed > 0), wouldn't this evaluate to SHRINK_STOP? When do_shrink_slab() receives SHRINK_STOP, it breaks its scanning loop and bypasses accounting for the successfully freed pages, causing them to be lost from shrinker accounting. Similarly, if the scan fails to free any pages because the objects are active or locked (freed is 0, remaining > 0), it also evaluates to SHRINK_STOP. Wouldn't this cause do_shrink_slab() to abort scanning the msm_gem slab entirely for this cycle? This means unfreeable objects at the head of the LRU completely block the reclaim of reclaimable objects further down the list, which could severely impact the system's ability to reclaim GPU memory under pressure. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
