linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Michal Hocko <mhocko@kernel.org>
Cc: linux-mm@kvack.org, Peter Zijlstra <peterz@infradead.org>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	xfs@oss.sgi.com, Qu Wenruo <quwenruo@cn.fujitsu.com>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: Xfs lockdep warning with for-dave-for-4.6 branch
Date: Wed, 15 Jun 2016 17:21:54 +1000	[thread overview]
Message-ID: <20160615072154.GF26977@dastard> (raw)
In-Reply-To: <20160606122022.GH11895@dhcp22.suse.cz>

[sorry for the slow repsonse - been on holidays]

On Mon, Jun 06, 2016 at 02:20:22PM +0200, Michal Hocko wrote:
> On Fri 03-06-16 09:22:54, Dave Chinner wrote:
> > On Thu, Jun 02, 2016 at 05:46:19PM +0200, Michal Hocko wrote:
> > > On Thu 02-06-16 17:11:16, Peter Zijlstra wrote:
> > > > With scope I mostly meant the fact that you have two calls that you need
> > > > to pair up. That's not really nice as you can 'annotate' a _lot_ of code
> > > > in between. I prefer the narrower annotations where you annotate a
> > > > single specific site.
> > > 
> > > Yes, I can see you point. What I meant to say is that we would most
> > > probably end up with the following pattern
> > > 	lockdep_trace_alloc_enable()
> > > 	some_foo_with_alloc(gfp_mask);
> > > 	lockdep_trace_alloc_disable()
> > >
> > > and some_foo_with_alloc might be a lot of code.
> > 
> > That's the problem I see with this - the only way to make it
> > maintainable is to precede each enable/disable() pair with a comment
> > explaining *exactly* what those calls are protecting.  And that, in
> > itself, becomes a maintenance problem, because then code several
> > layers deep has no idea what context it is being called from and we
> > are likely to disable warnings in contexts where we probably
> > shouldn't be.
> 
> I am not sure I understand what you mean here. I thought the problem is
> that:
> 
> func_A (!trans. context)		func_B (trans. context)
>  foo1()					  foo2()
>    bar(inode, GFP_KERNEL)		    bar(inode, GFP_NOFS)
> 
> so bar(inode, gfp) can be called from two different contexts which
> would confuse the lockdep.

Yes, that's the core of the problem. What I think you are missing is
the scale of the problem.

> And the workaround would be annotating bar
> depending on the context it is called from - either pass a special gfp
> flag or do disable/enable thing. In both cases that anotation should be
> global for the whole func_A, no? Or is it possible that something in
> that path would really need a reclaim lockdep detection?

The problem is that there are cases where the call stack that leads
to bar() has many different entry points. See, for example, the
xfs_bmapi*() interfaces.  They all end up in the same low level
btree traversal code (and hence memory allocation points).
xfs_bmapi_read() can be called from both inside and outside
transaction context and there's ~30 callers we'd have to audit and
annotate. Then there's ~10 callers of xfs_bmapi_write which are all
within transaction context.

And then there's xfs_bmapi_delay(), which can end up in the same
low level code outside transaction context. Then there's
10 callers of xfs_bunmapi(), which runs both inside and outside
transaction context, too. Add to that all the miscellenous points
that can read extents off disk, and you get another ~12 entry
points.

Hopefully you can see the complexity of the issue - for an allocation
in the bmap btree code that could occur outside both inside and
outside of a transaction context, we've got to work out which of
those ~60 high level entry points would need to be annotated. And
then we have to ensure that in future we don't miss adding or
removing an annotation as we change the code deep inside the btree
implementation. It's the latter that is the long term maintainence
problem the hihg-level annotation approach introduces.

> > I think such an annotation approach really requires per-alloc site
> > annotation, the reason for it should be more obvious from the
> > context. e.g. any function that does memory alloc and takes an
> > optional transaction context needs annotation. Hence, from an XFS
> > perspective, I think it makes more sense to add a new KM_ flag to
> > indicate this call site requirement, then jump through whatever
> > lockdep hoop is required within the kmem_* allocation wrappers.
> > e.g, we can ignore the new KM_* flag if we are in a transaction
> > context and so the flag is only activated in the situations were
> > we currently enforce an external GFP_NOFS context from the call
> > site.....
> 
> Hmm, I thought we would achive this by using the scope GFP_NOFS usage
> which would mark those transaction related conctexts and no lockdep
> specific workarounds would be needed...

There are allocations outside transaction context which need to be
GFP_NOFS - this is what KM_NOFS was originally intended for. We need to
disambiguate the two cases where we use KM_NOFS to shut up lockdep
vs the cases where it is necessary to prevent reclaim deadlocks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2016-06-15  7:22 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-12  5:53 Xfs lockdep warning with for-dave-for-4.6 branch Qu Wenruo
2016-05-12  5:57 ` Darrick J. Wong
2016-05-12  8:03   ` Dave Chinner
2016-05-13 16:03     ` Michal Hocko
2016-05-16 10:41       ` Peter Zijlstra
2016-05-16 13:05         ` Michal Hocko
2016-05-16 13:25           ` Peter Zijlstra
2016-05-16 23:10             ` Dave Chinner
2016-05-17 14:49               ` Peter Zijlstra
2016-05-17 22:35                 ` Dave Chinner
2016-05-18  7:20                   ` Peter Zijlstra
2016-05-18  8:25                     ` Michal Hocko
2016-05-18  9:49                       ` Peter Zijlstra
2016-05-18 11:31                         ` Michal Hocko
2016-05-19  8:11                   ` Peter Zijlstra
2016-05-20  0:17                     ` Dave Chinner
2016-06-01 13:17                       ` Michal Hocko
2016-06-01 18:16                         ` Peter Zijlstra
2016-06-02 14:50                           ` Michal Hocko
2016-06-02 15:11                             ` Peter Zijlstra
2016-06-02 15:46                               ` Michal Hocko
2016-06-02 23:22                                 ` Dave Chinner
2016-06-06 12:20                                   ` Michal Hocko
2016-06-15  7:21                                     ` Dave Chinner [this message]
2016-06-21 14:26                                       ` Michal Hocko
2016-06-22  1:03                                         ` Dave Chinner
2016-06-22 12:38                                           ` Michal Hocko
2016-06-22 22:58                                             ` Dave Chinner
2016-06-23 11:35                                               ` Michal Hocko
2016-10-06 13:04                           ` Michal Hocko
2016-10-17 13:49                             ` Michal Hocko
2016-10-19  0:33                             ` Dave Chinner
2016-10-19  5:30                               ` Dave Chinner
2016-10-19  8:33                             ` Peter Zijlstra
2016-10-19 12:06                               ` Michal Hocko
2016-10-19 21:49                                 ` Dave Chinner
2016-10-20  7:15                                   ` Michal Hocko

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=20160615072154.GF26977@dastard \
    --to=david@fromorbit.com \
    --cc=darrick.wong@oracle.com \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=quwenruo@cn.fujitsu.com \
    --cc=xfs@oss.sgi.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).