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 1/2] mm/vmscan: add sync_shrinkers function
To:     Vlastimil Babka <vbabka@suse.cz>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Cc:     ray.huang@amd.com, daniel@ffwll.ch, akpm@linux-foundation.org
References: <20210409071725.1532-1-christian.koenig@amd.com>
 <462c2a51-4aa8-47ba-1c67-171ca651b016@suse.cz>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>
Message-ID: <951bf630-35e4-9b5a-2ace-007a685d1994@gmail.com>
Date:   Fri, 9 Apr 2021 13:04:22 +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: <462c2a51-4aa8-47ba-1c67-171ca651b016@suse.cz>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Am 09.04.21 um 13:00 schrieb Vlastimil Babka:
> On 4/9/21 9:17 AM, Christian König wrote:
>> To be able to switch to a spinlock and reduce lock contention in the TTM
>> shrinker we don't want to hold a mutex while unmapping and freeing pages
>> from the pool.
> Does using spinlock instead of mutex really reduce lock contention?

Well using the spinlock instead of the mutex is only the cherry on the cake.

The real improvement for the contention is the fact that we just grab 
the next pool and drop the lock again instead of doing the whole IOMMU 
unmap and flushing of the CPU TLB dance while holding the lock.

>> But then we somehow need to prevent a race between (for example) the shrinker
>> trying to free pages and hotplug trying to remove the device which those pages
>> belong to.
>>
>> Taking and releasing the shrinker semaphore on the write side after
>> unmapping and freeing all pages should make sure that no shrinker is running in
>> paralell any more.
> So you explain this in this commit log for adding the function, but then the
> next patch just adds a sync_shrinkers() call without any comment. I would expect
> there a comment explaining why it's done there - what it protects against, as
> it's not an obvious pattern IMHO.

Good point, going to add a comment.

Thanks,
Christian.

>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   include/linux/shrinker.h |  1 +
>>   mm/vmscan.c              | 10 ++++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
>> index 0f80123650e2..6b75dc372fce 100644
>> --- a/include/linux/shrinker.h
>> +++ b/include/linux/shrinker.h
>> @@ -92,4 +92,5 @@ extern void register_shrinker_prepared(struct shrinker *shrinker);
>>   extern int register_shrinker(struct shrinker *shrinker);
>>   extern void unregister_shrinker(struct shrinker *shrinker);
>>   extern void free_prealloced_shrinker(struct shrinker *shrinker);
>> +extern void sync_shrinkers(void);
>>   #endif
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 562e87cbd7a1..46cd9c215d73 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -408,6 +408,16 @@ void unregister_shrinker(struct shrinker *shrinker)
>>   }
>>   EXPORT_SYMBOL(unregister_shrinker);
>>   
>> +/**
>> + * sync_shrinker - Wait for all running shrinkers to complete.
>> + */
>> +void sync_shrinkers(void)
>> +{
>> +	down_write(&shrinker_rwsem);
>> +	up_write(&shrinker_rwsem);
>> +}
>> +EXPORT_SYMBOL(sync_shrinkers);
>> +
>>   #define SHRINK_BATCH 128
>>   
>>   static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
>>