Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590006 > unrolled thread

Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature

Started byByungchul Park <byungchul.park@lge.com>
First post2017-03-01 06:50 +0100
Last post2017-03-03 01:50 +0100
Articles 11 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-01 06:50 +0100
    Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-01 15:50 +0100
      Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-02 18:50 +0100
        Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-03 01:40 +0100
          Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-03 09:20 +0100
            Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-03 10:50 +0100
              Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-03 10:50 +0100
              Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-05 04:40 +0100
            Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-05 04:10 +0100
              Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-07 19:50 +0100
        Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-03 01:50 +0100

#1590006 — Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature

FromByungchul Park <byungchul.park@lge.com>
Date2017-03-01 06:50 +0100
SubjectRe: [PATCH v5 06/13] lockdep: Implement crossrelease feature
Message-ID<tg5jc-6MD-7@gated-at.bofh.it>
On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > +static int commit_xhlocks(struct cross_lock *xlock)
> > +{
> > +	struct task_struct *curr = current;
> > +	struct hist_lock *xhlock_c = xhlock_curr(curr);
> > +	struct hist_lock *xhlock = xhlock_c;
> > +
> > +	do {
> > +		xhlock = xhlock_prev(curr, xhlock);
> > +
> > +		if (!xhlock_used(xhlock))
> > +			break;
> > +
> > +		if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id))
> > +			break;
> > +
> > +		if (same_context_xhlock(xhlock) &&
> > +		    before(xhlock->prev_gen_id, xlock->hlock.gen_id) &&
> > +		    !commit_xhlock(xlock, xhlock))
> > +			return 0;
> > +	} while (xhlock_c != xhlock);
> > +
> > +	return 1;
> > +}
> 
> So I'm still struggling with prev_gen_id; is it an optimization or is it
> required for correctness?

It's an optimization, but very essential and important optimization.

          in hlocks[]
          ------------
          A gen_id (4) --+
                         | previous gen_id
          B gen_id (3) <-+
          C gen_id (3)
          D gen_id (2)
oldest -> E gen_id (1)

          in xhlocks[]
          ------------
       ^  A gen_id (4) prev_gen_id (3: B's gen id)
       |  B gen_id (3) prev_gen_id (3: C's gen id)
       |  C gen_id (3) prev_gen_id (2: D's gen id)
       |  D gen_id (2) prev_gen_id (1: E's gen id)
       |  E gen_id (1) prev_gen_id (NA)

Let's consider the case that the gen id of xlock to commit is 3.

In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
'B -> A' which are already generated by original lockdep.

I use the prev_gen_id to avoid adding this kind of redundant
dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
means that the previous lock in hlocks[] is able to handle the
dependency on its commit stage.

[toc] | [next] | [standalone]

#1590366

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-01 15:50 +0100
Message-ID<tgdJM-4r7-21@gated-at.bofh.it>
In reply to#1590006
On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > > +static int commit_xhlocks(struct cross_lock *xlock)
> > > +{
> > > +	struct task_struct *curr = current;
> > > +	struct hist_lock *xhlock_c = xhlock_curr(curr);
> > > +	struct hist_lock *xhlock = xhlock_c;
> > > +
> > > +	do {
> > > +		xhlock = xhlock_prev(curr, xhlock);
> > > +
> > > +		if (!xhlock_used(xhlock))
> > > +			break;
> > > +
> > > +		if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id))
> > > +			break;
> > > +
> > > +		if (same_context_xhlock(xhlock) &&
> > > +		    before(xhlock->prev_gen_id, xlock->hlock.gen_id) &&
> > > +		    !commit_xhlock(xlock, xhlock))
> > > +			return 0;
> > > +	} while (xhlock_c != xhlock);
> > > +
> > > +	return 1;
> > > +}
> > 
> > So I'm still struggling with prev_gen_id; is it an optimization or is it
> > required for correctness?
> 
> It's an optimization, but very essential and important optimization.
> 
>           in hlocks[]
>           ------------
>           A gen_id (4) --+
>                          | previous gen_id
>           B gen_id (3) <-+
>           C gen_id (3)
>           D gen_id (2)
> oldest -> E gen_id (1)
> 
>           in xhlocks[]
>           ------------
>        ^  A gen_id (4) prev_gen_id (3: B's gen id)
>        |  B gen_id (3) prev_gen_id (3: C's gen id)
>        |  C gen_id (3) prev_gen_id (2: D's gen id)
>        |  D gen_id (2) prev_gen_id (1: E's gen id)
>        |  E gen_id (1) prev_gen_id (NA)
> 
> Let's consider the case that the gen id of xlock to commit is 3.
> 
> In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> 'B -> A' which are already generated by original lockdep.
> 
> I use the prev_gen_id to avoid adding this kind of redundant
> dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> means that the previous lock in hlocks[] is able to handle the
> dependency on its commit stage.
> 

Aah, I completely missed it was against held_locks.

Hurm.. it feels like this is solving a problem we shouldn't be solving
though.

That is, ideally we'd already be able to (quickly) tell if a relation
exists or not, but given how the whole chain_hash stuff is build now, it
looks like we cannot.


Let me think about this a bit more.

[toc] | [prev] | [next] | [standalone]

#1591344

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-02 18:50 +0100
Message-ID<tgD1w-5Gf-29@gated-at.bofh.it>
In reply to#1590366
On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:

> > It's an optimization, but very essential and important optimization.

Since its not for correctness, please put it in a separate patch with a
good Changelog, the below would make a good beginning of that.

Also, I feel, the source comments can be improved.

> >           in hlocks[]
> >           ------------
> >           A gen_id (4) --+
> >                          | previous gen_id
> >           B gen_id (3) <-+
> >           C gen_id (3)
> >           D gen_id (2)
> > oldest -> E gen_id (1)
> > 
> >           in xhlocks[]
> >           ------------
> >        ^  A gen_id (4) prev_gen_id (3: B's gen id)
> >        |  B gen_id (3) prev_gen_id (3: C's gen id)
> >        |  C gen_id (3) prev_gen_id (2: D's gen id)
> >        |  D gen_id (2) prev_gen_id (1: E's gen id)
> >        |  E gen_id (1) prev_gen_id (NA)
> > 
> > Let's consider the case that the gen id of xlock to commit is 3.
> > 
> > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > 'B -> A' which are already generated by original lockdep.
> > 
> > I use the prev_gen_id to avoid adding this kind of redundant
> > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > means that the previous lock in hlocks[] is able to handle the
> > dependency on its commit stage.
> > 
> 
> Aah, I completely missed it was against held_locks.
> 
> Hurm.. it feels like this is solving a problem we shouldn't be solving
> though.
> 
> That is, ideally we'd already be able to (quickly) tell if a relation
> exists or not, but given how the whole chain_hash stuff is build now, it
> looks like we cannot.
> 
> 
> Let me think about this a bit more.

OK, so neither this nor the chain-hash completely avoid redundant
dependencies. The only way to do that is doing graph-walks for every
proposed link.

Now, we already do a ton of __bfs() walks in check_prev_add(), so one
more might not hurt too much [*].

Esp. with the chain-hash avoiding all the obvious duplicate work, this
might just work.

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a95e5d1..7baea89 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 		}
 	}
 
+	/*
+	 * Is the <prev> -> <next> redundant?
+	 */
+	this.class = hlock_class(prev);
+	this.parent = NULL;
+	ret = check_noncircular(&this, hlock_class(next), &target_entry);
+	if (!ret) /* exists, redundant */
+		return 2;
+	if (ret < 0)
+		return print_bfs_bug(ret);
+
 	if (!*stack_saved) {
 		if (!save_trace(&trace))
 			return 0;



[*] A while ago someone, and I cannot find the email just now, asked if
we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock
like we use for other things like workqueues etc. I think this should be
possible which allows reducing the 'irq' states and will reduce the
amount of __bfs() lookups we do.

Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
not mistaken, more than making up for the 1 we'd have to add to detect
redundant links.


 include/linux/lockdep.h         | 11 +-----
 include/linux/sched.h           |  1 -
 kernel/locking/lockdep.c        | 87 +----------------------------------------
 kernel/locking/lockdep_states.h |  1 -
 mm/internal.h                   | 40 +++++++++++++++++++
 mm/page_alloc.c                 | 13 ++++--
 mm/slab.h                       |  7 +++-
 mm/slob.c                       |  8 +++-
 mm/vmscan.c                     | 13 +++---
 9 files changed, 71 insertions(+), 110 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
  * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
  * the total number of states... :-(
  */
-#define XXX_LOCK_USAGE_STATES		(1+3*4)
+#define XXX_LOCK_USAGE_STATES		(1+2*4)
 
 /*
  * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map *lock,
 	lock_set_class(lock, lock->name, lock->key, subclass, ip);
 }
 
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
 struct pin_cookie { unsigned int val; };
 
 #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
 extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
 extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
 
-# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0,
+# define INIT_LOCKDEP				.lockdep_recursion = 0,
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
@@ -413,9 +409,6 @@ static inline void lockdep_on(void)
 # define lock_release(l, n, i)			do { } while (0)
 # define lock_set_class(l, n, k, s, i)		do { } while (0)
 # define lock_set_subclass(l, s, i)		do { } while (0)
-# define lockdep_set_current_reclaim_state(g)	do { } while (0)
-# define lockdep_clear_current_reclaim_state()	do { } while (0)
-# define lockdep_trace_alloc(g)			do { } while (0)
 # define lockdep_info()				do { } while (0)
 # define lockdep_init_map(lock, name, key, sub) \
 		do { (void)(name); (void)(key); } while (0)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..0fa8a8f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -806,7 +806,6 @@ struct task_struct {
 	int				lockdep_depth;
 	unsigned int			lockdep_recursion;
 	struct held_lock		held_locks[MAX_LOCK_DEPTH];
-	gfp_t				lockdep_reclaim_gfp;
 #endif
 
 #ifdef CONFIG_UBSAN
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a95e5d1..1051600 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -343,14 +343,12 @@ EXPORT_SYMBOL(lockdep_on);
 #if VERBOSE
 # define HARDIRQ_VERBOSE	1
 # define SOFTIRQ_VERBOSE	1
-# define RECLAIM_VERBOSE	1
 #else
 # define HARDIRQ_VERBOSE	0
 # define SOFTIRQ_VERBOSE	0
-# define RECLAIM_VERBOSE	0
 #endif
 
-#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE || RECLAIM_VERBOSE
+#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE
 /*
  * Quick filtering for interesting events:
  */
@@ -2553,14 +2551,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
 	return 0;
 }
 
-static int RECLAIM_FS_verbose(struct lock_class *class)
-{
-#if RECLAIM_VERBOSE
-	return class_filter(class);
-#endif
-	return 0;
-}
-
 #define STRICT_READ_CHECKS	1
 
 static int (*state_verbose_f[])(struct lock_class *class) = {
@@ -2856,51 +2846,6 @@ void trace_softirqs_off(unsigned long ip)
 		debug_atomic_inc(redundant_softirqs_off);
 }
 
-static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
-{
-	struct task_struct *curr = current;
-
-	if (unlikely(!debug_locks))
-		return;
-
-	/* no reclaim without waiting on it */
-	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
-		return;
-
-	/* this guy won't enter reclaim */
-	if ((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
-		return;
-
-	/* We're only interested __GFP_FS allocations for now */
-	if (!(gfp_mask & __GFP_FS))
-		return;
-
-	/*
-	 * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
-	 */
-	if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
-		return;
-
-	mark_held_locks(curr, RECLAIM_FS);
-}
-
-static void check_flags(unsigned long flags);
-
-void lockdep_trace_alloc(gfp_t gfp_mask)
-{
-	unsigned long flags;
-
-	if (unlikely(current->lockdep_recursion))
-		return;
-
-	raw_local_irq_save(flags);
-	check_flags(flags);
-	current->lockdep_recursion = 1;
-	__lockdep_trace_alloc(gfp_mask, flags);
-	current->lockdep_recursion = 0;
-	raw_local_irq_restore(flags);
-}
-
 static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
 {
 	/*
@@ -2946,22 +2891,6 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
 		}
 	}
 
-	/*
-	 * We reuse the irq context infrastructure more broadly as a general
-	 * context checking code. This tests GFP_FS recursion (a lock taken
-	 * during reclaim for a GFP_FS allocation is held over a GFP_FS
-	 * allocation).
-	 */
-	if (!hlock->trylock && (curr->lockdep_reclaim_gfp & __GFP_FS)) {
-		if (hlock->read) {
-			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS_READ))
-					return 0;
-		} else {
-			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS))
-					return 0;
-		}
-	}
-
 	return 1;
 }
 
@@ -3020,10 +2949,6 @@ static inline int separate_irq_context(struct task_struct *curr,
 	return 0;
 }
 
-void lockdep_trace_alloc(gfp_t gfp_mask)
-{
-}
-
 #endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
 
 /*
@@ -3859,16 +3784,6 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 }
 EXPORT_SYMBOL_GPL(lock_unpin_lock);
 
-void lockdep_set_current_reclaim_state(gfp_t gfp_mask)
-{
-	current->lockdep_reclaim_gfp = gfp_mask;
-}
-
-void lockdep_clear_current_reclaim_state(void)
-{
-	current->lockdep_reclaim_gfp = 0;
-}
-
 #ifdef CONFIG_LOCK_STAT
 static int
 print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
diff --git a/kernel/locking/lockdep_states.h b/kernel/locking/lockdep_states.h
index 995b0cc..35ca09f 100644
--- a/kernel/locking/lockdep_states.h
+++ b/kernel/locking/lockdep_states.h
@@ -6,4 +6,3 @@
  */
 LOCKDEP_STATE(HARDIRQ)
 LOCKDEP_STATE(SOFTIRQ)
-LOCKDEP_STATE(RECLAIM_FS)
diff --git a/mm/internal.h b/mm/internal.h
index ccfc2a2..88b9107 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -15,6 +15,8 @@
 #include <linux/mm.h>
 #include <linux/pagemap.h>
 #include <linux/tracepoint-defs.h>
+#include <linux/lockdep.h>
+#include <linux/sched/mm.h>
 
 /*
  * The set of flags that only affect watermark checking and reclaim
@@ -498,4 +500,42 @@ extern const struct trace_print_flags pageflag_names[];
 extern const struct trace_print_flags vmaflag_names[];
 extern const struct trace_print_flags gfpflag_names[];
 
+
+#ifdef CONFIG_LOCKDEP
+extern struct lockdep_map __fs_reclaim_map;
+
+static inline bool __need_fs_reclaim(gfp_t gfp_mask)
+{
+	gfp_mask = memalloc_noio_flags(gfp_mask);
+
+	/* no reclaim without waiting on it */
+	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
+		return false;
+
+	/* this guy won't enter reclaim */
+	if ((current->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
+		return false;
+
+	/* We're only interested __GFP_FS allocations for now */
+	if (!(gfp_mask & __GFP_FS))
+		return false;
+
+	return true;
+}
+
+static inline void fs_reclaim_acquire(gfp_t gfp_mask)
+{
+	if (__need_fs_reclaim(gfp_mask))
+		lock_map_acquire(&__fs_reclaim_map);
+}
+static inline void fs_reclaim_release(gfp_t gfp_mask)
+{
+	if (__need_fs_reclaim(gfp_mask))
+		lock_map_release(&__fs_reclaim_map);
+}
+#else
+static inline void fs_reclaim_acquire(gfp_t gfp_mask) { }
+static inline void fs_reclaim_release(gfp_t gfp_mask) { }
+#endif
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eaa64d2..85ea8bf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3387,6 +3387,12 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 }
 #endif /* CONFIG_COMPACTION */
 
+
+#ifdef CONFIG_LOCKDEP
+struct lockdep_map __fs_reclaim_map =
+	STATIC_LOCKDEP_MAP_INIT("fs_reclaim", &__fs_reclaim_map);
+#endif
+
 /* Perform direct synchronous page reclaim */
 static int
 __perform_reclaim(gfp_t gfp_mask, unsigned int order,
@@ -3400,7 +3406,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
 	/* We now go into synchronous reclaim */
 	cpuset_memory_pressure_bump();
 	current->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	current->reclaim_state = &reclaim_state;
 
@@ -3408,7 +3414,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
 								ac->nodemask);
 
 	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
+	fs_reclaim_release(gfp_mask);
 	current->flags &= ~PF_MEMALLOC;
 
 	cond_resched();
@@ -3913,7 +3919,8 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 			*alloc_flags |= ALLOC_CPUSET;
 	}
 
-	lockdep_trace_alloc(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
+	fs_reclaim_release(gfp_mask);
 
 	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
 
diff --git a/mm/slab.h b/mm/slab.h
index 65e7c3f..753f552 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -44,6 +44,8 @@ struct kmem_cache {
 #include <linux/kmemleak.h>
 #include <linux/random.h>
 
+#include "internal.h"
+
 /*
  * State of the slab allocator.
  *
@@ -428,7 +430,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
 						     gfp_t flags)
 {
 	flags &= gfp_allowed_mask;
-	lockdep_trace_alloc(flags);
+
+	fs_reclaim_acquire(flags);
+	fs_reclaim_release(flags);
+
 	might_sleep_if(gfpflags_allow_blocking(flags));
 
 	if (should_failslab(s, flags))
diff --git a/mm/slob.c b/mm/slob.c
index eac04d43..3e32280 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -73,6 +73,8 @@
 #include <linux/atomic.h>
 
 #include "slab.h"
+#include "internal.h"
+
 /*
  * slob_block has a field 'units', which indicates size of block if +ve,
  * or offset of next block if -ve (in SLOB_UNITs).
@@ -432,7 +434,8 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 
 	gfp &= gfp_allowed_mask;
 
-	lockdep_trace_alloc(gfp);
+	fs_reclaim_acquire(gfp);
+	fs_reclaim_release(gfp);
 
 	if (size < PAGE_SIZE - align) {
 		if (!size)
@@ -538,7 +541,8 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 
 	flags &= gfp_allowed_mask;
 
-	lockdep_trace_alloc(flags);
+	fs_reclaim_acquire(flags);
+	fs_reclaim_release(flags);
 
 	if (c->size < PAGE_SIZE) {
 		b = slob_alloc(c->size, flags, c->align, node);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index bc8031e..2f57e36 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3418,8 +3418,6 @@ static int kswapd(void *p)
 	};
 	const struct cpumask *cpumask = cpumask_of_node(pgdat->node_id);
 
-	lockdep_set_current_reclaim_state(GFP_KERNEL);
-
 	if (!cpumask_empty(cpumask))
 		set_cpus_allowed_ptr(tsk, cpumask);
 	current->reclaim_state = &reclaim_state;
@@ -3475,7 +3473,9 @@ static int kswapd(void *p)
 		 */
 		trace_mm_vmscan_kswapd_wake(pgdat->node_id, classzone_idx,
 						alloc_order);
+		fs_reclaim_acquire(GFP_KERNEL);
 		reclaim_order = balance_pgdat(pgdat, alloc_order, classzone_idx);
+		fs_reclaim_release(GFP_KERNEL);
 		if (reclaim_order < alloc_order)
 			goto kswapd_try_sleep;
 
@@ -3485,7 +3485,6 @@ static int kswapd(void *p)
 
 	tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD);
 	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
 
 	return 0;
 }
@@ -3550,14 +3549,14 @@ unsigned long shrink_all_memory(unsigned long nr_to_reclaim)
 	unsigned long nr_reclaimed;
 
 	p->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(sc.gfp_mask);
+	fs_reclaim_acquire(sc.gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
 
 	p->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
+	fs_reclaim_release(sc.gfp_mask);
 	p->flags &= ~PF_MEMALLOC;
 
 	return nr_reclaimed;
@@ -3741,7 +3740,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	 * and RECLAIM_UNMAP.
 	 */
 	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
@@ -3756,8 +3755,8 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	}
 
 	p->reclaim_state = NULL;
+	fs_reclaim_release(gfp_mask);
 	current->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE);
-	lockdep_clear_current_reclaim_state();
 	return sc.nr_reclaimed >= nr_pages;
 }

[toc] | [prev] | [next] | [standalone]

#1591582

FromByungchul Park <byungchul.park@lge.com>
Date2017-03-03 01:40 +0100
Message-ID<tgJqi-1AZ-23@gated-at.bofh.it>
In reply to#1591344
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> 
> > > It's an optimization, but very essential and important optimization.
> 
> Since its not for correctness, please put it in a separate patch with a
> good Changelog, the below would make a good beginning of that.

OK. I will do it.

> Also, I feel, the source comments can be improved.
> 
> > >           in hlocks[]
> > >           ------------
> > >           A gen_id (4) --+
> > >                          | previous gen_id
> > >           B gen_id (3) <-+
> > >           C gen_id (3)
> > >           D gen_id (2)
> > > oldest -> E gen_id (1)
> > > 
> > >           in xhlocks[]
> > >           ------------
> > >        ^  A gen_id (4) prev_gen_id (3: B's gen id)
> > >        |  B gen_id (3) prev_gen_id (3: C's gen id)
> > >        |  C gen_id (3) prev_gen_id (2: D's gen id)
> > >        |  D gen_id (2) prev_gen_id (1: E's gen id)
> > >        |  E gen_id (1) prev_gen_id (NA)
> > > 
> > > Let's consider the case that the gen id of xlock to commit is 3.
> > > 
> > > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > > 'B -> A' which are already generated by original lockdep.
> > > 
> > > I use the prev_gen_id to avoid adding this kind of redundant
> > > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > > means that the previous lock in hlocks[] is able to handle the
> > > dependency on its commit stage.

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..7baea89 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
>  		}
>  	}
>  
> +	/*
> +	 * Is the <prev> -> <next> redundant?
> +	 */
> +	this.class = hlock_class(prev);
> +	this.parent = NULL;
> +	ret = check_noncircular(&this, hlock_class(next), &target_entry);
> +	if (!ret) /* exists, redundant */
> +		return 2;
> +	if (ret < 0)
> +		return print_bfs_bug(ret);
> +
>  	if (!*stack_saved) {
>  		if (!save_trace(&trace))
>  			return 0;

This whoud be very nice if you allow to add this code. However, prev_gen_id
thingy is still useful, the code above can achieve it though. Agree?

[toc] | [prev] | [next] | [standalone]

#1591738

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-03 09:20 +0100
Message-ID<tgQBs-6J0-19@gated-at.bofh.it>
In reply to#1591582
On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:

> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index a95e5d1..7baea89 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Is the <prev> -> <next> redundant?
> > +	 */
> > +	this.class = hlock_class(prev);
> > +	this.parent = NULL;
> > +	ret = check_noncircular(&this, hlock_class(next), &target_entry);
> > +	if (!ret) /* exists, redundant */
> > +		return 2;
> > +	if (ret < 0)
> > +		return print_bfs_bug(ret);
> > +
> >  	if (!*stack_saved) {
> >  		if (!save_trace(&trace))
> >  			return 0;
> 
> This whoud be very nice if you allow to add this code. However, prev_gen_id
> thingy is still useful, the code above can achieve it though. Agree?

So my goal was to avoid prev_gen_id, and yes I think the above does
that.

Now the problem with the above condition is that it makes reports
harder to decipher, because by avoiding adding redundant links to our
graph we loose a possible shorter path.

So while for correctness sake it doesn't matter, it is irrelevant how
long the cycle is after all, all that matters is that there is a cycle.
But the humans on the receiving end tend to like shorter cycles.

And I think the same is true for crossrelease, avoiding redundant links
increases cycle length.

(And remember, BFS will otherwise find the shortest cycle.)

That said; I'd be fairly interested in numbers on how many links this
avoids, I'll go make a check_redundant() version of the above and put a
proper counter in so I can see what it does for a regular boot etc..

[toc] | [prev] | [next] | [standalone]

#1591803

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-03 10:50 +0100
Message-ID<tgS0x-7G0-3@gated-at.bofh.it>
In reply to#1591738
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:

> That said; I'd be fairly interested in numbers on how many links this
> avoids, I'll go make a check_redundant() version of the above and put a
> proper counter in so I can see what it does for a regular boot etc..

Two boots + a make defconfig, the first didn't have the redundant bit
in, the second did (full diff below still includes the reclaim rework,
because that was still in that kernel and I forgot to reset the tree).


 lock-classes:                         1168       1169 [max: 8191]
 direct dependencies:                  7688       5812 [max: 32768]
 indirect dependencies:               25492      25937
 all direct dependencies:            220113     217512
 dependency chains:                    9005       9008 [max: 65536]
 dependency chain hlocks:             34450      34366 [max: 327680]
 in-hardirq chains:                      55         51
 in-softirq chains:                     371        378
 in-process chains:                    8579       8579
 stack-trace entries:                108073      88474 [max: 524288]
 combined max dependencies:       178738560  169094640

 max locking depth:                      15         15
 max bfs queue depth:                   320        329

 cyclic checks:                        9123       9190

 redundant checks:                                5046
 redundant links:                                 1828

 find-mask forwards checks:            2564       2599
 find-mask backwards checks:          39521      39789


So it saves nearly 2k links and a fair chunk of stack-trace entries, but
as expected, makes no real difference on the indirect dependencies.

At the same time, you see the max BFS depth increase, which is also
expected, although it could easily be boot variance -- these numbers are
not entirely stable between boots.

Could you run something similar? Or I'll take a look on your next spin
of the patches.



---
 include/linux/lockdep.h            |  11 +---
 include/linux/sched.h              |   1 -
 kernel/locking/lockdep.c           | 114 +++++++++----------------------------
 kernel/locking/lockdep_internals.h |   2 +
 kernel/locking/lockdep_proc.c      |   4 ++
 kernel/locking/lockdep_states.h    |   1 -
 mm/internal.h                      |  40 +++++++++++++
 mm/page_alloc.c                    |  13 ++++-
 mm/slab.h                          |   7 ++-
 mm/slob.c                          |   8 ++-
 mm/vmscan.c                        |  13 ++---
 11 files changed, 104 insertions(+), 110 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
  * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
  * the total number of states... :-(
  */
-#define XXX_LOCK_USAGE_STATES		(1+3*4)
+#define XXX_LOCK_USAGE_STATES		(1+2*4)
 
 /*
  * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map *lock,
 	lock_set_class(lock, lock->name, lock->key, subclass, ip);
 }
 
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
 struct pin_cookie { unsigned int val; };
 
 #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
 extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
 extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
 
-# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0,
+# define INIT_LOCKDEP				.lockdep_recursion = 0,
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
@@ -413,9 +409,6 @@ static inline void lockdep_on(void)
 # define lock_release(l, n, i)			do { } while (0)
 # define lock_set_class(l, n, k, s, i)		do { } while (0)
 # define lock_set_subclass(l, s, i)		do { } while (0)
-# define lockdep_set_current_reclaim_state(g)	do { } while (0)
-# define lockdep_clear_current_reclaim_state()	do { } while (0)
-# define lockdep_trace_alloc(g)			do { } while (0)
 # define lockdep_info()				do { } while (0)
 # define lockdep_init_map(lock, name, key, sub) \
 		do { (void)(name); (void)(key); } while (0)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..0fa8a8f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -806,7 +806,6 @@ struct task_struct {
 	int				lockdep_depth;
 	unsigned int			lockdep_recursion;
 	struct held_lock		held_locks[MAX_LOCK_DEPTH];
-	gfp_t				lockdep_reclaim_gfp;
 #endif
 
 #ifdef CONFIG_UBSAN
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a95e5d1..e3cc398 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -343,14 +343,12 @@ EXPORT_SYMBOL(lockdep_on);
 #if VERBOSE
 # define HARDIRQ_VERBOSE	1
 # define SOFTIRQ_VERBOSE	1
-# define RECLAIM_VERBOSE	1
 #else
 # define HARDIRQ_VERBOSE	0
 # define SOFTIRQ_VERBOSE	0
-# define RECLAIM_VERBOSE	0
 #endif
 
-#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE || RECLAIM_VERBOSE
+#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE
 /*
  * Quick filtering for interesting events:
  */
@@ -1295,6 +1293,19 @@ check_noncircular(struct lock_list *root, struct lock_class *target,
 	return result;
 }
 
+static noinline int
+check_redundant(struct lock_list *root, struct lock_class *target,
+		struct lock_list **target_entry)
+{
+	int result;
+
+	debug_atomic_inc(nr_redundant_checks);
+
+	result = __bfs_forwards(root, target, class_equal, target_entry);
+
+	return result;
+}
+
 #if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING)
 /*
  * Forwards and backwards subgraph searching, for the purposes of
@@ -1860,6 +1871,20 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 		}
 	}
 
+	/*
+	 * Is the <prev> -> <next> link redundant?
+	 */
+	this.class = hlock_class(prev);
+	this.parent = NULL;
+	ret = check_redundant(&this, hlock_class(next), &target_entry);
+	if (!ret) {
+		debug_atomic_inc(nr_redundant);
+		return 2;
+	}
+	if (ret < 0)
+		return print_bfs_bug(ret);
+
+
 	if (!*stack_saved) {
 		if (!save_trace(&trace))
 			return 0;
@@ -2553,14 +2578,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
 	return 0;
 }
 
-static int RECLAIM_FS_verbose(struct lock_class *class)
-{
-#if RECLAIM_VERBOSE
-	return class_filter(class);
-#endif
-	return 0;
-}
-
 #define STRICT_READ_CHECKS	1
 
 static int (*state_verbose_f[])(struct lock_class *class) = {
@@ -2856,51 +2873,6 @@ void trace_softirqs_off(unsigned long ip)
 		debug_atomic_inc(redundant_softirqs_off);
 }
 
-static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
-{
-	struct task_struct *curr = current;
-
-	if (unlikely(!debug_locks))
-		return;
-
-	/* no reclaim without waiting on it */
-	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
-		return;
-
-	/* this guy won't enter reclaim */
-	if ((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
-		return;
-
-	/* We're only interested __GFP_FS allocations for now */
-	if (!(gfp_mask & __GFP_FS))
-		return;
-
-	/*
-	 * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
-	 */
-	if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
-		return;
-
-	mark_held_locks(curr, RECLAIM_FS);
-}
-
-static void check_flags(unsigned long flags);
-
-void lockdep_trace_alloc(gfp_t gfp_mask)
-{
-	unsigned long flags;
-
-	if (unlikely(current->lockdep_recursion))
-		return;
-
-	raw_local_irq_save(flags);
-	check_flags(flags);
-	current->lockdep_recursion = 1;
-	__lockdep_trace_alloc(gfp_mask, flags);
-	current->lockdep_recursion = 0;
-	raw_local_irq_restore(flags);
-}
-
 static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
 {
 	/*
@@ -2946,22 +2918,6 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
 		}
 	}
 
-	/*
-	 * We reuse the irq context infrastructure more broadly as a general
-	 * context checking code. This tests GFP_FS recursion (a lock taken
-	 * during reclaim for a GFP_FS allocation is held over a GFP_FS
-	 * allocation).
-	 */
-	if (!hlock->trylock && (curr->lockdep_reclaim_gfp & __GFP_FS)) {
-		if (hlock->read) {
-			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS_READ))
-					return 0;
-		} else {
-			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS))
-					return 0;
-		}
-	}
-
 	return 1;
 }
 
@@ -3020,10 +2976,6 @@ static inline int separate_irq_context(struct task_struct *curr,
 	return 0;
 }
 
-void lockdep_trace_alloc(gfp_t gfp_mask)
-{
-}
-
 #endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
 
 /*
@@ -3859,16 +3811,6 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 }
 EXPORT_SYMBOL_GPL(lock_unpin_lock);
 
-void lockdep_set_current_reclaim_state(gfp_t gfp_mask)
-{
-	current->lockdep_reclaim_gfp = gfp_mask;
-}
-
-void lockdep_clear_current_reclaim_state(void)
-{
-	current->lockdep_reclaim_gfp = 0;
-}
-
 #ifdef CONFIG_LOCK_STAT
 static int
 print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h
index c2b8849..7809269 100644
--- a/kernel/locking/lockdep_internals.h
+++ b/kernel/locking/lockdep_internals.h
@@ -143,6 +143,8 @@ struct lockdep_stats {
 	int	redundant_softirqs_on;
 	int	redundant_softirqs_off;
 	int	nr_unused_locks;
+	int	nr_redundant_checks;
+	int	nr_redundant;
 	int	nr_cyclic_checks;
 	int	nr_cyclic_check_recursions;
 	int	nr_find_usage_forwards_checks;
diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c
index 6d1fcc7..68d9e26 100644
--- a/kernel/locking/lockdep_proc.c
+++ b/kernel/locking/lockdep_proc.c
@@ -201,6 +201,10 @@ static void lockdep_stats_debug_show(struct seq_file *m)
 		debug_atomic_read(chain_lookup_hits));
 	seq_printf(m, " cyclic checks:                 %11llu\n",
 		debug_atomic_read(nr_cyclic_checks));
+	seq_printf(m, " redundant checks:              %11llu\n",
+		debug_atomic_read(nr_redundant_checks));
+	seq_printf(m, " redundant links:               %11llu\n",
+		debug_atomic_read(nr_redundant));
 	seq_printf(m, " find-mask forwards checks:     %11llu\n",
 		debug_atomic_read(nr_find_usage_forwards_checks));
 	seq_printf(m, " find-mask backwards checks:    %11llu\n",
diff --git a/kernel/locking/lockdep_states.h b/kernel/locking/lockdep_states.h
index 995b0cc..35ca09f 100644
--- a/kernel/locking/lockdep_states.h
+++ b/kernel/locking/lockdep_states.h
@@ -6,4 +6,3 @@
  */
 LOCKDEP_STATE(HARDIRQ)
 LOCKDEP_STATE(SOFTIRQ)
-LOCKDEP_STATE(RECLAIM_FS)
diff --git a/mm/internal.h b/mm/internal.h
index ccfc2a2..88b9107 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -15,6 +15,8 @@
 #include <linux/mm.h>
 #include <linux/pagemap.h>
 #include <linux/tracepoint-defs.h>
+#include <linux/lockdep.h>
+#include <linux/sched/mm.h>
 
 /*
  * The set of flags that only affect watermark checking and reclaim
@@ -498,4 +500,42 @@ extern const struct trace_print_flags pageflag_names[];
 extern const struct trace_print_flags vmaflag_names[];
 extern const struct trace_print_flags gfpflag_names[];
 
+
+#ifdef CONFIG_LOCKDEP
+extern struct lockdep_map __fs_reclaim_map;
+
+static inline bool __need_fs_reclaim(gfp_t gfp_mask)
+{
+	gfp_mask = memalloc_noio_flags(gfp_mask);
+
+	/* no reclaim without waiting on it */
+	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
+		return false;
+
+	/* this guy won't enter reclaim */
+	if ((current->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
+		return false;
+
+	/* We're only interested __GFP_FS allocations for now */
+	if (!(gfp_mask & __GFP_FS))
+		return false;
+
+	return true;
+}
+
+static inline void fs_reclaim_acquire(gfp_t gfp_mask)
+{
+	if (__need_fs_reclaim(gfp_mask))
+		lock_map_acquire(&__fs_reclaim_map);
+}
+static inline void fs_reclaim_release(gfp_t gfp_mask)
+{
+	if (__need_fs_reclaim(gfp_mask))
+		lock_map_release(&__fs_reclaim_map);
+}
+#else
+static inline void fs_reclaim_acquire(gfp_t gfp_mask) { }
+static inline void fs_reclaim_release(gfp_t gfp_mask) { }
+#endif
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eaa64d2..85ea8bf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3387,6 +3387,12 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 }
 #endif /* CONFIG_COMPACTION */
 
+
+#ifdef CONFIG_LOCKDEP
+struct lockdep_map __fs_reclaim_map =
+	STATIC_LOCKDEP_MAP_INIT("fs_reclaim", &__fs_reclaim_map);
+#endif
+
 /* Perform direct synchronous page reclaim */
 static int
 __perform_reclaim(gfp_t gfp_mask, unsigned int order,
@@ -3400,7 +3406,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
 	/* We now go into synchronous reclaim */
 	cpuset_memory_pressure_bump();
 	current->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	current->reclaim_state = &reclaim_state;
 
@@ -3408,7 +3414,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
 								ac->nodemask);
 
 	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
+	fs_reclaim_release(gfp_mask);
 	current->flags &= ~PF_MEMALLOC;
 
 	cond_resched();
@@ -3913,7 +3919,8 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 			*alloc_flags |= ALLOC_CPUSET;
 	}
 
-	lockdep_trace_alloc(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
+	fs_reclaim_release(gfp_mask);
 
 	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
 
diff --git a/mm/slab.h b/mm/slab.h
index 65e7c3f..753f552 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -44,6 +44,8 @@ struct kmem_cache {
 #include <linux/kmemleak.h>
 #include <linux/random.h>
 
+#include "internal.h"
+
 /*
  * State of the slab allocator.
  *
@@ -428,7 +430,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
 						     gfp_t flags)
 {
 	flags &= gfp_allowed_mask;
-	lockdep_trace_alloc(flags);
+
+	fs_reclaim_acquire(flags);
+	fs_reclaim_release(flags);
+
 	might_sleep_if(gfpflags_allow_blocking(flags));
 
 	if (should_failslab(s, flags))
diff --git a/mm/slob.c b/mm/slob.c
index eac04d43..3e32280 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -73,6 +73,8 @@
 #include <linux/atomic.h>
 
 #include "slab.h"
+#include "internal.h"
+
 /*
  * slob_block has a field 'units', which indicates size of block if +ve,
  * or offset of next block if -ve (in SLOB_UNITs).
@@ -432,7 +434,8 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 
 	gfp &= gfp_allowed_mask;
 
-	lockdep_trace_alloc(gfp);
+	fs_reclaim_acquire(gfp);
+	fs_reclaim_release(gfp);
 
 	if (size < PAGE_SIZE - align) {
 		if (!size)
@@ -538,7 +541,8 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 
 	flags &= gfp_allowed_mask;
 
-	lockdep_trace_alloc(flags);
+	fs_reclaim_acquire(flags);
+	fs_reclaim_release(flags);
 
 	if (c->size < PAGE_SIZE) {
 		b = slob_alloc(c->size, flags, c->align, node);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index bc8031e..2f57e36 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3418,8 +3418,6 @@ static int kswapd(void *p)
 	};
 	const struct cpumask *cpumask = cpumask_of_node(pgdat->node_id);
 
-	lockdep_set_current_reclaim_state(GFP_KERNEL);
-
 	if (!cpumask_empty(cpumask))
 		set_cpus_allowed_ptr(tsk, cpumask);
 	current->reclaim_state = &reclaim_state;
@@ -3475,7 +3473,9 @@ static int kswapd(void *p)
 		 */
 		trace_mm_vmscan_kswapd_wake(pgdat->node_id, classzone_idx,
 						alloc_order);
+		fs_reclaim_acquire(GFP_KERNEL);
 		reclaim_order = balance_pgdat(pgdat, alloc_order, classzone_idx);
+		fs_reclaim_release(GFP_KERNEL);
 		if (reclaim_order < alloc_order)
 			goto kswapd_try_sleep;
 
@@ -3485,7 +3485,6 @@ static int kswapd(void *p)
 
 	tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD);
 	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
 
 	return 0;
 }
@@ -3550,14 +3549,14 @@ unsigned long shrink_all_memory(unsigned long nr_to_reclaim)
 	unsigned long nr_reclaimed;
 
 	p->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(sc.gfp_mask);
+	fs_reclaim_acquire(sc.gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
 
 	p->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
+	fs_reclaim_release(sc.gfp_mask);
 	p->flags &= ~PF_MEMALLOC;
 
 	return nr_reclaimed;
@@ -3741,7 +3740,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	 * and RECLAIM_UNMAP.
 	 */
 	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	fs_reclaim_acquire(gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
@@ -3756,8 +3755,8 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	}
 
 	p->reclaim_state = NULL;
+	fs_reclaim_release(gfp_mask);
 	current->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE);
-	lockdep_clear_current_reclaim_state();
 	return sc.nr_reclaimed >= nr_pages;
 }

[toc] | [prev] | [next] | [standalone]

#1591806

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-03 10:50 +0100
Message-ID<tgS0y-7G0-11@gated-at.bofh.it>
In reply to#1591803
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> 
> > That said; I'd be fairly interested in numbers on how many links this
> > avoids, I'll go make a check_redundant() version of the above and put a
> > proper counter in so I can see what it does for a regular boot etc..
> 
> Two boots + a make defconfig, the first didn't have the redundant bit
> in, the second did (full diff below still includes the reclaim rework,
> because that was still in that kernel and I forgot to reset the tree).
> 
> 
>  lock-classes:                         1168       1169 [max: 8191]
>  direct dependencies:                  7688       5812 [max: 32768]
>  indirect dependencies:               25492      25937
>  all direct dependencies:            220113     217512
>  dependency chains:                    9005       9008 [max: 65536]
>  dependency chain hlocks:             34450      34366 [max: 327680]
>  in-hardirq chains:                      55         51
>  in-softirq chains:                     371        378
>  in-process chains:                    8579       8579
>  stack-trace entries:                108073      88474 [max: 524288]
>  combined max dependencies:       178738560  169094640
> 
>  max locking depth:                      15         15
>  max bfs queue depth:                   320        329
> 
>  cyclic checks:                        9123       9190
> 
>  redundant checks:                                5046
>  redundant links:                                 1828
> 
>  find-mask forwards checks:            2564       2599
>  find-mask backwards checks:          39521      39789
> 

OK, last email, I promise, then I'll go bury myself in futexes.

 find-mask forwards checks:            2999
 find-mask backwards checks:          56134

Is with a clean kernel, which shows how many __bfs() calls we save by
doing away with that RECLAIM state. OTOH:

 lock-classes:                         1167 [max: 8191]
 direct dependencies:                  7254 [max: 32768]
 indirect dependencies:               23763
 all direct dependencies:            219093

Shows that the added reclaim class isn't entirely free either ;-)

[toc] | [prev] | [next] | [standalone]

#1592691

FromByungchul Park <byungchul.park@lge.com>
Date2017-03-05 04:40 +0100
Message-ID<thvbz-2o1-1@gated-at.bofh.it>
In reply to#1591803
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> 
> Two boots + a make defconfig, the first didn't have the redundant bit
> in, the second did (full diff below still includes the reclaim rework,
> because that was still in that kernel and I forgot to reset the tree).
> 
> 
>  lock-classes:                         1168       1169 [max: 8191]
>  direct dependencies:                  7688       5812 [max: 32768]
>  indirect dependencies:               25492      25937
>  all direct dependencies:            220113     217512
>  dependency chains:                    9005       9008 [max: 65536]
>  dependency chain hlocks:             34450      34366 [max: 327680]
>  in-hardirq chains:                      55         51
>  in-softirq chains:                     371        378
>  in-process chains:                    8579       8579
>  stack-trace entries:                108073      88474 [max: 524288]
>  combined max dependencies:       178738560  169094640
> 
>  max locking depth:                      15         15
>  max bfs queue depth:                   320        329
> 
>  cyclic checks:                        9123       9190
> 
>  redundant checks:                                5046
>  redundant links:                                 1828
> 
>  find-mask forwards checks:            2564       2599
>  find-mask backwards checks:          39521      39789
> 
> 
> So it saves nearly 2k links and a fair chunk of stack-trace entries, but

It's as we expect.

> as expected, makes no real difference on the indirect dependencies.

It looks that the indirect dependencies increased to me. This result is
also somewhat anticipated.

> At the same time, you see the max BFS depth increase, which is also

Yes. The depth should increase.

> expected, although it could easily be boot variance -- these numbers are
> not entirely stable between boots.
> 
> Could you run something similar? Or I'll take a look on your next spin
> of the patches.

I will check same thing you did and let you know the result at next spin.

[toc] | [prev] | [next] | [standalone]

#1592689

FromByungchul Park <byungchul.park@lge.com>
Date2017-03-05 04:10 +0100
Message-ID<thuIx-257-7@gated-at.bofh.it>
In reply to#1591738
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> > On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> 
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index a95e5d1..7baea89 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * Is the <prev> -> <next> redundant?
> > > +	 */
> > > +	this.class = hlock_class(prev);
> > > +	this.parent = NULL;
> > > +	ret = check_noncircular(&this, hlock_class(next), &target_entry);
> > > +	if (!ret) /* exists, redundant */
> > > +		return 2;
> > > +	if (ret < 0)
> > > +		return print_bfs_bug(ret);
> > > +
> > >  	if (!*stack_saved) {
> > >  		if (!save_trace(&trace))
> > >  			return 0;
> > 
> > This whoud be very nice if you allow to add this code. However, prev_gen_id
> > thingy is still useful, the code above can achieve it though. Agree?
> 
> So my goal was to avoid prev_gen_id, and yes I think the above does
> that.
> 
> Now the problem with the above condition is that it makes reports
> harder to decipher, because by avoiding adding redundant links to our
> graph we loose a possible shorter path.

Let's see the following example:

   A -> B -> C

   where A, B and C are typical lock class.

Assume the graph above was built and operations happena in the
following order:

   CONTEXT X		CONTEXT Y
   ---------		---------
   acquire DX
			acquire A
			acquire B
			acquire C

			release and commit DX

   where A, B and C are typical lock class, DX is a crosslock class.

The graph will grow as following _without_ prev_gen_id.

        -> A -> B -> C
       /    /    /
   DX -----------

   where A, B and C are typical lock class, DX is a crosslock class.

The graph will grow as following _with_ prev_gen_id.

   DX -> A -> B -> C

   where A, B and C are typical lock class, DX is a crosslock class.

You said the former is better because it has smaller cost in bfs. But it
has to use _much_ more memory to keep additional nodes in graph. Without
exaggeration, every crosslock would get linked with all locks in history
locks, on commit, unless redundant. It might be pretty more than we
expect - I will check and let you know how many it is. Is it still good?

[toc] | [prev] | [next] | [standalone]

#1594513

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-07 19:50 +0100
Message-ID<tislj-35L-3@gated-at.bofh.it>
In reply to#1592689
On Sun, Mar 05, 2017 at 12:08:45PM +0900, Byungchul Park wrote:
> On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:

> > 
> > Now the problem with the above condition is that it makes reports
> > harder to decipher, because by avoiding adding redundant links to our
> > graph we loose a possible shorter path.
> 
> Let's see the following example:
> 
>    A -> B -> C
> 
>    where A, B and C are typical lock class.
> 
> Assume the graph above was built and operations happena in the
> following order:
> 
>    CONTEXT X		CONTEXT Y
>    ---------		---------
>    acquire DX
> 			acquire A
> 			acquire B
> 			acquire C
> 
> 			release and commit DX
> 
>    where A, B and C are typical lock class, DX is a crosslock class.
> 
> The graph will grow as following _without_ prev_gen_id.
> 
>         -> A -> B -> C
>        /    /    /
>    DX -----------
> 
>    where A, B and C are typical lock class, DX is a crosslock class.
> 
> The graph will grow as following _with_ prev_gen_id.
> 
>    DX -> A -> B -> C
> 
>    where A, B and C are typical lock class, DX is a crosslock class.
> 
> You said the former is better because it has smaller cost in bfs.

No, I said the former is better because when you report a DX inversion
against C, A and B are not required and the report is easier to
understand by _humans_.

I don't particularly care about the BFS cost itself.

> But it has to use _much_ more memory to keep additional nodes in
> graph. Without exaggeration, every crosslock would get linked with all
> locks in history locks, on commit, unless redundant. It might be
> pretty more than we expect - I will check and let you know how many it
> is. Is it still good?

Dunno, probably not.. but it would be good to have numbers.

[toc] | [prev] | [next] | [standalone]

#1591586

FromByungchul Park <byungchul.park@lge.com>
Date2017-03-03 01:50 +0100
Message-ID<tgJzY-1FS-5@gated-at.bofh.it>
In reply to#1591344
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> [*] A while ago someone, and I cannot find the email just now, asked if
> we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock

It looks interesting to me.

> like we use for other things like workqueues etc. I think this should be
> possible which allows reducing the 'irq' states and will reduce the
> amount of __bfs() lookups we do.
> 
> Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
> not mistaken, more than making up for the 1 we'd have to add to detect
> redundant links.

OK.

Thanks,
Byungchul

> 
> 
>  include/linux/lockdep.h         | 11 +-----
>  include/linux/sched.h           |  1 -
>  kernel/locking/lockdep.c        | 87 +----------------------------------------
>  kernel/locking/lockdep_states.h |  1 -
>  mm/internal.h                   | 40 +++++++++++++++++++
>  mm/page_alloc.c                 | 13 ++++--
>  mm/slab.h                       |  7 +++-
>  mm/slob.c                       |  8 +++-
>  mm/vmscan.c                     | 13 +++---
>  9 files changed, 71 insertions(+), 110 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 1e327bb..6ba1a65 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -29,7 +29,7 @@ extern int lock_stat;
>   * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
>   * the total number of states... :-(
>   */
> -#define XXX_LOCK_USAGE_STATES		(1+3*4)
> +#define XXX_LOCK_USAGE_STATES		(1+2*4)
>  
>  /*
>   * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> @@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map *lock,
>  	lock_set_class(lock, lock->name, lock->key, subclass, ip);
>  }
>  
> -extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
> -extern void lockdep_clear_current_reclaim_state(void);
> -extern void lockdep_trace_alloc(gfp_t mask);
> -
>  struct pin_cookie { unsigned int val; };
>  
>  #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
> @@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
>  extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
>  extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
>  
> -# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0,
> +# define INIT_LOCKDEP				.lockdep_recursion = 0,
>  
>  #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
>  
> @@ -413,9 +409,6 @@ static inline void lockdep_on(void)
>  # define lock_release(l, n, i)			do { } while (0)
>  # define lock_set_class(l, n, k, s, i)		do { } while (0)
>  # define lock_set_subclass(l, s, i)		do { } while (0)
> -# define lockdep_set_current_reclaim_state(g)	do { } while (0)
> -# define lockdep_clear_current_reclaim_state()	do { } while (0)
> -# define lockdep_trace_alloc(g)			do { } while (0)
>  # define lockdep_info()				do { } while (0)
>  # define lockdep_init_map(lock, name, key, sub) \
>  		do { (void)(name); (void)(key); } while (0)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee8..0fa8a8f 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -806,7 +806,6 @@ struct task_struct {
>  	int				lockdep_depth;
>  	unsigned int			lockdep_recursion;
>  	struct held_lock		held_locks[MAX_LOCK_DEPTH];
> -	gfp_t				lockdep_reclaim_gfp;
>  #endif
>  
>  #ifdef CONFIG_UBSAN
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..1051600 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -343,14 +343,12 @@ EXPORT_SYMBOL(lockdep_on);
>  #if VERBOSE
>  # define HARDIRQ_VERBOSE	1
>  # define SOFTIRQ_VERBOSE	1
> -# define RECLAIM_VERBOSE	1
>  #else
>  # define HARDIRQ_VERBOSE	0
>  # define SOFTIRQ_VERBOSE	0
> -# define RECLAIM_VERBOSE	0
>  #endif
>  
> -#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE || RECLAIM_VERBOSE
> +#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE
>  /*
>   * Quick filtering for interesting events:
>   */
> @@ -2553,14 +2551,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
>  	return 0;
>  }
>  
> -static int RECLAIM_FS_verbose(struct lock_class *class)
> -{
> -#if RECLAIM_VERBOSE
> -	return class_filter(class);
> -#endif
> -	return 0;
> -}
> -
>  #define STRICT_READ_CHECKS	1
>  
>  static int (*state_verbose_f[])(struct lock_class *class) = {
> @@ -2856,51 +2846,6 @@ void trace_softirqs_off(unsigned long ip)
>  		debug_atomic_inc(redundant_softirqs_off);
>  }
>  
> -static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
> -{
> -	struct task_struct *curr = current;
> -
> -	if (unlikely(!debug_locks))
> -		return;
> -
> -	/* no reclaim without waiting on it */
> -	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
> -		return;
> -
> -	/* this guy won't enter reclaim */
> -	if ((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
> -		return;
> -
> -	/* We're only interested __GFP_FS allocations for now */
> -	if (!(gfp_mask & __GFP_FS))
> -		return;
> -
> -	/*
> -	 * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
> -	 */
> -	if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
> -		return;
> -
> -	mark_held_locks(curr, RECLAIM_FS);
> -}
> -
> -static void check_flags(unsigned long flags);
> -
> -void lockdep_trace_alloc(gfp_t gfp_mask)
> -{
> -	unsigned long flags;
> -
> -	if (unlikely(current->lockdep_recursion))
> -		return;
> -
> -	raw_local_irq_save(flags);
> -	check_flags(flags);
> -	current->lockdep_recursion = 1;
> -	__lockdep_trace_alloc(gfp_mask, flags);
> -	current->lockdep_recursion = 0;
> -	raw_local_irq_restore(flags);
> -}
> -
>  static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
>  {
>  	/*
> @@ -2946,22 +2891,6 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)
>  		}
>  	}
>  
> -	/*
> -	 * We reuse the irq context infrastructure more broadly as a general
> -	 * context checking code. This tests GFP_FS recursion (a lock taken
> -	 * during reclaim for a GFP_FS allocation is held over a GFP_FS
> -	 * allocation).
> -	 */
> -	if (!hlock->trylock && (curr->lockdep_reclaim_gfp & __GFP_FS)) {
> -		if (hlock->read) {
> -			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS_READ))
> -					return 0;
> -		} else {
> -			if (!mark_lock(curr, hlock, LOCK_USED_IN_RECLAIM_FS))
> -					return 0;
> -		}
> -	}
> -
>  	return 1;
>  }
>  
> @@ -3020,10 +2949,6 @@ static inline int separate_irq_context(struct task_struct *curr,
>  	return 0;
>  }
>  
> -void lockdep_trace_alloc(gfp_t gfp_mask)
> -{
> -}
> -
>  #endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
>  
>  /*
> @@ -3859,16 +3784,6 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
>  }
>  EXPORT_SYMBOL_GPL(lock_unpin_lock);
>  
> -void lockdep_set_current_reclaim_state(gfp_t gfp_mask)
> -{
> -	current->lockdep_reclaim_gfp = gfp_mask;
> -}
> -
> -void lockdep_clear_current_reclaim_state(void)
> -{
> -	current->lockdep_reclaim_gfp = 0;
> -}
> -
>  #ifdef CONFIG_LOCK_STAT
>  static int
>  print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
> diff --git a/kernel/locking/lockdep_states.h b/kernel/locking/lockdep_states.h
> index 995b0cc..35ca09f 100644
> --- a/kernel/locking/lockdep_states.h
> +++ b/kernel/locking/lockdep_states.h
> @@ -6,4 +6,3 @@
>   */
>  LOCKDEP_STATE(HARDIRQ)
>  LOCKDEP_STATE(SOFTIRQ)
> -LOCKDEP_STATE(RECLAIM_FS)
> diff --git a/mm/internal.h b/mm/internal.h
> index ccfc2a2..88b9107 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -15,6 +15,8 @@
>  #include <linux/mm.h>
>  #include <linux/pagemap.h>
>  #include <linux/tracepoint-defs.h>
> +#include <linux/lockdep.h>
> +#include <linux/sched/mm.h>
>  
>  /*
>   * The set of flags that only affect watermark checking and reclaim
> @@ -498,4 +500,42 @@ extern const struct trace_print_flags pageflag_names[];
>  extern const struct trace_print_flags vmaflag_names[];
>  extern const struct trace_print_flags gfpflag_names[];
>  
> +
> +#ifdef CONFIG_LOCKDEP
> +extern struct lockdep_map __fs_reclaim_map;
> +
> +static inline bool __need_fs_reclaim(gfp_t gfp_mask)
> +{
> +	gfp_mask = memalloc_noio_flags(gfp_mask);
> +
> +	/* no reclaim without waiting on it */
> +	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
> +		return false;
> +
> +	/* this guy won't enter reclaim */
> +	if ((current->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
> +		return false;
> +
> +	/* We're only interested __GFP_FS allocations for now */
> +	if (!(gfp_mask & __GFP_FS))
> +		return false;
> +
> +	return true;
> +}
> +
> +static inline void fs_reclaim_acquire(gfp_t gfp_mask)
> +{
> +	if (__need_fs_reclaim(gfp_mask))
> +		lock_map_acquire(&__fs_reclaim_map);
> +}
> +static inline void fs_reclaim_release(gfp_t gfp_mask)
> +{
> +	if (__need_fs_reclaim(gfp_mask))
> +		lock_map_release(&__fs_reclaim_map);
> +}
> +#else
> +static inline void fs_reclaim_acquire(gfp_t gfp_mask) { }
> +static inline void fs_reclaim_release(gfp_t gfp_mask) { }
> +#endif
> +
>  #endif	/* __MM_INTERNAL_H */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index eaa64d2..85ea8bf 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3387,6 +3387,12 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>  }
>  #endif /* CONFIG_COMPACTION */
>  
> +
> +#ifdef CONFIG_LOCKDEP
> +struct lockdep_map __fs_reclaim_map =
> +	STATIC_LOCKDEP_MAP_INIT("fs_reclaim", &__fs_reclaim_map);
> +#endif
> +
>  /* Perform direct synchronous page reclaim */
>  static int
>  __perform_reclaim(gfp_t gfp_mask, unsigned int order,
> @@ -3400,7 +3406,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
>  	/* We now go into synchronous reclaim */
>  	cpuset_memory_pressure_bump();
>  	current->flags |= PF_MEMALLOC;
> -	lockdep_set_current_reclaim_state(gfp_mask);
> +	fs_reclaim_acquire(gfp_mask);
>  	reclaim_state.reclaimed_slab = 0;
>  	current->reclaim_state = &reclaim_state;
>  
> @@ -3408,7 +3414,7 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order,
>  								ac->nodemask);
>  
>  	current->reclaim_state = NULL;
> -	lockdep_clear_current_reclaim_state();
> +	fs_reclaim_release(gfp_mask);
>  	current->flags &= ~PF_MEMALLOC;
>  
>  	cond_resched();
> @@ -3913,7 +3919,8 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
>  			*alloc_flags |= ALLOC_CPUSET;
>  	}
>  
> -	lockdep_trace_alloc(gfp_mask);
> +	fs_reclaim_acquire(gfp_mask);
> +	fs_reclaim_release(gfp_mask);
>  
>  	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
>  
> diff --git a/mm/slab.h b/mm/slab.h
> index 65e7c3f..753f552 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -44,6 +44,8 @@ struct kmem_cache {
>  #include <linux/kmemleak.h>
>  #include <linux/random.h>
>  
> +#include "internal.h"
> +
>  /*
>   * State of the slab allocator.
>   *
> @@ -428,7 +430,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
>  						     gfp_t flags)
>  {
>  	flags &= gfp_allowed_mask;
> -	lockdep_trace_alloc(flags);
> +
> +	fs_reclaim_acquire(flags);
> +	fs_reclaim_release(flags);
> +
>  	might_sleep_if(gfpflags_allow_blocking(flags));
>  
>  	if (should_failslab(s, flags))
> diff --git a/mm/slob.c b/mm/slob.c
> index eac04d43..3e32280 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -73,6 +73,8 @@
>  #include <linux/atomic.h>
>  
>  #include "slab.h"
> +#include "internal.h"
> +
>  /*
>   * slob_block has a field 'units', which indicates size of block if +ve,
>   * or offset of next block if -ve (in SLOB_UNITs).
> @@ -432,7 +434,8 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
>  
>  	gfp &= gfp_allowed_mask;
>  
> -	lockdep_trace_alloc(gfp);
> +	fs_reclaim_acquire(gfp);
> +	fs_reclaim_release(gfp);
>  
>  	if (size < PAGE_SIZE - align) {
>  		if (!size)
> @@ -538,7 +541,8 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
>  
>  	flags &= gfp_allowed_mask;
>  
> -	lockdep_trace_alloc(flags);
> +	fs_reclaim_acquire(flags);
> +	fs_reclaim_release(flags);
>  
>  	if (c->size < PAGE_SIZE) {
>  		b = slob_alloc(c->size, flags, c->align, node);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bc8031e..2f57e36 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3418,8 +3418,6 @@ static int kswapd(void *p)
>  	};
>  	const struct cpumask *cpumask = cpumask_of_node(pgdat->node_id);
>  
> -	lockdep_set_current_reclaim_state(GFP_KERNEL);
> -
>  	if (!cpumask_empty(cpumask))
>  		set_cpus_allowed_ptr(tsk, cpumask);
>  	current->reclaim_state = &reclaim_state;
> @@ -3475,7 +3473,9 @@ static int kswapd(void *p)
>  		 */
>  		trace_mm_vmscan_kswapd_wake(pgdat->node_id, classzone_idx,
>  						alloc_order);
> +		fs_reclaim_acquire(GFP_KERNEL);
>  		reclaim_order = balance_pgdat(pgdat, alloc_order, classzone_idx);
> +		fs_reclaim_release(GFP_KERNEL);
>  		if (reclaim_order < alloc_order)
>  			goto kswapd_try_sleep;
>  
> @@ -3485,7 +3485,6 @@ static int kswapd(void *p)
>  
>  	tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD);
>  	current->reclaim_state = NULL;
> -	lockdep_clear_current_reclaim_state();
>  
>  	return 0;
>  }
> @@ -3550,14 +3549,14 @@ unsigned long shrink_all_memory(unsigned long nr_to_reclaim)
>  	unsigned long nr_reclaimed;
>  
>  	p->flags |= PF_MEMALLOC;
> -	lockdep_set_current_reclaim_state(sc.gfp_mask);
> +	fs_reclaim_acquire(sc.gfp_mask);
>  	reclaim_state.reclaimed_slab = 0;
>  	p->reclaim_state = &reclaim_state;
>  
>  	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
>  
>  	p->reclaim_state = NULL;
> -	lockdep_clear_current_reclaim_state();
> +	fs_reclaim_release(sc.gfp_mask);
>  	p->flags &= ~PF_MEMALLOC;
>  
>  	return nr_reclaimed;
> @@ -3741,7 +3740,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
>  	 * and RECLAIM_UNMAP.
>  	 */
>  	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
> -	lockdep_set_current_reclaim_state(gfp_mask);
> +	fs_reclaim_acquire(gfp_mask);
>  	reclaim_state.reclaimed_slab = 0;
>  	p->reclaim_state = &reclaim_state;
>  
> @@ -3756,8 +3755,8 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
>  	}
>  
>  	p->reclaim_state = NULL;
> +	fs_reclaim_release(gfp_mask);
>  	current->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE);
> -	lockdep_clear_current_reclaim_state();
>  	return sc.nr_reclaimed >= nr_pages;
>  }
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web