From: Peter Zijlstra <peterz@infradead.org> To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: torvalds@linux-foundation.org, davej@codemonkey.org.uk, npiggin@gmail.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, mhocko@kernel.org Subject: Re: [4.15-rc9] fs_reclaim lockdep trace Date: Mon, 29 Jan 2018 14:55:47 +0100 [thread overview] Message-ID: <20180129135547.GR2269@hirez.programming.kicks-ass.net> (raw) In-Reply-To: <201801292047.EHC05241.OHSQOJOVtFMFLF@I-love.SAKURA.ne.jp> On Mon, Jan 29, 2018 at 08:47:20PM +0900, Tetsuo Handa wrote: > Peter Zijlstra wrote: > > On Sun, Jan 28, 2018 at 02:55:28PM +0900, Tetsuo Handa wrote: > > > This warning seems to be caused by commit d92a8cfcb37ecd13 > > > ("locking/lockdep: Rework FS_RECLAIM annotation") which moved the > > > location of > > > > > > /* this guy won't enter reclaim */ > > > if ((current->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC)) > > > return false; > > > > > > check added by commit cf40bd16fdad42c0 ("lockdep: annotate reclaim context > > > (__GFP_NOFS)"). > > > > I'm not entirly sure I get what you mean here. How did I move it? It was > > part of lockdep_trace_alloc(), if __GFP_NOMEMALLOC was set, it would not > > mark the lock as held. > > d92a8cfcb37ecd13 replaced lockdep_set_current_reclaim_state() with > fs_reclaim_acquire(), and removed current->lockdep_recursion handling. > > ---------- > # git show d92a8cfcb37ecd13 | grep recursion > -# define INIT_LOCKDEP .lockdep_recursion = 0, .lockdep_reclaim_gfp = 0, > +# define INIT_LOCKDEP .lockdep_recursion = 0, > unsigned int lockdep_recursion; > - if (unlikely(current->lockdep_recursion)) > - current->lockdep_recursion = 1; > - current->lockdep_recursion = 0; > - * context checking code. This tests GFP_FS recursion (a lock taken > ---------- That should not matter at all. The only case that would matter for is if lockdep itself would ever call into lockdep again. Not something that happens here. > > The new code has it in fs_reclaim_acquire/release to the same effect, if > > __GFP_NOMEMALLOC, we'll not acquire/release the lock. > > Excuse me, but I can't catch. > We currently acquire/release __fs_reclaim_map if __GFP_NOMEMALLOC. Right, got the case inverted, same difference though. Before we'd do mark_held_lock(), now we do acquire/release under the same conditions. > > > Since __kmalloc_reserve() from __alloc_skb() adds > > > __GFP_NOMEMALLOC | __GFP_NOWARN to gfp_mask, __need_fs_reclaim() is > > > failing to return false despite PF_MEMALLOC context (and resulted in > > > lockdep warning). > > > > But that's correct right, __GFP_NOMEMALLOC should negate PF_MEMALLOC. > > That's what the name says. > > __GFP_NOMEMALLOC negates PF_MEMALLOC regarding what watermark that allocation > request should use. Right. > But at the same time, PF_MEMALLOC negates __GFP_DIRECT_RECLAIM. Ah indeed. > Then, how can fs_reclaim contribute to deadlock? Not sure it can. But if we're going to allow this, it needs to come with a clear description on why. Not a few clues to a puzzle. Now, even if its not strictly a deadlock, there is something to be said for flagging GFP_FS allocs that lead to nested GFP_FS allocs, do we ever want to allow that?
WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org> To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: torvalds@linux-foundation.org, davej@codemonkey.org.uk, npiggin@gmail.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, mhocko@kernel.org Subject: Re: [4.15-rc9] fs_reclaim lockdep trace Date: Mon, 29 Jan 2018 14:55:47 +0100 [thread overview] Message-ID: <20180129135547.GR2269@hirez.programming.kicks-ass.net> (raw) In-Reply-To: <201801292047.EHC05241.OHSQOJOVtFMFLF@I-love.SAKURA.ne.jp> On Mon, Jan 29, 2018 at 08:47:20PM +0900, Tetsuo Handa wrote: > Peter Zijlstra wrote: > > On Sun, Jan 28, 2018 at 02:55:28PM +0900, Tetsuo Handa wrote: > > > This warning seems to be caused by commit d92a8cfcb37ecd13 > > > ("locking/lockdep: Rework FS_RECLAIM annotation") which moved the > > > location of > > > > > > /* this guy won't enter reclaim */ > > > if ((current->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC)) > > > return false; > > > > > > check added by commit cf40bd16fdad42c0 ("lockdep: annotate reclaim context > > > (__GFP_NOFS)"). > > > > I'm not entirly sure I get what you mean here. How did I move it? It was > > part of lockdep_trace_alloc(), if __GFP_NOMEMALLOC was set, it would not > > mark the lock as held. > > d92a8cfcb37ecd13 replaced lockdep_set_current_reclaim_state() with > fs_reclaim_acquire(), and removed current->lockdep_recursion handling. > > ---------- > # git show d92a8cfcb37ecd13 | grep recursion > -# define INIT_LOCKDEP .lockdep_recursion = 0, .lockdep_reclaim_gfp = 0, > +# define INIT_LOCKDEP .lockdep_recursion = 0, > unsigned int lockdep_recursion; > - if (unlikely(current->lockdep_recursion)) > - current->lockdep_recursion = 1; > - current->lockdep_recursion = 0; > - * context checking code. This tests GFP_FS recursion (a lock taken > ---------- That should not matter at all. The only case that would matter for is if lockdep itself would ever call into lockdep again. Not something that happens here. > > The new code has it in fs_reclaim_acquire/release to the same effect, if > > __GFP_NOMEMALLOC, we'll not acquire/release the lock. > > Excuse me, but I can't catch. > We currently acquire/release __fs_reclaim_map if __GFP_NOMEMALLOC. Right, got the case inverted, same difference though. Before we'd do mark_held_lock(), now we do acquire/release under the same conditions. > > > Since __kmalloc_reserve() from __alloc_skb() adds > > > __GFP_NOMEMALLOC | __GFP_NOWARN to gfp_mask, __need_fs_reclaim() is > > > failing to return false despite PF_MEMALLOC context (and resulted in > > > lockdep warning). > > > > But that's correct right, __GFP_NOMEMALLOC should negate PF_MEMALLOC. > > That's what the name says. > > __GFP_NOMEMALLOC negates PF_MEMALLOC regarding what watermark that allocation > request should use. Right. > But at the same time, PF_MEMALLOC negates __GFP_DIRECT_RECLAIM. Ah indeed. > Then, how can fs_reclaim contribute to deadlock? Not sure it can. But if we're going to allow this, it needs to come with a clear description on why. Not a few clues to a puzzle. Now, even if its not strictly a deadlock, there is something to be said for flagging GFP_FS allocs that lead to nested GFP_FS allocs, do we ever want to allow that? -- 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-29 13:55 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-01-24 1:36 [4.15-rc9] fs_reclaim lockdep trace Dave Jones 2018-01-24 1:36 ` Dave Jones 2018-01-27 22:24 ` Dave Jones 2018-01-27 22:24 ` Dave Jones 2018-01-27 22:43 ` Linus Torvalds 2018-01-27 22:43 ` Linus Torvalds 2018-01-28 1:16 ` Tetsuo Handa 2018-01-28 1:16 ` Tetsuo Handa 2018-01-28 4:25 ` Tetsuo Handa 2018-01-28 4:25 ` Tetsuo Handa 2018-01-28 5:55 ` Tetsuo Handa 2018-01-28 5:55 ` Tetsuo Handa 2018-01-28 5:55 ` Tetsuo Handa 2018-01-29 2:43 ` Dave Jones 2018-01-29 2:43 ` Dave Jones 2018-01-29 10:27 ` Peter Zijlstra 2018-01-29 10:27 ` Peter Zijlstra 2018-01-29 11:47 ` Tetsuo Handa 2018-01-29 11:47 ` Tetsuo Handa 2018-01-29 13:55 ` Peter Zijlstra [this message] 2018-01-29 13:55 ` Peter Zijlstra 2018-02-01 11:36 ` Tetsuo Handa 2018-02-01 11:36 ` Tetsuo Handa 2018-02-08 11:43 ` [PATCH v2] lockdep: Fix fs_reclaim warning Tetsuo Handa 2018-02-08 11:43 ` Tetsuo Handa 2018-02-12 12:08 ` Nikolay Borisov 2018-02-12 12:08 ` Nikolay Borisov 2018-02-12 13:46 ` Tetsuo Handa 2018-02-12 13:46 ` Tetsuo Handa 2018-02-19 11:52 ` Tetsuo Handa 2018-02-19 11:52 ` Tetsuo Handa 2018-02-27 21:50 ` [PATCH v2 (RESEND)] " Tetsuo Handa 2018-03-07 21:44 ` Tetsuo Handa 2018-03-07 23:33 ` Andrew Morton 2018-03-08 15:30 ` Tetsuo Handa
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=20180129135547.GR2269@hirez.programming.kicks-ass.net \ --to=peterz@infradead.org \ --cc=davej@codemonkey.org.uk \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=npiggin@gmail.com \ --cc=penguin-kernel@I-love.SAKURA.ne.jp \ --cc=torvalds@linux-foundation.org \ /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.