From: Michal Hocko <mhocko@kernel.org> To: Johannes Weiner <hannes@cmpxchg.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>, Minchan Kim <minchan@kernel.org>, Huang Ying <ying.huang@intel.com>, Mel Gorman <mgorman@techsingularity.net>, Vladimir Davydov <vdavydov.dev@gmail.com>, Andrew Morton <akpm@linux-foundation.org>, Shakeel Butt <shakeelb@google.com>, Greg Thelen <gthelen@google.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mm,vmscan: Kill global shrinker lock. Date: Wed, 15 Nov 2017 15:11:13 +0100 [thread overview] Message-ID: <20171115141113.2nw4c4nejermhckb@dhcp22.suse.cz> (raw) In-Reply-To: <20171115140020.GA6771@cmpxchg.org> On Wed 15-11-17 09:00:20, Johannes Weiner wrote: > On Wed, Nov 15, 2017 at 10:02:51AM +0100, Michal Hocko wrote: > > On Tue 14-11-17 06:37:42, Tetsuo Handa wrote: > > > This patch uses polling loop with short sleep for unregister_shrinker() > > > rather than wait_on_atomic_t(), for we can save reader's cost (plain > > > atomic_dec() compared to atomic_dec_and_test()), we can expect that > > > do_shrink_slab() of unregistering shrinker likely returns shortly, and > > > we can avoid khungtaskd warnings when do_shrink_slab() of unregistering > > > shrinker unexpectedly took so long. > > > > I would use wait_event_interruptible in the remove path rather than the > > short sleep loop which is just too ugly. The shrinker walk would then > > just wake_up the sleeper when the ref. count drops to 0. Two > > synchronize_rcu is quite ugly as well, but I was not able to simplify > > them. I will keep thinking. It just sucks how we cannot follow the > > standard rcu list with dynamically allocated structure pattern here. > > It's because the refcount is dropped too early. The refcount protects > the object during shrink, but not for the list_next(), and so you need > an additional grace period just for that part. Exactly > I think you could drop the reference count in the next iteration. This > way the list_next() works without requiring a second RCU grace period. That would work. I was playing with an idea of prefetching the next elemnt before dropping the reference but that would require a lock for the remove operation. Ugly... > ref count protects the object and its list pointers; RCU protects what > the list pointers point to before we acquire the reference: > > rcu_read_lock(); > list_for_each_entry_rcu(pos, list) { > if (!atomic_inc_not_zero(&pos->ref)) > continue; > rcu_read_unlock(); > > if (prev) > atomic_dec(&prev->ref); > prev = pos; > > shrink(); > > rcu_read_lock(); > } > rcu_read_unlock(); > if (prev) > atomic_dec(&prev->ref); > > In any case, Minchan's lock breaking seems way preferable over that > level of headscratching complexity for an unusual case like Shakeel's. agreed! I would go the more complex way only if it turns out that early break out causes some real problems. -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org> To: Johannes Weiner <hannes@cmpxchg.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>, Minchan Kim <minchan@kernel.org>, Huang Ying <ying.huang@intel.com>, Mel Gorman <mgorman@techsingularity.net>, Vladimir Davydov <vdavydov.dev@gmail.com>, Andrew Morton <akpm@linux-foundation.org>, Shakeel Butt <shakeelb@google.com>, Greg Thelen <gthelen@google.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mm,vmscan: Kill global shrinker lock. Date: Wed, 15 Nov 2017 15:11:13 +0100 [thread overview] Message-ID: <20171115141113.2nw4c4nejermhckb@dhcp22.suse.cz> (raw) In-Reply-To: <20171115140020.GA6771@cmpxchg.org> On Wed 15-11-17 09:00:20, Johannes Weiner wrote: > On Wed, Nov 15, 2017 at 10:02:51AM +0100, Michal Hocko wrote: > > On Tue 14-11-17 06:37:42, Tetsuo Handa wrote: > > > This patch uses polling loop with short sleep for unregister_shrinker() > > > rather than wait_on_atomic_t(), for we can save reader's cost (plain > > > atomic_dec() compared to atomic_dec_and_test()), we can expect that > > > do_shrink_slab() of unregistering shrinker likely returns shortly, and > > > we can avoid khungtaskd warnings when do_shrink_slab() of unregistering > > > shrinker unexpectedly took so long. > > > > I would use wait_event_interruptible in the remove path rather than the > > short sleep loop which is just too ugly. The shrinker walk would then > > just wake_up the sleeper when the ref. count drops to 0. Two > > synchronize_rcu is quite ugly as well, but I was not able to simplify > > them. I will keep thinking. It just sucks how we cannot follow the > > standard rcu list with dynamically allocated structure pattern here. > > It's because the refcount is dropped too early. The refcount protects > the object during shrink, but not for the list_next(), and so you need > an additional grace period just for that part. Exactly > I think you could drop the reference count in the next iteration. This > way the list_next() works without requiring a second RCU grace period. That would work. I was playing with an idea of prefetching the next elemnt before dropping the reference but that would require a lock for the remove operation. Ugly... > ref count protects the object and its list pointers; RCU protects what > the list pointers point to before we acquire the reference: > > rcu_read_lock(); > list_for_each_entry_rcu(pos, list) { > if (!atomic_inc_not_zero(&pos->ref)) > continue; > rcu_read_unlock(); > > if (prev) > atomic_dec(&prev->ref); > prev = pos; > > shrink(); > > rcu_read_lock(); > } > rcu_read_unlock(); > if (prev) > atomic_dec(&prev->ref); > > In any case, Minchan's lock breaking seems way preferable over that > level of headscratching complexity for an unusual case like Shakeel's. agreed! I would go the more complex way only if it turns out that early break out causes some real problems. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-11-15 14:11 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-13 21:37 [PATCH 1/2] mm,vmscan: Kill global shrinker lock Tetsuo Handa 2017-11-13 21:37 ` Tetsuo Handa 2017-11-13 21:37 ` [PATCH 2/2] mm,vmscan: Allow parallel registration/unregistration of shrinkers Tetsuo Handa 2017-11-13 21:37 ` Tetsuo Handa 2017-11-13 22:05 ` [PATCH 1/2] mm,vmscan: Kill global shrinker lock Shakeel Butt 2017-11-13 22:05 ` Shakeel Butt 2017-11-15 0:56 ` Minchan Kim 2017-11-15 0:56 ` Minchan Kim 2017-11-15 6:28 ` Shakeel Butt 2017-11-15 6:28 ` Shakeel Butt 2017-11-16 0:46 ` Minchan Kim 2017-11-16 0:46 ` Minchan Kim 2017-11-16 1:41 ` Shakeel Butt 2017-11-16 1:41 ` Shakeel Butt 2017-11-16 4:50 ` Minchan Kim 2017-11-16 4:50 ` Minchan Kim 2017-11-15 8:56 ` Michal Hocko 2017-11-15 8:56 ` Michal Hocko 2017-11-15 9:18 ` Michal Hocko 2017-11-15 9:18 ` Michal Hocko 2017-11-16 17:44 ` Johannes Weiner 2017-11-16 17:44 ` Johannes Weiner 2017-11-23 23:46 ` Minchan Kim 2017-11-23 23:46 ` Minchan Kim 2017-11-15 9:02 ` Michal Hocko 2017-11-15 9:02 ` Michal Hocko 2017-11-15 10:58 ` Tetsuo Handa 2017-11-15 10:58 ` Tetsuo Handa 2017-11-15 11:51 ` Michal Hocko 2017-11-15 11:51 ` Michal Hocko 2017-11-16 0:56 ` Minchan Kim 2017-11-16 0:56 ` Minchan Kim 2017-11-15 13:28 ` Johannes Weiner 2017-11-15 13:28 ` Johannes Weiner 2017-11-16 10:56 ` Tetsuo Handa 2017-11-16 10:56 ` Tetsuo Handa 2017-11-15 14:00 ` Johannes Weiner 2017-11-15 14:00 ` Johannes Weiner 2017-11-15 14:11 ` Michal Hocko [this message] 2017-11-15 14:11 ` Michal Hocko 2018-01-25 2:04 ` Tetsuo Handa 2018-01-25 2:04 ` Tetsuo Handa 2018-01-25 8:36 ` Michal Hocko 2018-01-25 8:36 ` Michal Hocko 2018-01-25 10:56 ` Tetsuo Handa 2018-01-25 10:56 ` Tetsuo Handa 2018-01-25 11:41 ` Michal Hocko 2018-01-25 11:41 ` Michal Hocko 2018-01-25 22:19 ` Eric Wheeler 2018-01-25 22:19 ` Eric Wheeler 2018-01-26 3:12 ` Tetsuo Handa 2018-01-26 3:12 ` Tetsuo Handa 2018-01-26 10:08 ` Michal Hocko 2018-01-26 10:08 ` Michal Hocko 2017-11-17 17:35 ` Christoph Hellwig 2017-11-17 17:35 ` Christoph Hellwig 2017-11-17 17:41 ` Shakeel Butt 2017-11-17 17:41 ` Shakeel Butt 2017-11-17 17:53 ` Shakeel Butt 2017-11-17 17:53 ` Shakeel Butt 2017-11-17 18:36 ` Christoph Hellwig 2017-11-17 18:36 ` Christoph Hellwig 2017-11-20 9:25 ` Michal Hocko 2017-11-20 9:25 ` Michal Hocko 2017-11-20 9:33 ` Christoph Hellwig 2017-11-20 9:33 ` Christoph Hellwig 2017-11-20 9:42 ` Michal Hocko 2017-11-20 9:42 ` Michal Hocko 2017-11-20 10:41 ` Christoph Hellwig 2017-11-20 10:41 ` Christoph Hellwig 2017-11-20 10:56 ` Tetsuo Handa 2017-11-20 10:56 ` Tetsuo Handa 2017-11-20 18:28 ` Paul E. McKenney 2017-11-20 18:28 ` Paul E. McKenney
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=20171115141113.2nw4c4nejermhckb@dhcp22.suse.cz \ --to=mhocko@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=gthelen@google.com \ --cc=hannes@cmpxchg.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@techsingularity.net \ --cc=minchan@kernel.org \ --cc=penguin-kernel@I-love.SAKURA.ne.jp \ --cc=shakeelb@google.com \ --cc=vdavydov.dev@gmail.com \ --cc=ying.huang@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.