xfs
[Top] [All Lists]

Re: Xfs lockdep warning with for-dave-for-4.6 branch

To: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Subject: Re: Xfs lockdep warning with for-dave-for-4.6 branch
From: Michal Hocko <mhocko@xxxxxxxxxx>
Date: Thu, 2 Jun 2016 16:50:49 +0200
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>, Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, linux-mm@xxxxxxxxx, Ingo Molnar <mingo@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20160601181617.GV3190@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <20160516104130.GK3193@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20160516130519.GJ23146@xxxxxxxxxxxxxx> <20160516132541.GP3193@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20160516231056.GE18496@dastard> <20160517144912.GZ3193@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20160517223549.GV26977@dastard> <20160519081146.GS3193@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20160520001714.GC26977@dastard> <20160601131758.GO26601@xxxxxxxxxxxxxx> <20160601181617.GV3190@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.6.0 (2016-04-01)
On Wed 01-06-16 20:16:17, Peter Zijlstra wrote:
> On Wed, Jun 01, 2016 at 03:17:58PM +0200, Michal Hocko wrote:
> > Thanks Dave for your detailed explanation again! Peter do you have any
> > other idea how to deal with these situations other than opt out from
> > lockdep reclaim machinery?
> > 
> > If not I would rather go with an annotation than a gfp flag to be honest
> > but if you absolutely hate that approach then I will try to check wheter
> > a CONFIG_LOCKDEP GFP_FOO doesn't break something else. Otherwise I would
> > steal the description from Dave's email and repost my patch.
> > 
> > I plan to repost my scope gfp patches in few days and it would be good
> > to have some mechanism to drop those GFP_NOFS to paper over lockdep
> > false positives for that.
> 
> Right; sorry I got side-tracked in other things again.
> 
> 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

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.
-- 
Michal Hocko
SUSE Labs

<Prev in Thread] Current Thread [Next in Thread>