On 2/17/23 15:02, Thomas Zimmermann wrote:
> Hi
>
> Am 08.01.23 um 22:04 schrieb Dmitry Osipenko:
>> Consider this scenario:
>>
>> 1. APP1 continuously creates lots of small GEMs
>> 2. APP2 triggers `drop_caches`
>> 3. Shrinker starts to evict APP1 GEMs, while APP1 produces new purgeable
>> GEMs
>> 4. msm_gem_shrinker_scan() returns non-zero number of freed pages
>> and causes shrinker to try shrink more
>> 5. msm_gem_shrinker_scan() returns non-zero number of freed pages again,
>> goto 4
>> 6. The APP2 is blocked in `drop_caches` until APP1 stops producing
>> purgeable GEMs
>>
>> To prevent this blocking scenario, check number of remaining pages
>> that GPU shrinker couldn't release due to a GEM locking contention
>> or shrinking rejection. If there are no remaining pages left to shrink,
>> then there is no need to free up more pages and shrinker may break out
>> from the loop.
>>
>> This problem was found during shrinker/madvise IOCTL testing of
>> virtio-gpu driver. The MSM driver is affected in the same way.
>>
>> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
>> Fixes: b352ba54a820 ("drm/msm/gem: Convert to using drm_gem_lru")
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_gem.c | 9 +++++++--
>> drivers/gpu/drm/msm/msm_gem_shrinker.c | 8 ++++++--
>> include/drm/drm_gem.h | 4 +++-
>> 3 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 59a0bb5ebd85..c6bca5ac6e0f 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1388,10 +1388,13 @@ EXPORT_SYMBOL(drm_gem_lru_move_tail);
>> *
>> * @lru: The LRU to scan
>> * @nr_to_scan: The number of pages to try to reclaim
>> + * @remaining: The number of pages left to reclaim
>> * @shrink: Callback to try to shrink/reclaim the object.
>> */
>> unsigned long
>> -drm_gem_lru_scan(struct drm_gem_lru *lru, unsigned nr_to_scan,
>> +drm_gem_lru_scan(struct drm_gem_lru *lru,
>> + unsigned int nr_to_scan,
>> + unsigned long *remaining,
>> bool (*shrink)(struct drm_gem_object *obj))
>> {
>> struct drm_gem_lru still_in_lru;
>> @@ -1430,8 +1433,10 @@ drm_gem_lru_scan(struct drm_gem_lru *lru,
>> unsigned nr_to_scan,
>> * hit shrinker in response to trying to get backing pages
>> * for this obj (ie. while it's lock is already held)
>> */
>> - if (!dma_resv_trylock(obj->resv))
>> + if (!dma_resv_trylock(obj->resv)) {
>> + *remaining += obj->size >> PAGE_SHIFT;
>> goto tail;
>> + }
>> if (shrink(obj)) {
>> freed += obj->size >> PAGE_SHIFT;
>> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
>> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
>> index 051bdbc093cf..b7c1242014ec 100644
>> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
>> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
>> @@ -116,12 +116,14 @@ msm_gem_shrinker_scan(struct shrinker *shrinker,
>> struct shrink_control *sc)
>> };
>> long nr = sc->nr_to_scan;
>> unsigned long freed = 0;
>> + unsigned long remaining = 0;
>> for (unsigned i = 0; (nr > 0) && (i < ARRAY_SIZE(stages)); i++) {
>> if (!stages[i].cond)
>> continue;
>> stages[i].freed =
>> - drm_gem_lru_scan(stages[i].lru, nr, stages[i].shrink);
>> + drm_gem_lru_scan(stages[i].lru, nr, &remaining,
>
> This function relies in remaining being pre-initialized. That's not
> obvious and error prone. At least, pass-in something like
> &stages[i].remaining that is then initialized internally by
> drm_gem_lru_scan() to zero. And similar to freed, sum up the individual
> stages' remaining here.
>
> TBH I somehow don't like the overall design of how all these functions
> interact with each other. But I also can't really point to the actual
> problem. So it's best to take what you have here; maybe with the change
> I proposed.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
I had to keep to the remaining being pre-initialized because moving the
initialization was hurting the rest of the code. Though, updated the MSM
patch to use &stages[i].remaining
--
Best regards,
Dmitry
