From: Kirill Tkhai <tkhai@ya.ru>
To: Qi Zheng <qi.zheng@linux.dev>, paulmck@kernel.org
Cc: RCU <rcu@vger.kernel.org>, "Yujie Liu" <yujie.liu@intel.com>,
oe-lkp@lists.linux.dev, lkp@intel.com,
linux-kernel@vger.kernel.org,
"Andrew Morton" <akpm@linux-foundation.org>,
"Vlastimil Babka" <vbabka@suse.cz>,
"Roman Gushchin" <roman.gushchin@linux.dev>,
"Christian König" <christian.koenig@amd.com>,
"David Hildenbrand" <david@redhat.com>,
"Davidlohr Bueso" <dave@stgolabs.net>,
"Johannes Weiner" <hannes@cmpxchg.org>,
"Michal Hocko" <mhocko@kernel.org>,
"Muchun Song" <muchun.song@linux.dev>,
"Shakeel Butt" <shakeelb@google.com>,
"Yang Shi" <shy828301@gmail.com>,
linux-mm@kvack.org, ying.huang@intel.com, feng.tang@intel.com,
fengwei.yin@intel.com
Subject: Re: [linus:master] [mm] f95bdb700b: stress-ng.ramfs.ops_per_sec -88.8% regression
Date: Thu, 1 Jun 2023 03:57:10 +0300 [thread overview]
Message-ID: <bab60fe4-964c-43a6-ecce-4cbd4981d875@ya.ru> (raw)
In-Reply-To: <26da75a6-115f-a17a-73bb-381a6b4a3a75@linux.dev>
Hi,
On 30.05.2023 06:07, Qi Zheng wrote:
> Hi,
>
> On 2023/5/29 20:51, Paul E. McKenney wrote:
>> On Mon, May 29, 2023 at 10:39:21AM +0800, Qi Zheng wrote:
>
> [...]
>
>>>
>>> Thanks for such a detailed explanation.
>>>
>>> Now I think we can continue to try to complete the idea[1] from
>>> Kirill Tkhai. The patch moves heavy synchronize_srcu() to delayed
>>> work, so it doesn't affect on user-visible unregistration speed.
>>>
>>> [1]. https://lore.kernel.org/lkml/153365636747.19074.12610817307548583381.stgit@localhost.localdomain/
>>
>> A blast from the past! ;-)
>>
>> But yes, moving the long-latency synchronize_srcu() off the user-visible
>> critical code path can be even better.
>
> Yeah, I applied these patches ([PATCH RFC 04/10]~[PATCH RFC 10/10],
> with few conflicts), the ops/s does get back to the previous levels.
>
> I'll continue updating this patchset after doing more testing.
You may also fix the issue using the below generic solution.
In addition to this we need patch, which calls unregister_shrinker_delayed_initiate()
instead of unregister_shrinker() in deactivate_locked_super(), and calls
unregister_shrinker_delayed_finalize() from destroy_super_work(). Compilation tested only.
---
include/linux/shrinker.h | 2 ++
mm/vmscan.c | 38 ++++++++++++++++++++++++++++++--------
2 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 224293b2dd06..4ba2986716d3 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -4,6 +4,7 @@
#include <linux/atomic.h>
#include <linux/types.h>
+#include <linux/rwsem.h>
/*
* This struct is used to pass information from page reclaim to the shrinkers.
@@ -83,6 +84,7 @@ struct shrinker {
#endif
/* objs pending delete, per node */
atomic_long_t *nr_deferred;
+ struct rw_semaphore rwsem;
};
#define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index eeca83e28c9b..19fc129771ce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -706,6 +706,7 @@ static int __prealloc_shrinker(struct shrinker *shrinker)
if (!shrinker->nr_deferred)
return -ENOMEM;
+ init_rwsem(&shrinker->rwsem);
return 0;
}
@@ -757,7 +758,9 @@ void register_shrinker_prepared(struct shrinker *shrinker)
{
mutex_lock(&shrinker_mutex);
list_add_tail_rcu(&shrinker->list, &shrinker_list);
+ down_write(&shrinker->rwsem);
shrinker->flags |= SHRINKER_REGISTERED;
+ up_write(&shrinker->rwsem);
shrinker_debugfs_add(shrinker);
mutex_unlock(&shrinker_mutex);
}
@@ -802,7 +805,7 @@ EXPORT_SYMBOL(register_shrinker);
/*
* Remove one
*/
-void unregister_shrinker(struct shrinker *shrinker)
+void unregister_shrinker_delayed_initiate(struct shrinker *shrinker)
{
struct dentry *debugfs_entry;
int debugfs_id;
@@ -812,20 +815,33 @@ void unregister_shrinker(struct shrinker *shrinker)
mutex_lock(&shrinker_mutex);
list_del_rcu(&shrinker->list);
+ down_write(&shrinker->rwsem);
shrinker->flags &= ~SHRINKER_REGISTERED;
+ up_write(&shrinker->rwsem);
if (shrinker->flags & SHRINKER_MEMCG_AWARE)
unregister_memcg_shrinker(shrinker);
debugfs_entry = shrinker_debugfs_detach(shrinker, &debugfs_id);
mutex_unlock(&shrinker_mutex);
+ shrinker_debugfs_remove(debugfs_entry, debugfs_id); // This is moved in your patch
+}
+EXPORT_SYMBOL(unregister_shrinker_delayed_initiate);
+
+void unregister_shrinker_delayed_finalize(struct shrinker *shrinker)
+{
atomic_inc(&shrinker_srcu_generation);
synchronize_srcu(&shrinker_srcu);
- shrinker_debugfs_remove(debugfs_entry, debugfs_id);
-
kfree(shrinker->nr_deferred);
shrinker->nr_deferred = NULL;
}
+EXPORT_SYMBOL(unregister_shrinker_delayed_finalize);
+
+void unregister_shrinker(struct shrinker *shrinker)
+{
+ unregister_shrinker_delayed_initiate(shrinker);
+ unregister_shrinker_delayed_finalize(shrinker);
+}
EXPORT_SYMBOL(unregister_shrinker);
/**
@@ -856,9 +872,14 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
: SHRINK_BATCH;
long scanned = 0, next_deferred;
+ down_read(&shrinker->rwsem);
+ if (!(shrinker->flags & SHRINKER_REGISTERED))
+ goto unlock;
freeable = shrinker->count_objects(shrinker, shrinkctl);
- if (freeable == 0 || freeable == SHRINK_EMPTY)
- return freeable;
+ if (freeable == 0 || freeable == SHRINK_EMPTY) {
+ freed = freeable;
+ goto unlock;
+ }
/*
* copy the current shrinker scan count into a local variable
@@ -935,6 +956,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
* manner that handles concurrent updates.
*/
new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl);
+unlock:
+ up_read(&shrinker->rwsem);
trace_mm_shrink_slab_end(shrinker, shrinkctl->nid, freed, nr, new_nr, total_scan);
return freed;
@@ -968,9 +991,8 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
struct shrinker *shrinker;
shrinker = idr_find(&shrinker_idr, i);
- if (unlikely(!shrinker || !(shrinker->flags & SHRINKER_REGISTERED))) {
- if (!shrinker)
- clear_bit(i, info->map);
+ if (unlikely(!shrinker)) {
+ clear_bit(i, info->map);
continue;
}
next prev parent reply other threads:[~2023-06-01 1:07 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <202305230837.db2c233f-yujie.liu@intel.com>
[not found] ` <eba38fce-2454-d7a4-10ef-240b4686f23d@linux.dev>
[not found] ` <ZG29ULGNJdErnatI@yujie-X299>
[not found] ` <896bbb09-d400-ec73-ba3a-b64c6e9bbe46@linux.dev>
[not found] ` <e5fb8b34-c1ad-92e0-e7e5-f7ed1605dbc6@linux.dev>
2023-05-24 11:22 ` [linus:master] [mm] f95bdb700b: stress-ng.ramfs.ops_per_sec -88.8% regression Qi Zheng
2023-05-24 11:56 ` Qi Zheng
2023-05-25 4:03 ` Qi Zheng
2023-05-27 11:14 ` Paul E. McKenney
2023-05-29 2:39 ` Qi Zheng
2023-05-29 12:51 ` Paul E. McKenney
2023-05-30 3:07 ` Qi Zheng
2023-06-01 0:57 ` Kirill Tkhai [this message]
2023-06-01 8:34 ` Qi Zheng
[not found] ` <932751685611907@mail.yandex.ru>
2023-06-01 10:44 ` Qi Zheng
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=bab60fe4-964c-43a6-ecce-4cbd4981d875@ya.ru \
--to=tkhai@ya.ru \
--cc=akpm@linux-foundation.org \
--cc=christian.koenig@amd.com \
--cc=dave@stgolabs.net \
--cc=david@redhat.com \
--cc=feng.tang@intel.com \
--cc=fengwei.yin@intel.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lkp@intel.com \
--cc=mhocko@kernel.org \
--cc=muchun.song@linux.dev \
--cc=oe-lkp@lists.linux.dev \
--cc=paulmck@kernel.org \
--cc=qi.zheng@linux.dev \
--cc=rcu@vger.kernel.org \
--cc=roman.gushchin@linux.dev \
--cc=shakeelb@google.com \
--cc=shy828301@gmail.com \
--cc=vbabka@suse.cz \
--cc=ying.huang@intel.com \
--cc=yujie.liu@intel.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;
as well as URLs for NNTP newsgroup(s).