Message-ID: <d527005e-5f55-a6f7-21ce-817076860e99@amd.com>
Date:   Thu, 9 Mar 2023 09:11:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v4 7/8] mm: vmscan: remove shrinker_rwsem from
 synchronize_shrinkers()
Content-Language: en-US
To:     Qi Zheng <zhengqi.arch@bytedance.com>, Kirill Tkhai <tkhai@ya.ru>,
        akpm@linux-foundation.org, hannes@cmpxchg.org, shakeelb@google.com,
        mhocko@kernel.org, roman.gushchin@linux.dev, muchun.song@linux.dev,
        david@redhat.com, shy828301@gmail.com, rppt@kernel.org
Cc:     sultan@kerneltoast.com, dave@stgolabs.net,
        penguin-kernel@I-love.SAKURA.ne.jp, paulmck@kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20230307065605.58209-1-zhengqi.arch@bytedance.com>
 <20230307065605.58209-8-zhengqi.arch@bytedance.com>
 <bc829246-4606-cc8d-4a4f-da285255f5a3@ya.ru>
 <07078623-b7ef-ceb4-eccc-8872a4067273@bytedance.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <07078623-b7ef-ceb4-eccc-8872a4067273@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N20ycitxMEozZVQvKzNPQUQyM0J6MDRNc2xlVy9mOXZETXZKYVZ4ejlkK0hU?=
 =?utf-8?B?ZDRpcFhRYVZPZjIzenNNWkhEQ1drZ3JCWDVjR2V5STVSNnlKenF6c1VpS0xZ?=
 =?utf-8?B?My9Vb1FUR2FQS1RUVTdkNi9vZUN1S1ZIWlBrSit4Zm95a3RITnZZNncvK3ZJ?=
 =?utf-8?B?WGphdlpTT3J6VWlhOUlBdWhxOWNreFRCWUorVGkrQ3Q5TmhqM3hXS2VEYVNo?=
 =?utf-8?B?YXR3eHJsV1ZIZ1R5RmRyNVg2TkFONUpCM3IycmgrQzF1andKNnZva0VLVXoy?=
 =?utf-8?B?NFBNQTRhN2o0MXBWT3BTSWg2THg1ZHNTSXR5WFk1akxJZFFrdlU1Y0UzZS9a?=
 =?utf-8?B?RndrTDVtQXVFeGJMYTc5YmpLSCtsRk0wYit5M0l6a2lKZWc0cGJFV0ZtdS83?=
 =?utf-8?B?bms2SmMzV29zWm12dW0vOHMrSjFnd0w1YVZ4WkZXOW1iQnpCMzJGd2FRWEFK?=
 =?utf-8?B?dmIrVWZXK3I5NWFHMUd5ZmYydU1aZUYvZ2tFb0ZKcmozeHFCOEYxbks4cFpl?=
 =?utf-8?B?enlQUGdvZ2NVbjg2YjkvNHE1MnZWMGE0K29MZEwyN0N1ODJFWFJzTXYvbkVk?=
 =?utf-8?B?UmlHNWMxWG0yK2YzbUgwMGVyNGQrbkQxRXZzRDlUTElUTFMyZnBESWo5bjky?=
 =?utf-8?B?YzB2aGQybGJ0UlU5WXFZSVhlR0pBM2tpNGdGNWZrTEEra0NSNnhVQnNKRFJV?=
 =?utf-8?B?aHozZWlCaXNnOWNhVDJmMUhYbEJZckFJMStxWGdBRXc3cnI1TDJuTHJES3BN?=
 =?utf-8?B?ajE4M3Fqd09CdjRXcURveE16YzUxVTlRTFZNLzNqTXNmOThGMC9SakFaTHBo?=
 =?utf-8?B?RU1VOWRvQ2QvbEhHQnBLbzcyQzJhWXArTFlsRmNNZnR2b2V2VVFpeVBLZlFO?=
 =?utf-8?B?QnpSVG0rVjJETUpqZkdFQUV0VFh4Tk9STU9jTzdxQ21PM0VOU00vdVFwZGd4?=
 =?utf-8?B?QmhsVmRadWdodjJVRDBkSGRYdHI2eDRwdXZ3dE9DQkRjdTZsSUI5Z3VWaXZa?=
 =?utf-8?B?TEpKeGQ4bkhnRkFpaTIvdTdRYjc3NmF6Q0FaK25sVnRvZEtneW1IQTFLWHNt?=
 =?utf-8?B?VnpnbUU3K0FTazk0Z2JNZjA3eU5jaGxpVEhPVkw4VEZBQTBFcDVPZWNrb0V3?=
 =?utf-8?B?SUZ5UUgxWENQMlNIbXBINS9udlZuZTVrdzRncEJoY1lyZ0U0U1JIME5UMCtX?=
 =?utf-8?B?V2EvQnRob0dZZEZKSCs3dkFEU3dqTXdndWdjNmJJL1ZYdXE5OUMxTFc1RVVK?=
 =?utf-8?B?YUhFOW9HWnhJOWVkMHpIZHJjaHFrSmxmL3hOV2hEeHdqc3ZRUXlNbjBCWnhV?=
 =?utf-8?B?bXZIVHpGNkYwdng0OERJWWVSRFg5ZUJhSTFSN0kwK2h3ZWpRZW9hVmVya1N2?=
 =?utf-8?B?eVdsK1pWanczN2hvcTdwMVJhakJvMjBEWVNRc2FOL0o4STl1bEFoRjZaMmRQ?=
 =?utf-8?B?RGZGQ2NYUFA2QVVxSVM0bUhhZlBRY3VYaFdGV1hzQ2I4TXg0T244YmI1eXgz?=
 =?utf-8?B?MGUwbmpoTE43aTUweUhBYkdXamYvcXJzdExjV2g2bXdxSXdSS3BKMVdQdk9Q?=
 =?utf-8?B?SVBSdW11Qk1QRUJ3bTgrTlVtdEw3VEFua2ovZDVGcU9zNUR3ZVBydVlkbjdt?=
 =?utf-8?B?N2RpY0tKeVhDZnpvV0RIZ2xHVVZIRlZSUlJ5MGtwQWdGTHpDVVV1RHRzeVJw?=
 =?utf-8?B?Z3FjSnJoS1Aza21Lb3p6WlAyQTB4ay9NeHgxM3o2U2hhQ1ZVTWJlMkdDMldT?=
 =?utf-8?B?VU1oZEdGUEZCSi96MmdYV1lHNGNlSHBURkNWcEY1UlkyWjhYaDgreXRFczlR?=
 =?utf-8?B?cDZqamdVWStEZmxNZk1rWjNYS1VJeFdmNXNzUWhKemREcGF2eXdyaUw4Y3BC?=
 =?utf-8?B?NytmbGZBR3hHWlN0TytwMGhOczBWbVU5R1N2SUlKYnlJK2crOU9sOVM4c25o?=
 =?utf-8?B?NXlzM3g1VXRkUHVHUEFJQURSVW5oSTM4SDFOcS81VWhJczBoWjJyYkx6ci9l?=
 =?utf-8?B?Q09XbFdLTjVQbDBKSGVMT29TSEJ4TWFZdW1BMjU2N3dSMlpIS2swREpXU3NW?=
 =?utf-8?B?SFBGbzF1OGxNZHBWOXl2NENqa2pUMVVEOVJQdlBEeVFIcWdFNDIwOXEvaXpv?=
 =?utf-8?Q?+hloBONxFkUu+aj2P+mZJhwTe?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c474cd29-69ab-4e28-d490-08db2075f62d
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 08:12:01.8861
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7kePNtyc1zKpoIUvk0cl61Gb625DhDEJCZc22RaC+/loy2LEWyvXquizParg0X54
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5328
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 09.03.23 um 08:06 schrieb Qi Zheng:
> Hi Kirill,
>
> On 2023/3/9 06:39, Kirill Tkhai wrote:
>> On 07.03.2023 09:56, Qi Zheng wrote:
>>> Now there are no readers of shrinker_rwsem, so
>>> synchronize_shrinkers() does not need to hold the
>>> writer of shrinker_rwsem to wait for all running
>>> shinkers to complete, synchronize_srcu() is enough.
>>>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>>> ---
>>>   mm/vmscan.c | 8 ++------
>>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 7aaf6f94ac1b..ac7ab4aa344f 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -796,15 +796,11 @@ EXPORT_SYMBOL(unregister_shrinker);
>>>   /**
>>>    * synchronize_shrinkers - Wait for all running shrinkers to 
>>> complete.
>>>    *
>>> - * This is equivalent to calling unregister_shrink() and 
>>> register_shrinker(),
>>> - * but atomically and with less overhead. This is useful to 
>>> guarantee that all
>>> - * shrinker invocations have seen an update, before freeing memory, 
>>> similar to
>>> - * rcu.
>>> + * This is useful to guarantee that all shrinker invocations have 
>>> seen an
>>> + * update, before freeing memory.
>>>    */
>>>   void synchronize_shrinkers(void)
>>>   {
>>> -    down_write(&shrinker_rwsem);
>>> -    up_write(&shrinker_rwsem);
>>>       atomic_inc(&shrinker_srcu_generation);
>>>       synchronize_srcu(&shrinker_srcu);
>>>   }
>>
>> Just curious, callers of synchronize_shrinkers() don't want to have 
>> parallel register_shrinker() and unregister_shrink() are completed?
>> Here we only should wait for parallel shrink_slab(), correct?
>
> I think yes.
>
> The synchronize_shrinkers() is currently only used by TTM pool.
>
> In TTM pool, a shrinker named "drm-ttm_pool" is registered, and
> the scan_objects callback will pick an entry from its own shrinker_list:
>
> ttm_pool_shrink
> --> spin_lock(&shrinker_lock);
>     pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
>     list_move_tail(&pt->shrinker_list, &shrinker_list);
>     spin_unlock(&shrinker_lock);
>
> These entries have been removed from shrinker_list before calling
> synchronize_shrinkers():
>
> ttm_pool_fini
> --> ttm_pool_type_fini
>     --> spin_lock(&shrinker_lock);
>     list_del(&pt->shrinker_list);
>     spin_unlock(&shrinker_lock);
>     synchronize_shrinkers
>
> So IIUC, we only need to wait for the parallel shrink_slab() here. Like
> its comment says:
>
> /* 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.
>  */

Yes your analyses is completely correct.

I just didn't wanted to add another SRCU into the critical code paths of 
the TTM pool just for device hot plug when I have that functionality 
already.

We just make sure that no shrinker is running in parallel with 
destruction of the pool. Registering and unregistering is harmless.

Regards,
Christian.

>
> + CC: Christian König :)
>
> Thanks,
> Qi