On Thu, Jun 02, 2016 at 04:50:49PM +0200, Michal Hocko wrote:
> On Wed 01-06-16 20:16:17, Peter Zijlstra wrote:
> > So my favourite is the dedicated GFP flag, but if that's unpalatable for
> > the mm folks then something like the below might work. It should be
> > similar in effect to your proposal, except its more limited in scope.
> [...]
> > @@ -2876,11 +2883,36 @@ static void __lockdep_trace_alloc(gfp_t gfp_mask,
> > unsigned long flags)
> > if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
> > return;
> >
> > + /*
> > + * Skip _one_ allocation as per the lockdep_skip_alloc() request.
> > + * Must be done last so that we don't loose the annotation for
> > + * GFP_ATOMIC like things from IRQ or other nesting contexts.
> > + */
> > + if (current->lockdep_reclaim_gfp & __GFP_SKIP_ALLOC) {
> > + current->lockdep_reclaim_gfp &= ~__GFP_SKIP_ALLOC;
> > + return;
> > + }
> > +
> > mark_held_locks(curr, RECLAIM_FS);
> > }
>
> I might be missing something but does this work actually? Say you would
> want a kmalloc(size), it would call
> slab_alloc_node
> slab_pre_alloc_hook
> lockdep_trace_alloc
> [...]
> ____cache_alloc_node
> cache_grow_begin
> kmem_getpages
> __alloc_pages_node
> __alloc_pages_nodemask
> lockdep_trace_alloc
Bugger :/ You're right, that would fail.
So how about doing:
#define __GFP_NOLOCKDEP (1u << __GFP_BITS_SHIFT)
this means it cannot be part of address_space::flags or
radix_tree_root::gfp_mask, but that might not be a bad thing.
And this solves the scarcity thing, because per pagemap we need to have
5 'spare' bits anyway.
> I understand your concerns about the scope but usually all allocations
> have to be __GFP_NOFS or none in the same scope so I would see it as a
> huge deal.
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.
|