From: Dave Chinner <david@fromorbit.com>
To: Kirill Tkhai <tkhai@ya.ru>
Cc: akpm@linux-foundation.org, roman.gushchin@linux.dev,
vbabka@suse.cz, viro@zeniv.linux.org.uk, brauner@kernel.org,
djwong@kernel.org, hughd@google.com, paulmck@kernel.org,
muchun.song@linux.dev, linux-mm@kvack.org,
linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,
linux-kernel@vger.kernel.org, zhengqi.arch@bytedance.com
Subject: Re: [PATCH v2 0/3] mm: Make unregistration of super_block shrinker more faster
Date: Tue, 6 Jun 2023 08:32:35 +1000 [thread overview]
Message-ID: <ZH5ig590WleaH1Ed@dread.disaster.area> (raw)
In-Reply-To: <168599103578.70911.9402374667983518835.stgit@pro.pro>
On Mon, Jun 05, 2023 at 10:02:46PM +0300, Kirill Tkhai wrote:
> This patch set introduces a new scheme of shrinker unregistration. It allows to split
> the unregistration in two parts: fast and slow. This allows to hide slow part from
> a user, so user-visible unregistration becomes fast.
>
> This fixes the -88.8% regression of stress-ng.ramfs.ops_per_sec noticed
> by kernel test robot:
>
> https://lore.kernel.org/lkml/202305230837.db2c233f-yujie.liu@intel.com/
>
> ---
>
> Kirill Tkhai (2):
> mm: Split unregister_shrinker() in fast and slow part
> fs: Use delayed shrinker unregistration
Did you test any filesystem other than ramfs?
Filesystems more complex than ramfs have internal shrinkers, and so
they will still be running the slow synchronize_srcu() - potentially
multiple times! - in every unmount. Both XFS and ext4 have 3
internal shrinker instances per mount, so they will still call
synchronize_srcu() at least 3 times per unmount after this change.
What about any other subsystem that runs a shrinker - do they have
context depedent shrinker instances that get frequently created and
destroyed? They'll need the same treatment.
Seriously, part of changing shrinker infrastructure is doing an
audit of all the shrinker instances to determine how the change will
impact those shrinkers, and if the same structural changes are
needed to those implementations.
I don't see any of this being done - this looks like a "slap a bandaid
over the visible symptom" patch set without any deeper investigation
of the scope of the issue having been gained.
Along with all shrinkers now running under a SRCU critical region
and requiring a machine wide synchronisation point for every
unregister_shrinker() call made, the ability to repeated abort
global shrinker passes via external SRCU expediting, and now an
intricate locking and state dance in do_shrink_slab() vs
unregister_shrinker, I can't say I'm particularly liking any of
this, regardles of the benefits it supposedly provides.
-Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2023-06-05 22:32 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-05 19:02 [PATCH v2 0/3] mm: Make unregistration of super_block shrinker more faster Kirill Tkhai
2023-06-05 19:03 ` [PATCH v2 1/3] mm: vmscan: move shrinker_debugfs_remove() before synchronize_srcu() Kirill Tkhai
2023-06-06 0:31 ` Roman Gushchin
2023-06-05 19:03 ` [PATCH v2 2/3] mm: Split unregister_shrinker() in fast and slow part Kirill Tkhai
2023-06-07 4:49 ` kernel test robot
2023-06-07 7:33 ` Yujie Liu
2023-06-05 19:03 ` [PATCH v2 3/3] fs: Use delayed shrinker unregistration Kirill Tkhai
2023-06-06 0:38 ` Roman Gushchin
2023-06-06 1:24 ` Dave Chinner
2023-06-06 2:56 ` Roman Gushchin
2023-06-06 6:51 ` Dave Chinner
2023-06-06 15:56 ` Roman Gushchin
2023-06-06 21:21 ` Kirill Tkhai
2023-06-06 22:30 ` Dave Chinner
2023-06-08 16:36 ` Theodore Ts'o
2023-06-08 23:17 ` Dave Chinner
2023-06-09 0:27 ` Andrew Morton
2023-06-09 2:50 ` Qi Zheng
2023-06-05 22:32 ` Dave Chinner [this message]
2023-06-06 21:06 ` [PATCH v2 0/3] mm: Make unregistration of super_block shrinker more faster Kirill Tkhai
2023-06-06 22:02 ` Dave Chinner
2023-06-07 2:51 ` Qi Zheng
2023-06-08 21:58 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZH5ig590WleaH1Ed@dread.disaster.area \
--to=david@fromorbit.com \
--cc=akpm@linux-foundation.org \
--cc=brauner@kernel.org \
--cc=djwong@kernel.org \
--cc=hughd@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-xfs@vger.kernel.org \
--cc=muchun.song@linux.dev \
--cc=paulmck@kernel.org \
--cc=roman.gushchin@linux.dev \
--cc=tkhai@ya.ru \
--cc=vbabka@suse.cz \
--cc=viro@zeniv.linux.org.uk \
--cc=zhengqi.arch@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox