Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] drm/ttm: optimize the pool shrinker a bit v2
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, vbabka@suse.cz, daniel@ffwll.ch,
        ray.huang@amd.com
References: <20210415115624.2904-1-christian.koenig@amd.com>
 <20210415115624.2904-2-christian.koenig@amd.com>
 <20210415133310.1ee9df70a9eb887be937c3a3@linux-foundation.org>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>
Message-ID: <57572373-d68c-80de-7f9e-c04239d1b050@gmail.com>
Date:   Fri, 16 Apr 2021 09:08:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <20210415133310.1ee9df70a9eb887be937c3a3@linux-foundation.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Am 15.04.21 um 22:33 schrieb Andrew Morton:
> On Thu, 15 Apr 2021 13:56:24 +0200 "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> @@ -530,6 +525,11 @@ void ttm_pool_fini(struct ttm_pool *pool)
>>   			for (j = 0; j < MAX_ORDER; ++j)
>>   				ttm_pool_type_fini(&pool->caching[i].orders[j]);
>>   	}
>> +
>> +	/* We removed the pool types from the LRU, but we need to also make sure
>> +	 * that no shrinker is concurrently freeing pages from the pool.
>> +	 */
>> +	sync_shrinkers();
> It isn't immediately clear to me how this works.  ttm_pool_fini() has
> already freed all the pages hasn't it?  So why would it care if some
> shrinkers are still playing with the pages?

Yes ttm_pool_fini() has freed up all pages which had been in the pool 
when the function was called.

But the problem is it is possible that a parallel running shrinker has 
taken a page from the pool and is in the process of freeing it up.

When I return here the pool structure and especially the device 
structure are freed while the parallel running shrinker is still using them.

I could go for a design where we have one shrinker per device instead, 
but that would put a bit to much pressure on the pool in my opinion.

> Or is it the case that ttm_pool_fini() is assuming that there will be
> some further action against these pages, which requires that shrinkers
> no longer be accessing the pages and which further assumes that future
> shrinker invocations will not be able to look up these pages?
>
> IOW, a bit more explanation about the dynamics here would help!

Sorry, I'm not a native speaker of English and sometimes still have a 
hard time explaining things.

Regards,
Christian.