From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 53139CD3432 for ; Tue, 19 Sep 2023 02:37:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DA5916B048C; Mon, 18 Sep 2023 22:37:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D55C06B048E; Mon, 18 Sep 2023 22:37:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C1D7B6B048F; Mon, 18 Sep 2023 22:37:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id B1B2E6B048C for ; Mon, 18 Sep 2023 22:37:30 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 8262CB38DB for ; Tue, 19 Sep 2023 02:37:30 +0000 (UTC) X-FDA: 81251785860.24.9CD3126 Received: from out-212.mta1.migadu.com (out-212.mta1.migadu.com [95.215.58.212]) by imf05.hostedemail.com (Postfix) with ESMTP id 9F39010000B for ; Tue, 19 Sep 2023 02:37:27 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=SP1SG92C; spf=pass (imf05.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.212 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695091048; a=rsa-sha256; cv=none; b=unBw2MQPwstDew1t71YJ3cukf5Ot1swYKbw8MHozdOknnnC9xVrNUBDvxt4Exmc9tCV9Zq y1TDmr+WKZHmEg7Wp2FA//ZbIV927qr7Nd/yHasHZZEsixzRreoZruGMuD+3XH9+i3rMQH P4Kvdp1YpWjliZWTP5Q6jZDlPHUNujA= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=SP1SG92C; spf=pass (imf05.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.212 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695091048; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5OJNjyBnLREqB4t2y6BnDouZQfoMamnWSnx+o56AOxg=; b=lhDKRq4xEurRttDv1KU0eUmWs0U/InRS8AH5wtPxgXlQObPFFt9UvX5RwlYH+Jd5MaHCAz AjdTdfprZJhKbAWfcM8Z7HYHXljpgRE16W++tZv52veLySYY9DXsEq30dqOAtqLi3LK73X 8Ksn673XsB0pdi7jPGy169ssO5Y6bzE= Content-Type: text/plain; charset=us-ascii DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1695091045; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5OJNjyBnLREqB4t2y6BnDouZQfoMamnWSnx+o56AOxg=; b=SP1SG92CdlrZckX/eX1RfKyREfr7y1srpE2MIRRG4j65DzPvjkZETTLcQ6PbIrgwcXcKbc 4o13mEguIAmfirNWUzGJFkZs7KXv1gE3Jeh1KkMgQaLn04N/Rric50c3yRN9JQLaErM35U q/aAqFcCfvguVSwdbBP6QjcjS8YL4Yg= Mime-Version: 1.0 Subject: Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: <529bce6f-e23b-4b84-a7e6-cce3c12645aa@bytedance.com> Date: Tue, 19 Sep 2023 10:36:42 +0800 Cc: Andrew Morton , LKML , Linux-MM , linux-fsdevel@vger.kernel.org, david@fromorbit.com, tkhai@ya.ru, Vlastimil Babka , Roman Gushchin , djwong@kernel.org, Christian Brauner , "Paul E. McKenney" , tytso@mit.edu, steven.price@arm.com, cel@kernel.org, Sergey Senozhatsky , yujie.liu@intel.com, Greg KH Content-Transfer-Encoding: quoted-printable Message-Id: <2C92CE73-2766-4E1A-AB77-F0DD91043457@linux.dev> References: <20230911094444.68966-1-zhengqi.arch@bytedance.com> <20230911094444.68966-2-zhengqi.arch@bytedance.com> <4aff0e17-b40f-406d-65fd-72a2bacfcc1a@linux.dev> <529bce6f-e23b-4b84-a7e6-cce3c12645aa@bytedance.com> To: Qi Zheng X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 9F39010000B X-Stat-Signature: iwibz6cf5gu83jtztu4xjxfo1oy3jb1m X-Rspam-User: X-HE-Tag: 1695091047-250311 X-HE-Meta: U2FsdGVkX19mt4w8+UO79O+Asx22gfRK1S4SKM+rYIwOFrcP0GPZwE7Ph298HTLHPyQquTJYG9nXH5S3DrJRj27pXEnz9YcqcVYsfZf8cUl6kECyI/oWruFFTrsVj6OdFt8Anf+OvxGX/qpQkZQLhYFdO24tIYPKEKiD7l+ORd440TPwBYHsoAzoQQ4XxtBNnDVDOf+KWEFiuU7GG3VPUfMVB8y2a/mm3dI+lyTRMoncymDb6f/Ktpsv5dm2RHJgaCcntJv1+4jdfn1WRaAj61H9jbGYfXrtCtZwXfpNECA7IcjhTBdOxVRwZt7xJqtI7gRpnA3Ue6KMUm1fLc4M1XJcTy51Xn3qycm/KduZEWm911hUCVc2Nd9hRVQX2lbSJAMjOqb4pmHNrW2apuRaJspqlLFYcaxhdmTL9ltJ12zvLkqW6itHqCBTcQuil741VLpHXTte90osvAKlztP99UlExoq2IvaMB0DZaiTe9Rm+VZYJH7VuQAhIsYYd2zx8gSA93Sp0xf9Ru8+NIEACt4HQGSrJqyJRccY81qc/yaYnimr/a8/f5je+xBajM5HEh3nz+mxzYfRAMNKFW1y1nONylJd7Gyw2Ob9b/IRWzqQSWoRHRs3WzWg/0XXWbkQ1fEnsB1HaX30O+2M3wJIfHamySiyjvfTxjxuy6w/lktLjpoZawOz316CRNoorAN9sK2qY4WShOFlPGWrVriNEk+AIpx1NjbKWCXafgvOBquUX6aHx5uATHQm8MffEDHOcdSBI/22foUqGXBkMN63syOEcmhP4wdYcg/nFZsfCSQXIw2ZWvtUWbj+W6/jFCS+WQSiqbpyXK8aLNs021WJQcHOTZKcCjEpC33dHtE130rnZyoTJrYZ9sEi67D2r+Piwn9IXQe00wHHYGcVYE0EGGATe2IsA/NtY0Nin8qwwGBx6Mk5COeVSvdcHR7621QWthu8I2s06aElToFPSfDW IR/3tUwM rruLc6fWe8DrcBWvWFPF0130s6Cgk2ch3WentHBaKQWiOdkFdz8BajMZmbGZgxzw8zH7hF6ymfgXZ53Ea2CAoaJofdDnN9pppR+r5E1N16/wg743AruIBaK8MZEOeTIqQYLHpoJE/3It0JH1AM7ULzuJhZAmZWdA/NOxxKHT1d5LMO5jWuuUQilx5dw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: > On Sep 18, 2023, at 20:06, Qi Zheng = wrote: >=20 >=20 >=20 > On 2023/9/18 17:03, Muchun Song wrote: >=20 > ... >=20 >>> @@ -95,6 +97,11 @@ struct shrinker { >>> * non-MEMCG_AWARE shrinker should not have this flag set. >>> */ >>> #define SHRINKER_NONSLAB (1 << 3) >>> +#define SHRINKER_ALLOCATED (1 << 4) >> It is better to add a comment here to tell users >> it is only used by internals of shrinker. The users >> are not supposed to pass this flag to shrink APIs. >=20 > OK, will annotate SHRINKER_REGISTERED and SHRINKER_ALLOCATED > as flags used internally. >=20 >>> + >>> +struct shrinker *shrinker_alloc(unsigned int flags, const char = *fmt, ...); >>> +void shrinker_register(struct shrinker *shrinker); >>> +void shrinker_free(struct shrinker *shrinker); >>> extern int __printf(2, 3) prealloc_shrinker(struct shrinker = *shrinker, >>> const char *fmt, ...); >>> diff --git a/mm/internal.h b/mm/internal.h >>> index 0471d6326d01..5587cae20ebf 100644 >>> --- a/mm/internal.h >>> +++ b/mm/internal.h >>> @@ -1161,6 +1161,9 @@ unsigned long shrink_slab(gfp_t gfp_mask, int = nid, struct mem_cgroup *memcg, >>> #ifdef CONFIG_SHRINKER_DEBUG >>> extern int shrinker_debugfs_add(struct shrinker *shrinker); >>> +extern int shrinker_debugfs_name_alloc(struct shrinker *shrinker, >>> + const char *fmt, va_list ap); >>> +extern void shrinker_debugfs_name_free(struct shrinker *shrinker); >>> extern struct dentry *shrinker_debugfs_detach(struct shrinker = *shrinker, >>> int *debugfs_id); >>> extern void shrinker_debugfs_remove(struct dentry *debugfs_entry, >>> @@ -1170,6 +1173,14 @@ static inline int shrinker_debugfs_add(struct = shrinker *shrinker) >>> { >>> return 0; >>> } >>> +static inline int shrinker_debugfs_name_alloc(struct shrinker = *shrinker, >>> + const char *fmt, va_list ap) >>> +{ >>> + return 0; >>> +} >>> +static inline void shrinker_debugfs_name_free(struct shrinker = *shrinker) >>> +{ >>> +} >>> static inline struct dentry *shrinker_debugfs_detach(struct = shrinker *shrinker, >>> int *debugfs_id) >>> { >>> diff --git a/mm/shrinker.c b/mm/shrinker.c >>> index a16cd448b924..201211a67827 100644 >>> --- a/mm/shrinker.c >>> +++ b/mm/shrinker.c >>> @@ -550,6 +550,108 @@ unsigned long shrink_slab(gfp_t gfp_mask, int = nid, struct mem_cgroup *memcg, >>> return freed; >>> } >>> +struct shrinker *shrinker_alloc(unsigned int flags, const char = *fmt, ...) >>> +{ >>> + struct shrinker *shrinker; >>> + unsigned int size; >>> + va_list ap; >>> + int err; >>> + >>> + shrinker =3D kzalloc(sizeof(struct shrinker), GFP_KERNEL); >>> + if (!shrinker) >>> + return NULL; >>> + >>> + va_start(ap, fmt); >>> + err =3D shrinker_debugfs_name_alloc(shrinker, fmt, ap); >>> + va_end(ap); >>> + if (err) >>> + goto err_name; >>> + >>> + shrinker->flags =3D flags | SHRINKER_ALLOCATED; >>> + shrinker->seeks =3D DEFAULT_SEEKS; >>> + >>> + if (flags & SHRINKER_MEMCG_AWARE) { >>> + err =3D prealloc_memcg_shrinker(shrinker); >>> + if (err =3D=3D -ENOSYS) >>> + shrinker->flags &=3D ~SHRINKER_MEMCG_AWARE; >>> + else if (err =3D=3D 0) >>> + goto done; >>> + else >>> + goto err_flags; >> Actually, the code here is a little confusing me when I fist look >> at it. I think there could be some improvements here. Something >> like: >> if (flags & SHRINKER_MEMCG_AWARE) { >> err =3D prealloc_memcg_shrinker(shrinker); >> if (err =3D=3D -ENOSYS) { >> /* Memcg is not supported and fallback to = non-memcg-aware shrinker. */ >> shrinker->flags &=3D ~SHRINKER_MEMCG_AWARE; >> goto non-memcg; >> } >> if (err) >> goto err_flags; >> return shrinker; >> } >> non-memcg: >> [...] >> return shrinker; >> In this case, the code becomes more clear (at least for me). We have = split the >> code into two part, one is handling memcg-aware case, another is = non-memcg-aware >> case. Any side will have a explicit "return" keyword to return once = succeeds. >> It is a little implicit that the previous one uses "goto done". >> And the tag of "non-memcg" is also a good annotation to tell us the = following >> code handles non-memcg-aware case. >=20 > Make sense, will do. >=20 >>> + } >>> + >>> + /* >>> + * The nr_deferred is available on per memcg level for memcg = aware >>> + * shrinkers, so only allocate nr_deferred in the following = cases: >>> + * - non memcg aware shrinkers >>> + * - !CONFIG_MEMCG >>> + * - memcg is disabled by kernel command line >>> + */ >>> + size =3D sizeof(*shrinker->nr_deferred); >>> + if (flags & SHRINKER_NUMA_AWARE) >>> + size *=3D nr_node_ids; >>> + >>> + shrinker->nr_deferred =3D kzalloc(size, GFP_KERNEL); >>> + if (!shrinker->nr_deferred) >>> + goto err_flags; >>> + >>> +done: >>> + return shrinker; >>> + >>> +err_flags: >>> + shrinker_debugfs_name_free(shrinker); >>> +err_name: >>> + kfree(shrinker); >>> + return NULL; >>> +} >>> +EXPORT_SYMBOL_GPL(shrinker_alloc); >>> + >>> +void shrinker_register(struct shrinker *shrinker) >>> +{ >>> + if (unlikely(!(shrinker->flags & SHRINKER_ALLOCATED))) { >>> + pr_warn("Must use shrinker_alloc() to dynamically allocate = the shrinker"); >>> + return; >>> + } >>> + >>> + down_write(&shrinker_rwsem); >>> + list_add_tail(&shrinker->list, &shrinker_list); >>> + shrinker->flags |=3D SHRINKER_REGISTERED; >>> + shrinker_debugfs_add(shrinker); >>> + up_write(&shrinker_rwsem); >>> +} >>> +EXPORT_SYMBOL_GPL(shrinker_register); >>> + >>> +void shrinker_free(struct shrinker *shrinker) >>> +{ >>> + struct dentry *debugfs_entry =3D NULL; >>> + int debugfs_id; >>> + >>> + if (!shrinker) >>> + return; >>> + >>> + down_write(&shrinker_rwsem); >>> + if (shrinker->flags & SHRINKER_REGISTERED) { >>> + list_del(&shrinker->list); >>> + debugfs_entry =3D shrinker_debugfs_detach(shrinker, = &debugfs_id); >>> + shrinker->flags &=3D ~SHRINKER_REGISTERED; >>> + } else { >>> + shrinker_debugfs_name_free(shrinker); >> We could remove shrinker_debugfs_name_free() calling from >> shrinker_debugfs_detach(), then we could call >> shrinker_debugfs_name_free() anyway, otherwise, it it a little >> weird for me. And the srinker name is allocated from = shrinker_alloc(), >> so I think it it reasonable for shrinker_free() to free the >> shrinker name. >=20 > OK, will do. >=20 >> Thanks. >>> + } >>> + >>> + if (shrinker->flags & SHRINKER_MEMCG_AWARE) >>> + unregister_memcg_shrinker(shrinker); >>> + up_write(&shrinker_rwsem); >>> + >>> + if (debugfs_entry) >>> + shrinker_debugfs_remove(debugfs_entry, debugfs_id); >>> + >>> + kfree(shrinker->nr_deferred); >>> + shrinker->nr_deferred =3D NULL; >>> + >>> + kfree(shrinker); >>> +} >>> +EXPORT_SYMBOL_GPL(shrinker_free); >>> + >>> /* >>> * Add a shrinker callback to be called from the vm. >>> */ >>> diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c >>> index e4ce509f619e..38452f539f40 100644 >>> --- a/mm/shrinker_debug.c >>> +++ b/mm/shrinker_debug.c >>> @@ -193,6 +193,20 @@ int shrinker_debugfs_add(struct shrinker = *shrinker) >>> return 0; >>> } >>> +int shrinker_debugfs_name_alloc(struct shrinker *shrinker, const = char *fmt, >>> + va_list ap) >>> +{ >>> + shrinker->name =3D kvasprintf_const(GFP_KERNEL, fmt, ap); >>> + >>> + return shrinker->name ? 0 : -ENOMEM; >>> +} >>> + >>> +void shrinker_debugfs_name_free(struct shrinker *shrinker) >>> +{ >>> + kfree_const(shrinker->name); >>> + shrinker->name =3D NULL; >>> +} >> It it better to move both helpers to internal.h and mark them as = inline >> since both are very simple enough. >=20 > OK, will do. >=20 > Hi Andrew, below is the cleanup patch, which has a small conflict > with [PATCH v6 41/45]: >=20 > =46rom 5bc2b77484f5cd4616e510158f91c8877bd033ad Mon Sep 17 00:00:00 = 2001 > From: Qi Zheng > Date: Mon, 18 Sep 2023 10:41:15 +0000 > Subject: [PATCH] mm: shrinker: some cleanup >=20 > Signed-off-by: Qi Zheng Reviewed-by: Muchun Song Thanks.