Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <01213258-6e27-f304-b420-f3d915e54ed1@amd.com>
Date:   Wed, 16 Aug 2023 11:14:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 4/5] drm/ttm: introduce pool_shrink_rwsem
Content-Language: en-US
To:     Qi Zheng <zhengqi.arch@bytedance.com>, akpm@linux-foundation.org,
        david@fromorbit.com, tkhai@ya.ru, vbabka@suse.cz,
        roman.gushchin@linux.dev, djwong@kernel.org, brauner@kernel.org,
        paulmck@kernel.org, tytso@mit.edu, steven.price@arm.com,
        cel@kernel.org, senozhatsky@chromium.org, yujie.liu@intel.com,
        gregkh@linuxfoundation.org, muchun.song@linux.dev,
        joel@joelfernandes.org
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        dri-devel@lists.freedesktop.org, linux-fsdevel@vger.kernel.org,
        Muchun Song <songmuchun@bytedance.com>
References: <20230816083419.41088-1-zhengqi.arch@bytedance.com>
 <20230816083419.41088-5-zhengqi.arch@bytedance.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20230816083419.41088-5-zhengqi.arch@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?UHEwU3cwWnN3QjQ1ZEEwR0lOSEgxblpOL1RiM3NQRDcxUEpCWjVoVWZzMFhn?=
 =?utf-8?B?eDVMOFlqRW8vSW1VdThaL3FqTXRLOG1kS3Rhb05pT1pqTEMzNjVObHNPMUFj?=
 =?utf-8?B?MWJlRU03WWdxNDRKM1duYTgySEJSY3REUys2SnlRUVRJcWgreTNnWUdzRGpR?=
 =?utf-8?B?bmtSZzRGUDZLS0V4YWYzMWtwQndrZGhuU1hQNzlBSEdWblgxanQ1Ukp6QVdL?=
 =?utf-8?B?WHNtODUrOHhnRjFpQkR6b2h0NkVGNDFJeXZVMDE0NkZkMXoxMHhQMjBDOEk3?=
 =?utf-8?B?ZExhVm1xRWlrMEl3QUFjL0tQSzhXeXRHZUFjK1hkc1NDSXpIK3ZpRll5U1ZL?=
 =?utf-8?B?cUtFNFZMOHpZY0lMM2VVTDlUN2Mzd1RDM3FrZXVRNEg1U2lYMUVwU0pwYUor?=
 =?utf-8?B?a2tHOFRYRnY0ZFdJRFZyc0RzN3NyY2JLMzZNNDRoa1BqenRKeEVUbmNGaVg5?=
 =?utf-8?B?SXRBcjFhUS9tMU1PRk5SOWRYd3ozcDkwWUdoK2kxNEVmSDlwQUxaZGhCcXBB?=
 =?utf-8?B?Z1VYaGljMkRSMllTbWpkTDl0N0pLS01aSFdPaTJoRkdsbjg1S1VkeFo0dWts?=
 =?utf-8?B?NUE1SXJBeEVGUGxtV0VPUXRZdXVqR2IrQjJQb3c3MHVLVzFhNDM0clBRWmZz?=
 =?utf-8?B?OG1rK1NuL1orRTQ2eTZmWXB4Q3oxYWwwNDZiSnpxcXhkNkxMYll3Z2ZhTjdm?=
 =?utf-8?B?aEo1Qmtab2Zyc2FtQ0hrU3VmY2tXQjhhWTJpRGorS3NYU2ZMdGJsaG96WGgr?=
 =?utf-8?B?dlVKeU1ySHlQTjVtNDUwSTF4enpSQzM1NXpQUWp1UVFBMzBRQ28yS2JsZ2hL?=
 =?utf-8?B?dEZZRXNTNFpXcHhBRHNQTXlZVk12ei9uVlpxRTlvcEF5dGpiWnIrS1JiRGlR?=
 =?utf-8?B?bGd1Z3YwZENCMEEwN1p1bFVFZ2JoYmhMREJERUJzRk1kYkQvS1prVVBwOER2?=
 =?utf-8?B?S3F5Z1pKV0FQQkkzMEQ0ajB3c3ZyMDJwT05CeUk5bUh0NVBvelMySkJQZ3E4?=
 =?utf-8?B?M0dqRVU5dUZmUCs2QjFUek9PVWpVL1JXQVBuNVE1c1VmYzlGR2pFSndaQk4z?=
 =?utf-8?B?WGlnRjdLRHI5c3JKSXgvSEJzbXppK3lhcU9Lb0ZqajQ3WTQ5b2ttUzkyQS9z?=
 =?utf-8?B?TXBFTmJtVlREa3ZwWEJ2ZDdkSEhXcEJLR2ZpKzV6b0c3dFF6SFRzMUpLb0Rv?=
 =?utf-8?B?clp3YVB4RkcvaDdpVDFwbFNxbkpGYW8xVXErRm5zSmdIYUxXeUZTRHU2UnZS?=
 =?utf-8?B?MnloNHlMYnB4ZEQrMmtESWR0VEFsWXBycFI5T3VjVHcweG9Mdml2NFpmbENp?=
 =?utf-8?B?TC8zcTNIMGxxLzc2Z3hQaDJZYkJ4c2FvdVpzZ00rcXdCT3dRVDZmNktCcm1u?=
 =?utf-8?B?d0RzT1hnRUJqY1pkOXpsVFVnREZTYXVqOXptaUlmTll0NTU0M0l2TkcvMXo3?=
 =?utf-8?B?cE9HVTNMMDNSTHFRVTRiaE9YWHlUdFFid1ByQ3pkT0haVjJ5QjVCOG9CQ0xw?=
 =?utf-8?B?ekg5akhDQ3NUM2hYVzFDelhwYVBBcGt3R3U1QVZZYVZ3TTNWRHNMNEpVdXVC?=
 =?utf-8?B?UjNnRjY4N0tjM2ZtdGU0eGF2RmtHOXRrMjZsRXRoZVlVWGp6VTJtSFpERzND?=
 =?utf-8?B?UEE1VWRRQ1JtZTlDWm9yZktQSHJoTHlENTlsd2RaVnRJSUNac3V0NHNaRkVw?=
 =?utf-8?B?dVJNR2RyT3lxd0I4a21MM2ZpMFUrMGQ1WjErK3B4anl0OCt4NGc5YzQxSjRR?=
 =?utf-8?B?c3EzOFFXMVZuUGI5V2hHMzN4WGxKZC9jNVdHTXpURmtuekxJYzlEaUVUN2xo?=
 =?utf-8?B?R3ZUbU1JUmxQeXoxRGp3VDhtR1o3K1NEVXFkSng1d25uVTVqRDRHVjVFSjNU?=
 =?utf-8?B?UFd3SU1vcWVTRjE1TjZyZlB1aVF3bU54Mk5GSmpGWklOcmg1Rlh5WEpkVEZX?=
 =?utf-8?B?VE5rSDhmWThJYTlibW9mR3VIcVo4RU5FcUF1bGlkT1J5TU5hTDZkdGhjelJt?=
 =?utf-8?B?WHlGWUN1bFVOUnpLck9CZ2xNZjJSUllzY1Rid2hNYzV0R3FkMDUzeWNzbi9R?=
 =?utf-8?B?UytIZEgvVjNBUkJObndXcVdpUmZmd3p4TlBsWjQ1bjBBQmFGVlc1ODUxaG5l?=
 =?utf-8?B?Y3hvbGdya3FDSTdHVGE4a0VybkpXY1YzVTBQZ1B5aTJtR0RycVlMVTlIZ29C?=
 =?utf-8?Q?M78peWrBCWuHlZH1ilQ3QXttuOpYPDe3Xo3fydrrhC0X?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c7b8ec7b-716f-419c-599c-08db9e393654
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2023 09:14:36.7869
 (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: dlU23JzQC8sE0DWoxo0X3yotTjIgqeaYOxOlSSRgcycCFjfEdst0O8yRtNVk2KC5
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB8167
X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 16.08.23 um 10:34 schrieb Qi Zheng:
> Currently, the synchronize_shrinkers() is only used by TTM pool. It only
> requires that no shrinkers run in parallel.
>
> After we use RCU+refcount method to implement the lockless slab shrink,
> we can not use shrinker_rwsem or synchronize_rcu() to guarantee that all
> shrinker invocations have seen an update before freeing memory.
>
> So we introduce a new pool_shrink_rwsem to implement a private
> synchronize_shrinkers(), so as to achieve the same purpose.
>
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> ---
>   drivers/gpu/drm/ttm/ttm_pool.c | 15 +++++++++++++++
>   include/linux/shrinker.h       |  1 -
>   mm/shrinker.c                  | 15 ---------------
>   3 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index cddb9151d20f..713b1c0a70e1 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -74,6 +74,7 @@ static struct ttm_pool_type global_dma32_uncached[MAX_ORDER + 1];
>   static spinlock_t shrinker_lock;
>   static struct list_head shrinker_list;
>   static struct shrinker mm_shrinker;
> +static DECLARE_RWSEM(pool_shrink_rwsem);
>   
>   /* Allocate pages of size 1 << order with the given gfp_flags */
>   static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> @@ -317,6 +318,7 @@ static unsigned int ttm_pool_shrink(void)
>   	unsigned int num_pages;
>   	struct page *p;
>   
> +	down_read(&pool_shrink_rwsem);
>   	spin_lock(&shrinker_lock);
>   	pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
>   	list_move_tail(&pt->shrinker_list, &shrinker_list);
> @@ -329,6 +331,7 @@ static unsigned int ttm_pool_shrink(void)
>   	} else {
>   		num_pages = 0;
>   	}
> +	up_read(&pool_shrink_rwsem);
>   
>   	return num_pages;
>   }
> @@ -572,6 +575,18 @@ void ttm_pool_init(struct ttm_pool *pool, struct device *dev,
>   }
>   EXPORT_SYMBOL(ttm_pool_init);
>   
> +/**
> + * synchronize_shrinkers - Wait for all running shrinkers to complete.
> + *
> + * This is useful to guarantee that all shrinker invocations have seen an
> + * update, before freeing memory, similar to rcu.
> + */
> +static void synchronize_shrinkers(void)

Please rename that function to ttm_pool_synchronize_shrinkers().

With that done feel free to add Reviewed-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

> +{
> +	down_write(&pool_shrink_rwsem);
> +	up_write(&pool_shrink_rwsem);
> +}
> +
>   /**
>    * ttm_pool_fini - Cleanup a pool
>    *
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 8dc15aa37410..6b5843c3b827 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -103,7 +103,6 @@ extern int __printf(2, 3) register_shrinker(struct shrinker *shrinker,
>   					    const char *fmt, ...);
>   extern void unregister_shrinker(struct shrinker *shrinker);
>   extern void free_prealloced_shrinker(struct shrinker *shrinker);
> -extern void synchronize_shrinkers(void);
>   
>   #ifdef CONFIG_SHRINKER_DEBUG
>   extern int __printf(2, 3) shrinker_debugfs_rename(struct shrinker *shrinker,
> diff --git a/mm/shrinker.c b/mm/shrinker.c
> index 043c87ccfab4..a16cd448b924 100644
> --- a/mm/shrinker.c
> +++ b/mm/shrinker.c
> @@ -692,18 +692,3 @@ void unregister_shrinker(struct shrinker *shrinker)
>   	shrinker->nr_deferred = NULL;
>   }
>   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.
> - */
> -void synchronize_shrinkers(void)
> -{
> -	down_write(&shrinker_rwsem);
> -	up_write(&shrinker_rwsem);
> -}
> -EXPORT_SYMBOL(synchronize_shrinkers);