Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Wed, Jan 18, 2017 at 04:10:53PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 11:04:32AM +0900, Byungchul Park wrote:
> > On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> > > On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > > > What do you think about the following patches doing it?
> > > 
> > > I was more thinking about something like so...
> > > 
> > > Also, I think I want to muck with struct stack_trace; the members:
> > > max_nr_entries and skip are input arguments to save_stack_trace() and
> > > bloat the structure for no reason.
> > 
> > With your approach, save_trace() must be called whenever check_prevs_add()
> > is called, which might be unnecessary.
> 
> True.. but since we hold the graph_lock this is a slow path anyway, so I
> didn't care much.

If we don't need to care it, the problem becomes easy to solve. But IMHO,
it'd be better to care it as original lockdep code did, because
save_trace() might have bigger overhead than we expect and
check_prevs_add() can be called frequently, so it'd be better to avoid it
when possible.

> Then again, I forgot to clean up in a bunch of paths.
> 
> > Frankly speaking, I think what I proposed resolved it neatly. Don't you
> > think so?
> 
> My initial reaction was to your patches being radically different to
> what I had proposed. But after fixing mine I don't particularly like
> either one of them.
> 
> Also, I think yours has a hole in, you check nr_stack_trace_entries
> against an older copy to check we did save_stack(), this is not accurate
> as check_prev_add() can drop graph_lock in the verbose case and then
> someone else could have done save_stack().

Right. My mistake..

Then.. The following patch on top of my patch 2/2 can solve it. Right?

---

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 49b9386..0f5bded 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1892,7 +1892,7 @@ static inline void inc_chains(void)
if (entry->class == hlock_class(next)) {
if (distance == 1)
entry->distance = 1;
-   return 2;
+   return 1;
}
}
 
@@ -1927,9 +1927,10 @@ static inline void inc_chains(void)
print_lock_name(hlock_class(next));
printk(KERN_CONT "\n");
dump_stack();
-   return graph_lock();
+   if (!graph_lock())
+   return 0;
}
-   return 1;
+   return 2;
 }
 
 /*
@@ -1975,15 +1976,16 @@ static inline void inc_chains(void)
 * added:
 */
if (hlock->read != 2 && hlock->check) {
-   if (!check_prev_add(curr, hlock, next,
-   distance, , save))
+   int ret = check_prev_add(curr, hlock, next,
+   distance, , save);
+   if (!ret)
return 0;
 
/*
 * Stop saving stack_trace if save_trace() was
 * called at least once:
 */
-   if (save && start_nr != nr_stack_trace_entries)
+   if (save && ret == 2)
save = NULL;
 
/*

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Wed, Jan 18, 2017 at 04:10:53PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 11:04:32AM +0900, Byungchul Park wrote:
> > On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> > > On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > > > What do you think about the following patches doing it?
> > > 
> > > I was more thinking about something like so...
> > > 
> > > Also, I think I want to muck with struct stack_trace; the members:
> > > max_nr_entries and skip are input arguments to save_stack_trace() and
> > > bloat the structure for no reason.
> > 
> > With your approach, save_trace() must be called whenever check_prevs_add()
> > is called, which might be unnecessary.
> 
> True.. but since we hold the graph_lock this is a slow path anyway, so I
> didn't care much.

If we don't need to care it, the problem becomes easy to solve. But IMHO,
it'd be better to care it as original lockdep code did, because
save_trace() might have bigger overhead than we expect and
check_prevs_add() can be called frequently, so it'd be better to avoid it
when possible.

> Then again, I forgot to clean up in a bunch of paths.
> 
> > Frankly speaking, I think what I proposed resolved it neatly. Don't you
> > think so?
> 
> My initial reaction was to your patches being radically different to
> what I had proposed. But after fixing mine I don't particularly like
> either one of them.
> 
> Also, I think yours has a hole in, you check nr_stack_trace_entries
> against an older copy to check we did save_stack(), this is not accurate
> as check_prev_add() can drop graph_lock in the verbose case and then
> someone else could have done save_stack().

Right. My mistake..

Then.. The following patch on top of my patch 2/2 can solve it. Right?

---

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 49b9386..0f5bded 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1892,7 +1892,7 @@ static inline void inc_chains(void)
if (entry->class == hlock_class(next)) {
if (distance == 1)
entry->distance = 1;
-   return 2;
+   return 1;
}
}
 
@@ -1927,9 +1927,10 @@ static inline void inc_chains(void)
print_lock_name(hlock_class(next));
printk(KERN_CONT "\n");
dump_stack();
-   return graph_lock();
+   if (!graph_lock())
+   return 0;
}
-   return 1;
+   return 2;
 }
 
 /*
@@ -1975,15 +1976,16 @@ static inline void inc_chains(void)
 * added:
 */
if (hlock->read != 2 && hlock->check) {
-   if (!check_prev_add(curr, hlock, next,
-   distance, , save))
+   int ret = check_prev_add(curr, hlock, next,
+   distance, , save);
+   if (!ret)
return 0;
 
/*
 * Stop saving stack_trace if save_trace() was
 * called at least once:
 */
-   if (save && start_nr != nr_stack_trace_entries)
+   if (save && ret == 2)
save = NULL;
 
/*

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Wed, Jan 18, 2017 at 11:04:32AM +0900, Byungchul Park wrote:
> On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> > On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > > What do you think about the following patches doing it?
> > 
> > I was more thinking about something like so...
> > 
> > Also, I think I want to muck with struct stack_trace; the members:
> > max_nr_entries and skip are input arguments to save_stack_trace() and
> > bloat the structure for no reason.
> 
> With your approach, save_trace() must be called whenever check_prevs_add()
> is called, which might be unnecessary.

True.. but since we hold the graph_lock this is a slow path anyway, so I
didn't care much.

Then again, I forgot to clean up in a bunch of paths.

> Frankly speaking, I think what I proposed resolved it neatly. Don't you
> think so?

My initial reaction was to your patches being radically different to
what I had proposed. But after fixing mine I don't particularly like
either one of them.

Also, I think yours has a hole in, you check nr_stack_trace_entries
against an older copy to check we did save_stack(), this is not accurate
as check_prev_add() can drop graph_lock in the verbose case and then
someone else could have done save_stack().


Let me see if I can find something simpler..

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Wed, Jan 18, 2017 at 11:04:32AM +0900, Byungchul Park wrote:
> On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> > On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > > What do you think about the following patches doing it?
> > 
> > I was more thinking about something like so...
> > 
> > Also, I think I want to muck with struct stack_trace; the members:
> > max_nr_entries and skip are input arguments to save_stack_trace() and
> > bloat the structure for no reason.
> 
> With your approach, save_trace() must be called whenever check_prevs_add()
> is called, which might be unnecessary.

True.. but since we hold the graph_lock this is a slow path anyway, so I
didn't care much.

Then again, I forgot to clean up in a bunch of paths.

> Frankly speaking, I think what I proposed resolved it neatly. Don't you
> think so?

My initial reaction was to your patches being radically different to
what I had proposed. But after fixing mine I don't particularly like
either one of them.

Also, I think yours has a hole in, you check nr_stack_trace_entries
against an older copy to check we did save_stack(), this is not accurate
as check_prev_add() can drop graph_lock in the verbose case and then
someone else could have done save_stack().


Let me see if I can find something simpler..

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > What do you think about the following patches doing it?
> 
> I was more thinking about something like so...
> 
> Also, I think I want to muck with struct stack_trace; the members:
> max_nr_entries and skip are input arguments to save_stack_trace() and
> bloat the structure for no reason.

With your approach, save_trace() must be called whenever check_prevs_add()
is called, which might be unnecessary.

Frankly speaking, I think what I proposed resolved it neatly. Don't you
think so?

> 
> ---
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 7c38f8f3d97b..f2df300a96ee 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -430,6 +430,21 @@ static int save_trace(struct stack_trace *trace)
>   return 1;
>  }
>  
> +static bool return_trace(struct stack_trace *trace)
> +{
> + /*
> +  * If @trace is the last trace generated by save_trace(), then we can
> +  * return the entries by simply subtracting @nr_stack_trace_entries
> +  * again.
> +  */
> + if (trace->entries != stack_trace + nr_stack_trace_entries - 
> trace->nr_entres)
> + return false;
> +
> + nr_stack_trace_entries -= trace->nr_entries;
> + trace->entries = NULL;
> + return true;
> +}
> +
>  unsigned int nr_hardirq_chains;
>  unsigned int nr_softirq_chains;
>  unsigned int nr_process_chains;
> @@ -1797,20 +1812,12 @@ static inline void inc_chains(void)
>   */
>  static int
>  check_prev_add(struct task_struct *curr, struct held_lock *prev,
> -struct held_lock *next, int distance, int *stack_saved)
> +struct held_lock *next, int distance, struct stack_trace *trace)
>  {
>   struct lock_list *entry;
>   int ret;
>   struct lock_list this;
>   struct lock_list *uninitialized_var(target_entry);
> - /*
> -  * Static variable, serialized by the graph_lock().
> -  *
> -  * We use this static variable to save the stack trace in case
> -  * we call into this function multiple times due to encountering
> -  * trylocks in the held lock stack.
> -  */
> - static struct stack_trace trace;
>  
>   /*
>* Prove that the new  ->  dependency would not
> @@ -1858,11 +1865,7 @@ static inline void inc_chains(void)
>   }
>   }
>  
> - if (!*stack_saved) {
> - if (!save_trace())
> - return 0;
> - *stack_saved = 1;
> - }
> + trace->skip = 1; /* mark used */
>  
>   /*
>* Ok, all validations passed, add the new lock
> @@ -1870,14 +1873,14 @@ static inline void inc_chains(void)
>*/
>   ret = add_lock_to_list(hlock_class(next),
>  _class(prev)->locks_after,
> -next->acquire_ip, distance, );
> +next->acquire_ip, distance, trace);
>  
>   if (!ret)
>   return 0;
>  
>   ret = add_lock_to_list(hlock_class(prev),
>  _class(next)->locks_before,
> -next->acquire_ip, distance, );
> +next->acquire_ip, distance, trace);
>   if (!ret)
>   return 0;
>  
> @@ -1885,8 +1888,6 @@ static inline void inc_chains(void)
>* Debugging printouts:
>*/
>   if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
> - /* We drop graph lock, so another thread can overwrite trace. */
> - *stack_saved = 0;
>   graph_unlock();
>   printk("\n new dependency: ");
>   print_lock_name(hlock_class(prev));
> @@ -1908,10 +1909,15 @@ static inline void inc_chains(void)
>  static int
>  check_prevs_add(struct task_struct *curr, struct held_lock *next)
>  {
> + struct stack_trace trace = { .nr_entries = 0, .skip = 0, };
>   int depth = curr->lockdep_depth;
> - int stack_saved = 0;
>   struct held_lock *hlock;
>  
> + if (!save_trace())
> + goto out_bug;
> +
> + trace.skip = 0; /* abuse to mark usage */
> +
>   /*
>* Debugging checks.
>*
> @@ -1936,7 +1942,7 @@ static inline void inc_chains(void)
>*/
>   if (hlock->read != 2 && hlock->check) {
>   if (!check_prev_add(curr, hlock, next,
> - distance, _saved))
> + distance, ))
>   return 0;
>   /*
>* Stop after the first non-trylock entry,
> @@ -1962,6 +1968,9 @@ static inline void inc_chains(void)
>   }
>   return 1;
>  out_bug:
> + if (trace.nr_entries && !trace.skip)
> + return_trace();
> +
>   if (!debug_locks_off_graph_unlock())
>   return 0;
>  

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > What do you think about the following patches doing it?
> 
> I was more thinking about something like so...
> 
> Also, I think I want to muck with struct stack_trace; the members:
> max_nr_entries and skip are input arguments to save_stack_trace() and
> bloat the structure for no reason.

With your approach, save_trace() must be called whenever check_prevs_add()
is called, which might be unnecessary.

Frankly speaking, I think what I proposed resolved it neatly. Don't you
think so?

> 
> ---
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 7c38f8f3d97b..f2df300a96ee 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -430,6 +430,21 @@ static int save_trace(struct stack_trace *trace)
>   return 1;
>  }
>  
> +static bool return_trace(struct stack_trace *trace)
> +{
> + /*
> +  * If @trace is the last trace generated by save_trace(), then we can
> +  * return the entries by simply subtracting @nr_stack_trace_entries
> +  * again.
> +  */
> + if (trace->entries != stack_trace + nr_stack_trace_entries - 
> trace->nr_entres)
> + return false;
> +
> + nr_stack_trace_entries -= trace->nr_entries;
> + trace->entries = NULL;
> + return true;
> +}
> +
>  unsigned int nr_hardirq_chains;
>  unsigned int nr_softirq_chains;
>  unsigned int nr_process_chains;
> @@ -1797,20 +1812,12 @@ static inline void inc_chains(void)
>   */
>  static int
>  check_prev_add(struct task_struct *curr, struct held_lock *prev,
> -struct held_lock *next, int distance, int *stack_saved)
> +struct held_lock *next, int distance, struct stack_trace *trace)
>  {
>   struct lock_list *entry;
>   int ret;
>   struct lock_list this;
>   struct lock_list *uninitialized_var(target_entry);
> - /*
> -  * Static variable, serialized by the graph_lock().
> -  *
> -  * We use this static variable to save the stack trace in case
> -  * we call into this function multiple times due to encountering
> -  * trylocks in the held lock stack.
> -  */
> - static struct stack_trace trace;
>  
>   /*
>* Prove that the new  ->  dependency would not
> @@ -1858,11 +1865,7 @@ static inline void inc_chains(void)
>   }
>   }
>  
> - if (!*stack_saved) {
> - if (!save_trace())
> - return 0;
> - *stack_saved = 1;
> - }
> + trace->skip = 1; /* mark used */
>  
>   /*
>* Ok, all validations passed, add the new lock
> @@ -1870,14 +1873,14 @@ static inline void inc_chains(void)
>*/
>   ret = add_lock_to_list(hlock_class(next),
>  _class(prev)->locks_after,
> -next->acquire_ip, distance, );
> +next->acquire_ip, distance, trace);
>  
>   if (!ret)
>   return 0;
>  
>   ret = add_lock_to_list(hlock_class(prev),
>  _class(next)->locks_before,
> -next->acquire_ip, distance, );
> +next->acquire_ip, distance, trace);
>   if (!ret)
>   return 0;
>  
> @@ -1885,8 +1888,6 @@ static inline void inc_chains(void)
>* Debugging printouts:
>*/
>   if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
> - /* We drop graph lock, so another thread can overwrite trace. */
> - *stack_saved = 0;
>   graph_unlock();
>   printk("\n new dependency: ");
>   print_lock_name(hlock_class(prev));
> @@ -1908,10 +1909,15 @@ static inline void inc_chains(void)
>  static int
>  check_prevs_add(struct task_struct *curr, struct held_lock *next)
>  {
> + struct stack_trace trace = { .nr_entries = 0, .skip = 0, };
>   int depth = curr->lockdep_depth;
> - int stack_saved = 0;
>   struct held_lock *hlock;
>  
> + if (!save_trace())
> + goto out_bug;
> +
> + trace.skip = 0; /* abuse to mark usage */
> +
>   /*
>* Debugging checks.
>*
> @@ -1936,7 +1942,7 @@ static inline void inc_chains(void)
>*/
>   if (hlock->read != 2 && hlock->check) {
>   if (!check_prev_add(curr, hlock, next,
> - distance, _saved))
> + distance, ))
>   return 0;
>   /*
>* Stop after the first non-trylock entry,
> @@ -1962,6 +1968,9 @@ static inline void inc_chains(void)
>   }
>   return 1;
>  out_bug:
> + if (trace.nr_entries && !trace.skip)
> + return_trace();
> +
>   if (!debug_locks_off_graph_unlock())
>   return 0;
>  

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> What do you think about the following patches doing it?

I was more thinking about something like so...

Also, I think I want to muck with struct stack_trace; the members:
max_nr_entries and skip are input arguments to save_stack_trace() and
bloat the structure for no reason.

---
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 7c38f8f3d97b..f2df300a96ee 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -430,6 +430,21 @@ static int save_trace(struct stack_trace *trace)
return 1;
 }
 
+static bool return_trace(struct stack_trace *trace)
+{
+   /*
+* If @trace is the last trace generated by save_trace(), then we can
+* return the entries by simply subtracting @nr_stack_trace_entries
+* again.
+*/
+   if (trace->entries != stack_trace + nr_stack_trace_entries - 
trace->nr_entres)
+   return false;
+
+   nr_stack_trace_entries -= trace->nr_entries;
+   trace->entries = NULL;
+   return true;
+}
+
 unsigned int nr_hardirq_chains;
 unsigned int nr_softirq_chains;
 unsigned int nr_process_chains;
@@ -1797,20 +1812,12 @@ static inline void inc_chains(void)
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-  struct held_lock *next, int distance, int *stack_saved)
+  struct held_lock *next, int distance, struct stack_trace *trace)
 {
struct lock_list *entry;
int ret;
struct lock_list this;
struct lock_list *uninitialized_var(target_entry);
-   /*
-* Static variable, serialized by the graph_lock().
-*
-* We use this static variable to save the stack trace in case
-* we call into this function multiple times due to encountering
-* trylocks in the held lock stack.
-*/
-   static struct stack_trace trace;
 
/*
 * Prove that the new  ->  dependency would not
@@ -1858,11 +1865,7 @@ static inline void inc_chains(void)
}
}
 
-   if (!*stack_saved) {
-   if (!save_trace())
-   return 0;
-   *stack_saved = 1;
-   }
+   trace->skip = 1; /* mark used */
 
/*
 * Ok, all validations passed, add the new lock
@@ -1870,14 +1873,14 @@ static inline void inc_chains(void)
 */
ret = add_lock_to_list(hlock_class(next),
   _class(prev)->locks_after,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, trace);
 
if (!ret)
return 0;
 
ret = add_lock_to_list(hlock_class(prev),
   _class(next)->locks_before,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, trace);
if (!ret)
return 0;
 
@@ -1885,8 +1888,6 @@ static inline void inc_chains(void)
 * Debugging printouts:
 */
if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
-   /* We drop graph lock, so another thread can overwrite trace. */
-   *stack_saved = 0;
graph_unlock();
printk("\n new dependency: ");
print_lock_name(hlock_class(prev));
@@ -1908,10 +1909,15 @@ static inline void inc_chains(void)
 static int
 check_prevs_add(struct task_struct *curr, struct held_lock *next)
 {
+   struct stack_trace trace = { .nr_entries = 0, .skip = 0, };
int depth = curr->lockdep_depth;
-   int stack_saved = 0;
struct held_lock *hlock;
 
+   if (!save_trace())
+   goto out_bug;
+
+   trace.skip = 0; /* abuse to mark usage */
+
/*
 * Debugging checks.
 *
@@ -1936,7 +1942,7 @@ static inline void inc_chains(void)
 */
if (hlock->read != 2 && hlock->check) {
if (!check_prev_add(curr, hlock, next,
-   distance, _saved))
+   distance, ))
return 0;
/*
 * Stop after the first non-trylock entry,
@@ -1962,6 +1968,9 @@ static inline void inc_chains(void)
}
return 1;
 out_bug:
+   if (trace.nr_entries && !trace.skip)
+   return_trace();
+
if (!debug_locks_off_graph_unlock())
return 0;
 

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> What do you think about the following patches doing it?

I was more thinking about something like so...

Also, I think I want to muck with struct stack_trace; the members:
max_nr_entries and skip are input arguments to save_stack_trace() and
bloat the structure for no reason.

---
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 7c38f8f3d97b..f2df300a96ee 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -430,6 +430,21 @@ static int save_trace(struct stack_trace *trace)
return 1;
 }
 
+static bool return_trace(struct stack_trace *trace)
+{
+   /*
+* If @trace is the last trace generated by save_trace(), then we can
+* return the entries by simply subtracting @nr_stack_trace_entries
+* again.
+*/
+   if (trace->entries != stack_trace + nr_stack_trace_entries - 
trace->nr_entres)
+   return false;
+
+   nr_stack_trace_entries -= trace->nr_entries;
+   trace->entries = NULL;
+   return true;
+}
+
 unsigned int nr_hardirq_chains;
 unsigned int nr_softirq_chains;
 unsigned int nr_process_chains;
@@ -1797,20 +1812,12 @@ static inline void inc_chains(void)
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-  struct held_lock *next, int distance, int *stack_saved)
+  struct held_lock *next, int distance, struct stack_trace *trace)
 {
struct lock_list *entry;
int ret;
struct lock_list this;
struct lock_list *uninitialized_var(target_entry);
-   /*
-* Static variable, serialized by the graph_lock().
-*
-* We use this static variable to save the stack trace in case
-* we call into this function multiple times due to encountering
-* trylocks in the held lock stack.
-*/
-   static struct stack_trace trace;
 
/*
 * Prove that the new  ->  dependency would not
@@ -1858,11 +1865,7 @@ static inline void inc_chains(void)
}
}
 
-   if (!*stack_saved) {
-   if (!save_trace())
-   return 0;
-   *stack_saved = 1;
-   }
+   trace->skip = 1; /* mark used */
 
/*
 * Ok, all validations passed, add the new lock
@@ -1870,14 +1873,14 @@ static inline void inc_chains(void)
 */
ret = add_lock_to_list(hlock_class(next),
   _class(prev)->locks_after,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, trace);
 
if (!ret)
return 0;
 
ret = add_lock_to_list(hlock_class(prev),
   _class(next)->locks_before,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, trace);
if (!ret)
return 0;
 
@@ -1885,8 +1888,6 @@ static inline void inc_chains(void)
 * Debugging printouts:
 */
if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
-   /* We drop graph lock, so another thread can overwrite trace. */
-   *stack_saved = 0;
graph_unlock();
printk("\n new dependency: ");
print_lock_name(hlock_class(prev));
@@ -1908,10 +1909,15 @@ static inline void inc_chains(void)
 static int
 check_prevs_add(struct task_struct *curr, struct held_lock *next)
 {
+   struct stack_trace trace = { .nr_entries = 0, .skip = 0, };
int depth = curr->lockdep_depth;
-   int stack_saved = 0;
struct held_lock *hlock;
 
+   if (!save_trace())
+   goto out_bug;
+
+   trace.skip = 0; /* abuse to mark usage */
+
/*
 * Debugging checks.
 *
@@ -1936,7 +1942,7 @@ static inline void inc_chains(void)
 */
if (hlock->read != 2 && hlock->check) {
if (!check_prev_add(curr, hlock, next,
-   distance, _saved))
+   distance, ))
return 0;
/*
 * Stop after the first non-trylock entry,
@@ -1962,6 +1968,9 @@ static inline void inc_chains(void)
}
return 1;
 out_bug:
+   if (trace.nr_entries && !trace.skip)
+   return_trace();
+
if (!debug_locks_off_graph_unlock())
return 0;
 

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

What do you think about the following patches doing it?

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

What do you think about the following patches doing it?

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Thu, Jan 12, 2017 at 05:16:43PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 09, 2016 at 02:12:01PM +0900, Byungchul Park wrote:
> > check_prev_add() saves a stack trace of the current. But crossrelease
> > feature needs to use a separate stack trace of another context in
> > check_prev_add(). So make it use a separate stack trace instead of one
> > of the current.
> > 
> 
> So I was thinking, can't we make check_prevs_add() create the stack
> trace unconditionally but record if we used it or not, and then return
> the entries when unused. All that is serialized by graph_lock anyway and
> that way we already pass a stack into check_prev_add() so we can easily
> pass in a different one.
> 
> I think that removes a bunch of tricky and avoids all the new tricky.

Looks very good.

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

On Thu, Jan 12, 2017 at 05:16:43PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 09, 2016 at 02:12:01PM +0900, Byungchul Park wrote:
> > check_prev_add() saves a stack trace of the current. But crossrelease
> > feature needs to use a separate stack trace of another context in
> > check_prev_add(). So make it use a separate stack trace instead of one
> > of the current.
> > 
> 
> So I was thinking, can't we make check_prevs_add() create the stack
> trace unconditionally but record if we used it or not, and then return
> the entries when unused. All that is serialized by graph_lock anyway and
> that way we already pass a stack into check_prev_add() so we can easily
> pass in a different one.
> 
> I think that removes a bunch of tricky and avoids all the new tricky.

Looks very good.

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Fri, Dec 09, 2016 at 02:12:01PM +0900, Byungchul Park wrote:
> check_prev_add() saves a stack trace of the current. But crossrelease
> feature needs to use a separate stack trace of another context in
> check_prev_add(). So make it use a separate stack trace instead of one
> of the current.
> 

So I was thinking, can't we make check_prevs_add() create the stack
trace unconditionally but record if we used it or not, and then return
the entries when unused. All that is serialized by graph_lock anyway and
that way we already pass a stack into check_prev_add() so we can easily
pass in a different one.

I think that removes a bunch of tricky and avoids all the new tricky.

Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Peter Zijlstra]

On Fri, Dec 09, 2016 at 02:12:01PM +0900, Byungchul Park wrote:
> check_prev_add() saves a stack trace of the current. But crossrelease
> feature needs to use a separate stack trace of another context in
> check_prev_add(). So make it use a separate stack trace instead of one
> of the current.
> 

So I was thinking, can't we make check_prevs_add() create the stack
trace unconditionally but record if we used it or not, and then return
the entries when unused. All that is serialized by graph_lock anyway and
that way we already pass a stack into check_prev_add() so we can easily
pass in a different one.

I think that removes a bunch of tricky and avoids all the new tricky.

[PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

check_prev_add() saves a stack trace of the current. But crossrelease
feature needs to use a separate stack trace of another context in
check_prev_add(). So make it use a separate stack trace instead of one
of the current.

Signed-off-by: Byungchul Park 
---
 kernel/locking/lockdep.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 111839f..3eaa11c 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1793,7 +1793,8 @@ check_deadlock(struct task_struct *curr, struct held_lock 
*next,
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-  struct held_lock *next, int distance, int *stack_saved)
+  struct held_lock *next, int distance, int *stack_saved,
+  struct stack_trace *own_trace)
 {
struct lock_list *entry;
int ret;
@@ -1854,7 +1855,7 @@ check_prev_add(struct task_struct *curr, struct held_lock 
*prev,
}
}
 
-   if (!*stack_saved) {
+   if (!own_trace && stack_saved && !*stack_saved) {
if (!save_trace())
return 0;
*stack_saved = 1;
@@ -1866,14 +1867,14 @@ check_prev_add(struct task_struct *curr, struct 
held_lock *prev,
 */
ret = add_lock_to_list(hlock_class(prev), hlock_class(next),
   _class(prev)->locks_after,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, own_trace ?: );
 
if (!ret)
return 0;
 
ret = add_lock_to_list(hlock_class(next), hlock_class(prev),
   _class(next)->locks_before,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, own_trace ?: );
if (!ret)
return 0;
 
@@ -1882,7 +1883,8 @@ check_prev_add(struct task_struct *curr, struct held_lock 
*prev,
 */
if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
/* We drop graph lock, so another thread can overwrite trace. */
-   *stack_saved = 0;
+   if (stack_saved)
+   *stack_saved = 0;
graph_unlock();
printk("\n new dependency: ");
print_lock_name(hlock_class(prev));
@@ -1931,8 +1933,8 @@ check_prevs_add(struct task_struct *curr, struct 
held_lock *next)
 * added:
 */
if (hlock->read != 2 && hlock->check) {
-   if (!check_prev_add(curr, hlock, next,
-   distance, _saved))
+   if (!check_prev_add(curr, hlock, next, distance,
+   _saved, NULL))
return 0;
/*
 * Stop after the first non-trylock entry,
-- 
1.9.1

[PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack_trace

[Byungchul Park]

check_prev_add() saves a stack trace of the current. But crossrelease
feature needs to use a separate stack trace of another context in
check_prev_add(). So make it use a separate stack trace instead of one
of the current.

Signed-off-by: Byungchul Park 
---
 kernel/locking/lockdep.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 111839f..3eaa11c 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1793,7 +1793,8 @@ check_deadlock(struct task_struct *curr, struct held_lock 
*next,
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-  struct held_lock *next, int distance, int *stack_saved)
+  struct held_lock *next, int distance, int *stack_saved,
+  struct stack_trace *own_trace)
 {
struct lock_list *entry;
int ret;
@@ -1854,7 +1855,7 @@ check_prev_add(struct task_struct *curr, struct held_lock 
*prev,
}
}
 
-   if (!*stack_saved) {
+   if (!own_trace && stack_saved && !*stack_saved) {
if (!save_trace())
return 0;
*stack_saved = 1;
@@ -1866,14 +1867,14 @@ check_prev_add(struct task_struct *curr, struct 
held_lock *prev,
 */
ret = add_lock_to_list(hlock_class(prev), hlock_class(next),
   _class(prev)->locks_after,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, own_trace ?: );
 
if (!ret)
return 0;
 
ret = add_lock_to_list(hlock_class(next), hlock_class(prev),
   _class(next)->locks_before,
-  next->acquire_ip, distance, );
+  next->acquire_ip, distance, own_trace ?: );
if (!ret)
return 0;
 
@@ -1882,7 +1883,8 @@ check_prev_add(struct task_struct *curr, struct held_lock 
*prev,
 */
if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
/* We drop graph lock, so another thread can overwrite trace. */
-   *stack_saved = 0;
+   if (stack_saved)
+   *stack_saved = 0;
graph_unlock();
printk("\n new dependency: ");
print_lock_name(hlock_class(prev));
@@ -1931,8 +1933,8 @@ check_prevs_add(struct task_struct *curr, struct 
held_lock *next)
 * added:
 */
if (hlock->read != 2 && hlock->check) {
-   if (!check_prev_add(curr, hlock, next,
-   distance, _saved))
+   if (!check_prev_add(curr, hlock, next, distance,
+   _saved, NULL))
return 0;
/*
 * Stop after the first non-trylock entry,
-- 
1.9.1