From: Michal Hocko <mhocko@kernel.org> To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: hannes@cmpxchg.org, linux-mm@lists.ewheeler.net, minchan@kernel.org, ying.huang@intel.com, mgorman@techsingularity.net, vdavydov.dev@gmail.com, akpm@linux-foundation.org, shakeelb@google.com, gthelen@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mm,vmscan: Kill global shrinker lock. Date: Thu, 25 Jan 2018 12:41:04 +0100 [thread overview] Message-ID: <20180125114104.GP28465@dhcp22.suse.cz> (raw) In-Reply-To: <201801251956.FAH73425.VFJLFFtSHOOMQO@I-love.SAKURA.ne.jp> On Thu 25-01-18 19:56:59, Tetsuo Handa wrote: [...] > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 1afb2af..9858449 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -410,6 +410,9 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, > return freed; > } > > +struct lockdep_map __shrink_slab_map = > + STATIC_LOCKDEP_MAP_INIT("shrink_slab", &__shrink_slab_map); > + > /** > * shrink_slab - shrink slab caches > * @gfp_mask: allocation context > @@ -453,6 +456,8 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, > goto out; > } > > + lock_map_acquire(&__shrink_slab_map); > + > list_for_each_entry(shrinker, &shrinker_list, list) { > struct shrink_control sc = { > .gfp_mask = gfp_mask, > @@ -491,6 +496,8 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, > } > } > > + lock_map_release(&__shrink_slab_map); > + > up_read(&shrinker_rwsem); > out: > cond_resched(); I am not an expert on lockdep annotations. But is this something that makes sense? Don't you need lock_acquire_shared otherwise it will simply consider this a lockup if we succeed on trylock twice? But in any case the trylock already notes any dependency as the lockdep is involved when the lock is taken and we do not take any action otherwise. So what is the point? I am not familiar with XFS to read the lockdep trace properly. [...] > Normally shrinker_rwsem acts like a shared lock. But when > register_shrinker()/unregister_shrinker() called down_write(), > shrinker_rwsem suddenly starts acting like an exclusive lock. How come? We only do trylock and that means that we won't take it _after_ the write claims the lock. > What is unfortunate is that down_write() is called independent of > memory allocation requests. That is, shrinker_rwsem is essentially > a mutex (and hence the debug patch shown above). > > ---------------------------------------- > [<ffffffffac7538d3>] call_rwsem_down_write_failed+0x13/0x20 > [<ffffffffac1cb985>] register_shrinker+0x45/0xa0 > [<ffffffffac250f68>] sget_userns+0x468/0x4a0 > [<ffffffffac25106a>] mount_nodev+0x2a/0xa0 > [<ffffffffac251be4>] mount_fs+0x34/0x150 > [<ffffffffac2701f2>] vfs_kern_mount+0x62/0x120 > [<ffffffffac272a0e>] do_mount+0x1ee/0xc50 > [<ffffffffac27377e>] SyS_mount+0x7e/0xd0 > [<ffffffffac003831>] do_syscall_64+0x61/0x1a0 > [<ffffffffac80012c>] entry_SYSCALL64_slow_path+0x25/0x25 > [<ffffffffffffffff>] 0xffffffffffffffff > ---------------------------------------- > > Therefore, I think that when do_shrink_slab() for GFP_KERNEL is in progress > and down_read_trylock() starts failing because somebody else started waiting at > down_write(), do_shrink_slab() for GFP_NOFS or GFP_NOIO cannot be called. > Doesn't such race cause unexpected results? This is really hard to tell. I would expect that a skipped shrinkers would lead to an OOM killer sooner or later. As soon as the shrinker managed memory is the only one left for reclaim then we should OOM. And I do not see anything obvious that would prevent that. -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org> To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: hannes@cmpxchg.org, linux-mm@lists.ewheeler.net, minchan@kernel.org, ying.huang@intel.com, mgorman@techsingularity.net, vdavydov.dev@gmail.com, akpm@linux-foundation.org, shakeelb@google.com, gthelen@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mm,vmscan: Kill global shrinker lock. Date: Thu, 25 Jan 2018 12:41:04 +0100 [thread overview] Message-ID: <20180125114104.GP28465@dhcp22.suse.cz> (raw) In-Reply-To: <201801251956.FAH73425.VFJLFFtSHOOMQO@I-love.SAKURA.ne.jp> On Thu 25-01-18 19:56:59, Tetsuo Handa wrote: [...] > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 1afb2af..9858449 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -410,6 +410,9 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, > return freed; > } > > +struct lockdep_map __shrink_slab_map = > + STATIC_LOCKDEP_MAP_INIT("shrink_slab", &__shrink_slab_map); > + > /** > * shrink_slab - shrink slab caches > * @gfp_mask: allocation context > @@ -453,6 +456,8 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, > goto out; > } > > + lock_map_acquire(&__shrink_slab_map); > + > list_for_each_entry(shrinker, &shrinker_list, list) { > struct shrink_control sc = { > .gfp_mask = gfp_mask, > @@ -491,6 +496,8 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, > } > } > > + lock_map_release(&__shrink_slab_map); > + > up_read(&shrinker_rwsem); > out: > cond_resched(); I am not an expert on lockdep annotations. But is this something that makes sense? Don't you need lock_acquire_shared otherwise it will simply consider this a lockup if we succeed on trylock twice? But in any case the trylock already notes any dependency as the lockdep is involved when the lock is taken and we do not take any action otherwise. So what is the point? I am not familiar with XFS to read the lockdep trace properly. [...] > Normally shrinker_rwsem acts like a shared lock. But when > register_shrinker()/unregister_shrinker() called down_write(), > shrinker_rwsem suddenly starts acting like an exclusive lock. How come? We only do trylock and that means that we won't take it _after_ the write claims the lock. > What is unfortunate is that down_write() is called independent of > memory allocation requests. That is, shrinker_rwsem is essentially > a mutex (and hence the debug patch shown above). > > ---------------------------------------- > [<ffffffffac7538d3>] call_rwsem_down_write_failed+0x13/0x20 > [<ffffffffac1cb985>] register_shrinker+0x45/0xa0 > [<ffffffffac250f68>] sget_userns+0x468/0x4a0 > [<ffffffffac25106a>] mount_nodev+0x2a/0xa0 > [<ffffffffac251be4>] mount_fs+0x34/0x150 > [<ffffffffac2701f2>] vfs_kern_mount+0x62/0x120 > [<ffffffffac272a0e>] do_mount+0x1ee/0xc50 > [<ffffffffac27377e>] SyS_mount+0x7e/0xd0 > [<ffffffffac003831>] do_syscall_64+0x61/0x1a0 > [<ffffffffac80012c>] entry_SYSCALL64_slow_path+0x25/0x25 > [<ffffffffffffffff>] 0xffffffffffffffff > ---------------------------------------- > > Therefore, I think that when do_shrink_slab() for GFP_KERNEL is in progress > and down_read_trylock() starts failing because somebody else started waiting at > down_write(), do_shrink_slab() for GFP_NOFS or GFP_NOIO cannot be called. > Doesn't such race cause unexpected results? This is really hard to tell. I would expect that a skipped shrinkers would lead to an OOM killer sooner or later. As soon as the shrinker managed memory is the only one left for reclaim then we should OOM. And I do not see anything obvious that would prevent that. -- 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:[~2018-01-25 11:41 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 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 [this message] 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=20180125114104.GP28465@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=linux-mm@lists.ewheeler.net \ --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.