[PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

Johan Hovold reported a performance regression by crossrelease like:

> Boot time (from "Linux version" to login prompt) had in fact doubled
> since 4.13 where it took 17 seconds (with my current config) compared to
> the 35 seconds I now see with 4.14-rc4.
>
> I quick bisect pointed to lockdep and specifically the following commit:
>
> 	28a903f63ec0 ("locking/lockdep: Handle non(or multi)-acquisition
> 	               of a crosslock")
>
> which I've verified is the commit which doubled the boot time (compared
> to 28a903f63ec0^) (added by lockdep crossrelease series [1]).

Currently crossrelease performs unwind on every acquisition. But, that
overloads systems too much. So this patch makes unwind optional and set
it to N as default. Instead, it records only acquire_ip normally. Of
course, unwind is sometimes required for full analysis. In that case, we
can set CROSSRELEASE_STACK_TRACE to Y and use it.

In my qemu ubuntu machin (x86_64, 4 cores, 512M), the regression was
fixed like, measuring timestamp of "Freeing unused kernel memory":

1. No lockdep enabled
   Average : 1.543353 secs

2. Lockdep enabled
   Average : 1.570806 secs

3. Lockdep enabled + crossrelease enabled
   Average : 1.870317 secs

4. Lockdep enabled + crossrelease enabled + this patch applied
   Average : 1.574143 secs

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/lockdep.h  |  4 ++++
 kernel/locking/lockdep.c |  5 +++++
 lib/Kconfig.debug        | 15 +++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index bfa8e0b..70358b5 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -278,7 +278,11 @@ struct held_lock {
 };
 
 #ifdef CONFIG_LOCKDEP_CROSSRELEASE
+#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
 #define MAX_XHLOCK_TRACE_ENTRIES 5
+#else
+#define MAX_XHLOCK_TRACE_ENTRIES 1
+#endif
 
 /*
  * This is for keeping locks waiting for commit so that true dependencies
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e36e652..5c2ddf2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
 	xhlock->trace.nr_entries = 0;
 	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
 	xhlock->trace.entries = xhlock->trace_entries;
+#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
 	xhlock->trace.skip = 3;
 	save_stack_trace(&xhlock->trace);
+#else
+	xhlock->trace.nr_entries = 1;
+	xhlock->trace.entries[0] = hlock->acquire_ip;
+#endif
 }
 
 static inline int same_context_xhlock(struct hist_lock *xhlock)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 3db9167..5be7bdd 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1225,6 +1225,21 @@ config LOCKDEP_COMPLETIONS
 	 A deadlock caused by wait_for_completion() and complete() can be
 	 detected by lockdep using crossrelease feature.
 
+config CROSSRELEASE_STACK_TRACE
+	bool "Record more than one entity of stack trace in crossrelease"
+	depends on LOCKDEP_CROSSRELEASE
+	default n
+	help
+	 Crossrelease feature needs to record stack traces for all
+	 acquisitions for later use. And only acquire_ip is normally
+	 recorded because the unwind operation is too expensive. However,
+	 sometimes more than acquire_ip are required for full analysis.
+	 In the case that we need to record more than one entity of
+	 stack trace using unwind, this feature would be useful, with
+	 taking more overhead.
+
+	 If unsure, say N.
+
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
 	depends on DEBUG_KERNEL && LOCKDEP
-- 
1.9.1

[PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

Johan Hovold reported a performance regression by crossrelease like:

> Boot time (from "Linux version" to login prompt) had in fact doubled
> since 4.13 where it took 17 seconds (with my current config) compared to
> the 35 seconds I now see with 4.14-rc4.
>
> I quick bisect pointed to lockdep and specifically the following commit:
>
> 	28a903f63ec0 ("locking/lockdep: Handle non(or multi)-acquisition
> 	               of a crosslock")
>
> which I've verified is the commit which doubled the boot time (compared
> to 28a903f63ec0^) (added by lockdep crossrelease series [1]).

Currently crossrelease performs unwind on every acquisition. But, that
overloads systems too much. So this patch makes unwind optional and set
it to N as default. Instead, it records only acquire_ip normally. Of
course, unwind is sometimes required for full analysis. In that case, we
can set CROSSRELEASE_STACK_TRACE to Y and use it.

In my qemu ubuntu machin (x86_64, 4 cores, 512M), the regression was
fixed like, measuring timestamp of "Freeing unused kernel memory":

1. No lockdep enabled
   Average : 1.543353 secs

2. Lockdep enabled
   Average : 1.570806 secs

3. Lockdep enabled + crossrelease enabled
   Average : 1.870317 secs

4. Lockdep enabled + crossrelease enabled + this patch applied
   Average : 1.574143 secs

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/lockdep.h  |  4 ++++
 kernel/locking/lockdep.c |  5 +++++
 lib/Kconfig.debug        | 15 +++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index bfa8e0b..70358b5 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -278,7 +278,11 @@ struct held_lock {
 };
 
 #ifdef CONFIG_LOCKDEP_CROSSRELEASE
+#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
 #define MAX_XHLOCK_TRACE_ENTRIES 5
+#else
+#define MAX_XHLOCK_TRACE_ENTRIES 1
+#endif
 
 /*
  * This is for keeping locks waiting for commit so that true dependencies
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e36e652..5c2ddf2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
 	xhlock->trace.nr_entries = 0;
 	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
 	xhlock->trace.entries = xhlock->trace_entries;
+#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
 	xhlock->trace.skip = 3;
 	save_stack_trace(&xhlock->trace);
+#else
+	xhlock->trace.nr_entries = 1;
+	xhlock->trace.entries[0] = hlock->acquire_ip;
+#endif
 }
 
 static inline int same_context_xhlock(struct hist_lock *xhlock)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 3db9167..5be7bdd 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1225,6 +1225,21 @@ config LOCKDEP_COMPLETIONS
 	 A deadlock caused by wait_for_completion() and complete() can be
 	 detected by lockdep using crossrelease feature.
 
+config CROSSRELEASE_STACK_TRACE
+	bool "Record more than one entity of stack trace in crossrelease"
+	depends on LOCKDEP_CROSSRELEASE
+	default n
+	help
+	 Crossrelease feature needs to record stack traces for all
+	 acquisitions for later use. And only acquire_ip is normally
+	 recorded because the unwind operation is too expensive. However,
+	 sometimes more than acquire_ip are required for full analysis.
+	 In the case that we need to record more than one entity of
+	 stack trace using unwind, this feature would be useful, with
+	 taking more overhead.
+
+	 If unsure, say N.
+
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
 	depends on DEBUG_KERNEL && LOCKDEP
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
and LOCKDEP_COMPLETIONS.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 lib/Kconfig.debug | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5be7bdd..c17cb5d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1138,8 +1138,8 @@ config PROVE_LOCKING
 	select DEBUG_MUTEXES
 	select DEBUG_RT_MUTEXES if RT_MUTEXES
 	select DEBUG_LOCK_ALLOC
-	select LOCKDEP_CROSSRELEASE if BROKEN
-	select LOCKDEP_COMPLETIONS if BROKEN
+	select LOCKDEP_CROSSRELEASE
+	select LOCKDEP_COMPLETIONS
 	select TRACE_IRQFLAGS
 	default n
 	help
-- 
1.9.1

[PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
and LOCKDEP_COMPLETIONS.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 lib/Kconfig.debug | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5be7bdd..c17cb5d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1138,8 +1138,8 @@ config PROVE_LOCKING
 	select DEBUG_MUTEXES
 	select DEBUG_RT_MUTEXES if RT_MUTEXES
 	select DEBUG_LOCK_ALLOC
-	select LOCKDEP_CROSSRELEASE if BROKEN
-	select LOCKDEP_COMPLETIONS if BROKEN
+	select LOCKDEP_CROSSRELEASE
+	select LOCKDEP_COMPLETIONS
 	select TRACE_IRQFLAGS
 	default n
 	help
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> Johan Hovold reported a performance regression by crossrelease like:
> 
> > Boot time (from "Linux version" to login prompt) had in fact doubled
> > since 4.13 where it took 17 seconds (with my current config) compared to
> > the 35 seconds I now see with 4.14-rc4.
> >
> > I quick bisect pointed to lockdep and specifically the following commit:
> >
> > 	28a903f63ec0 ("locking/lockdep: Handle non(or multi)-acquisition
> > 	               of a crosslock")
> >
> > which I've verified is the commit which doubled the boot time (compared
> > to 28a903f63ec0^) (added by lockdep crossrelease series [1]).
> 
> Currently crossrelease performs unwind on every acquisition. But, that
> overloads systems too much. So this patch makes unwind optional and set
> it to N as default. Instead, it records only acquire_ip normally. Of
> course, unwind is sometimes required for full analysis. In that case, we
> can set CROSSRELEASE_STACK_TRACE to Y and use it.
> 
> In my qemu ubuntu machin (x86_64, 4 cores, 512M), the regression was
> fixed like, measuring timestamp of "Freeing unused kernel memory":
> 
> 1. No lockdep enabled
>    Average : 1.543353 secs
> 
> 2. Lockdep enabled
>    Average : 1.570806 secs
> 
> 3. Lockdep enabled + crossrelease enabled
>    Average : 1.870317 secs
> 
> 4. Lockdep enabled + crossrelease enabled + this patch applied
>    Average : 1.574143 secs

Ok, that looks really nice, recovers almost all of the lost performance, right?

Could you please run perf stat --null --repeat type of stats of a boot test (for 
example running init=/bin/true should boot up Qemu and make it exit), so that we 
can see how stable the numbers are and what the real slowdown is?

> +config CROSSRELEASE_STACK_TRACE
> +	bool "Record more than one entity of stack trace in crossrelease"
> +	depends on LOCKDEP_CROSSRELEASE
> +	default n
> +	help
> +	 Crossrelease feature needs to record stack traces for all
> +	 acquisitions for later use. And only acquire_ip is normally
> +	 recorded because the unwind operation is too expensive. However,
> +	 sometimes more than acquire_ip are required for full analysis.
> +	 In the case that we need to record more than one entity of
> +	 stack trace using unwind, this feature would be useful, with
> +	 taking more overhead.
> +
> +	 If unsure, say N.

Fixed the text for you:

> +	 The lockdep "cross-release" feature needs to record stack traces
> +	 (of calling functions) for all acquisitions, for eventual later use
> +	 during analysis.
> +	 By default only a single caller is recorded, because the unwind
> +	 operation can be very expensive with deeper stack chains.
> +	 However, sometimes deeper traces are required for full analysis.
> +	 This option turns on the saving of the full stack trace entries.
> +
> +	 If unsure, say N.

BTW., have you attempted limiting the depth of the stack traces? I suspect more 
than 2-4 are rarely required to disambiguate the calling context.

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> Johan Hovold reported a performance regression by crossrelease like:
> 
> > Boot time (from "Linux version" to login prompt) had in fact doubled
> > since 4.13 where it took 17 seconds (with my current config) compared to
> > the 35 seconds I now see with 4.14-rc4.
> >
> > I quick bisect pointed to lockdep and specifically the following commit:
> >
> > 	28a903f63ec0 ("locking/lockdep: Handle non(or multi)-acquisition
> > 	               of a crosslock")
> >
> > which I've verified is the commit which doubled the boot time (compared
> > to 28a903f63ec0^) (added by lockdep crossrelease series [1]).
> 
> Currently crossrelease performs unwind on every acquisition. But, that
> overloads systems too much. So this patch makes unwind optional and set
> it to N as default. Instead, it records only acquire_ip normally. Of
> course, unwind is sometimes required for full analysis. In that case, we
> can set CROSSRELEASE_STACK_TRACE to Y and use it.
> 
> In my qemu ubuntu machin (x86_64, 4 cores, 512M), the regression was
> fixed like, measuring timestamp of "Freeing unused kernel memory":
> 
> 1. No lockdep enabled
>    Average : 1.543353 secs
> 
> 2. Lockdep enabled
>    Average : 1.570806 secs
> 
> 3. Lockdep enabled + crossrelease enabled
>    Average : 1.870317 secs
> 
> 4. Lockdep enabled + crossrelease enabled + this patch applied
>    Average : 1.574143 secs

Ok, that looks really nice, recovers almost all of the lost performance, right?

Could you please run perf stat --null --repeat type of stats of a boot test (for 
example running init=/bin/true should boot up Qemu and make it exit), so that we 
can see how stable the numbers are and what the real slowdown is?

> +config CROSSRELEASE_STACK_TRACE
> +	bool "Record more than one entity of stack trace in crossrelease"
> +	depends on LOCKDEP_CROSSRELEASE
> +	default n
> +	help
> +	 Crossrelease feature needs to record stack traces for all
> +	 acquisitions for later use. And only acquire_ip is normally
> +	 recorded because the unwind operation is too expensive. However,
> +	 sometimes more than acquire_ip are required for full analysis.
> +	 In the case that we need to record more than one entity of
> +	 stack trace using unwind, this feature would be useful, with
> +	 taking more overhead.
> +
> +	 If unsure, say N.

Fixed the text for you:

> +	 The lockdep "cross-release" feature needs to record stack traces
> +	 (of calling functions) for all acquisitions, for eventual later use
> +	 during analysis.
> +	 By default only a single caller is recorded, because the unwind
> +	 operation can be very expensive with deeper stack chains.
> +	 However, sometimes deeper traces are required for full analysis.
> +	 This option turns on the saving of the full stack trace entries.
> +
> +	 If unsure, say N.

BTW., have you attempted limiting the depth of the stack traces? I suspect more 
than 2-4 are rarely required to disambiguate the calling context.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
> and LOCKDEP_COMPLETIONS.

Please write out CONFIG_ variables, i.e. CONFIG_LOCKDEP_CROSSRELEASE, etc. - to 
make it all more apparent to the reader that it's all Kconfig space changes.

Thanks,

	Ingo

Re: [PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
> and LOCKDEP_COMPLETIONS.

Please write out CONFIG_ variables, i.e. CONFIG_LOCKDEP_CROSSRELEASE, etc. - to 
make it all more apparent to the reader that it's all Kconfig space changes.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Byungchul Park wrote:
>  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
>  #define MAX_XHLOCK_TRACE_ENTRIES 5
> +#else
> +#define MAX_XHLOCK_TRACE_ENTRIES 1
> +#endif
>  
>  /*
>   * This is for keeping locks waiting for commit so that true dependencies
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index e36e652..5c2ddf2 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
>  	xhlock->trace.nr_entries = 0;
>  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
>  	xhlock->trace.entries = xhlock->trace_entries;
> +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
>  	xhlock->trace.skip = 3;
>  	save_stack_trace(&xhlock->trace);
> +#else
> +	xhlock->trace.nr_entries = 1;
> +	xhlock->trace.entries[0] = hlock->acquire_ip;
> +#endif

Hmm. Would it be possible to have this switchable at boot time via a
command line parameter? So in case of a splat with no stack trace, one
could just reboot and set something like 'lockdep_fullstack' on the kernel
command line to get the full data without having to recompile the kernel.

Thanks,

	tglx

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Byungchul Park wrote:
>  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
>  #define MAX_XHLOCK_TRACE_ENTRIES 5
> +#else
> +#define MAX_XHLOCK_TRACE_ENTRIES 1
> +#endif
>  
>  /*
>   * This is for keeping locks waiting for commit so that true dependencies
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index e36e652..5c2ddf2 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
>  	xhlock->trace.nr_entries = 0;
>  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
>  	xhlock->trace.entries = xhlock->trace_entries;
> +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
>  	xhlock->trace.skip = 3;
>  	save_stack_trace(&xhlock->trace);
> +#else
> +	xhlock->trace.nr_entries = 1;
> +	xhlock->trace.entries[0] = hlock->acquire_ip;
> +#endif

Hmm. Would it be possible to have this switchable at boot time via a
command line parameter? So in case of a splat with no stack trace, one
could just reboot and set something like 'lockdep_fullstack' on the kernel
command line to get the full data without having to recompile the kernel.

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 18 Oct 2017, Byungchul Park wrote:
> >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > +#else
> > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > +#endif
> >  
> >  /*
> >   * This is for keeping locks waiting for commit so that true dependencies
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index e36e652..5c2ddf2 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> >  	xhlock->trace.nr_entries = 0;
> >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> >  	xhlock->trace.entries = xhlock->trace_entries;
> > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> >  	xhlock->trace.skip = 3;
> >  	save_stack_trace(&xhlock->trace);
> > +#else
> > +	xhlock->trace.nr_entries = 1;
> > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > +#endif
> 
> Hmm. Would it be possible to have this switchable at boot time via a
> command line parameter? So in case of a splat with no stack trace, one
> could just reboot and set something like 'lockdep_fullstack' on the kernel
> command line to get the full data without having to recompile the kernel.

Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
option as well - i.e. let's have both.

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 18 Oct 2017, Byungchul Park wrote:
> >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > +#else
> > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > +#endif
> >  
> >  /*
> >   * This is for keeping locks waiting for commit so that true dependencies
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index e36e652..5c2ddf2 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> >  	xhlock->trace.nr_entries = 0;
> >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> >  	xhlock->trace.entries = xhlock->trace_entries;
> > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> >  	xhlock->trace.skip = 3;
> >  	save_stack_trace(&xhlock->trace);
> > +#else
> > +	xhlock->trace.nr_entries = 1;
> > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > +#endif
> 
> Hmm. Would it be possible to have this switchable at boot time via a
> command line parameter? So in case of a splat with no stack trace, one
> could just reboot and set something like 'lockdep_fullstack' on the kernel
> command line to get the full data without having to recompile the kernel.

Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
option as well - i.e. let's have both.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Wed, 18 Oct 2017, Byungchul Park wrote:
> > >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > > +#else
> > > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > > +#endif
> > >  
> > >  /*
> > >   * This is for keeping locks waiting for commit so that true dependencies
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index e36e652..5c2ddf2 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> > >  	xhlock->trace.nr_entries = 0;
> > >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> > >  	xhlock->trace.entries = xhlock->trace_entries;
> > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > >  	xhlock->trace.skip = 3;
> > >  	save_stack_trace(&xhlock->trace);
> > > +#else
> > > +	xhlock->trace.nr_entries = 1;
> > > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > > +#endif
> > 
> > Hmm. Would it be possible to have this switchable at boot time via a
> > command line parameter? So in case of a splat with no stack trace, one
> > could just reboot and set something like 'lockdep_fullstack' on the kernel
> > command line to get the full data without having to recompile the kernel.
> 
> Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
> option as well - i.e. let's have both.

That makes sense. Like we have with debug objects:
DEBUG_OBJECTS_ENABLE_DEFAULT.

Which reminds me that I wanted to convert them to static_key so they are
zero overhead when disabled. Sigh, why are todo lists growth only?

Thanks,

	tglx

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Wed, 18 Oct 2017, Byungchul Park wrote:
> > >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > > +#else
> > > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > > +#endif
> > >  
> > >  /*
> > >   * This is for keeping locks waiting for commit so that true dependencies
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index e36e652..5c2ddf2 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> > >  	xhlock->trace.nr_entries = 0;
> > >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> > >  	xhlock->trace.entries = xhlock->trace_entries;
> > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > >  	xhlock->trace.skip = 3;
> > >  	save_stack_trace(&xhlock->trace);
> > > +#else
> > > +	xhlock->trace.nr_entries = 1;
> > > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > > +#endif
> > 
> > Hmm. Would it be possible to have this switchable at boot time via a
> > command line parameter? So in case of a splat with no stack trace, one
> > could just reboot and set something like 'lockdep_fullstack' on the kernel
> > command line to get the full data without having to recompile the kernel.
> 
> Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
> option as well - i.e. let's have both.

That makes sense. Like we have with debug objects:
DEBUG_OBJECTS_ENABLE_DEFAULT.

Which reminds me that I wanted to convert them to static_key so they are
zero overhead when disabled. Sigh, why are todo lists growth only?

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Matthew Wilcox]

On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> Which reminds me that I wanted to convert them to static_key so they are
> zero overhead when disabled. Sigh, why are todo lists growth only?

This is why you need an Outreachy intern -- it gets at least one task
off your todo list, and in the best possible case, it gets a second
person working on your todo list for a long time.

... eventually they start their own todo lists ...

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Matthew Wilcox]

On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> Which reminds me that I wanted to convert them to static_key so they are
> zero overhead when disabled. Sigh, why are todo lists growth only?

This is why you need an Outreachy intern -- it gets at least one task
off your todo list, and in the best possible case, it gets a second
person working on your todo list for a long time.

... eventually they start their own todo lists ...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Matthew Wilcox wrote:

> On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> > Which reminds me that I wanted to convert them to static_key so they are
> > zero overhead when disabled. Sigh, why are todo lists growth only?
> 
> This is why you need an Outreachy intern -- it gets at least one task
> off your todo list, and in the best possible case, it gets a second
> person working on your todo list for a long time.
> 
> ... eventually they start their own todo lists ...

Good idea. Oh, wait.....  Getting an Outreachy intern is on my todo list already. 

Thanks,

	tglx

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Thomas Gleixner]

On Wed, 18 Oct 2017, Matthew Wilcox wrote:

> On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> > Which reminds me that I wanted to convert them to static_key so they are
> > zero overhead when disabled. Sigh, why are todo lists growth only?
> 
> This is why you need an Outreachy intern -- it gets at least one task
> off your todo list, and in the best possible case, it gets a second
> person working on your todo list for a long time.
> 
> ... eventually they start their own todo lists ...

Good idea. Oh, wait.....  Getting an Outreachy intern is on my todo list already. 

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 18 Oct 2017, Matthew Wilcox wrote:
> 
> > On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> > > Which reminds me that I wanted to convert them to static_key so they are
> > > zero overhead when disabled. Sigh, why are todo lists growth only?
> > 
> > This is why you need an Outreachy intern -- it gets at least one task
> > off your todo list, and in the best possible case, it gets a second
> > person working on your todo list for a long time.
> > 
> > ... eventually they start their own todo lists ...
> 
> Good idea. Oh, wait.....  Getting an Outreachy intern is on my todo list already. 

Please add "shrink my TODO list" to your TODO list ... wait a minute ...

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 18 Oct 2017, Matthew Wilcox wrote:
> 
> > On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> > > Which reminds me that I wanted to convert them to static_key so they are
> > > zero overhead when disabled. Sigh, why are todo lists growth only?
> > 
> > This is why you need an Outreachy intern -- it gets at least one task
> > off your todo list, and in the best possible case, it gets a second
> > person working on your todo list for a long time.
> > 
> > ... eventually they start their own todo lists ...
> 
> Good idea. Oh, wait.....  Getting an Outreachy intern is on my todo list already. 

Please add "shrink my TODO list" to your TODO list ... wait a minute ...

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

On Wed, Oct 18, 2017 at 12:12:08PM +0200, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
> > and LOCKDEP_COMPLETIONS.
> 
> Please write out CONFIG_ variables, i.e. CONFIG_LOCKDEP_CROSSRELEASE, etc. - to 
> make it all more apparent to the reader that it's all Kconfig space changes.

Yes, I will. Thank you.

> 
> Thanks,
> 
> 	Ingo

Re: [PATCH 2/2] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

On Wed, Oct 18, 2017 at 12:12:08PM +0200, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > Now the performance regression was fixed, re-enable LOCKDEP_CROSSRELEASE
> > and LOCKDEP_COMPLETIONS.
> 
> Please write out CONFIG_ variables, i.e. CONFIG_LOCKDEP_CROSSRELEASE, etc. - to 
> make it all more apparent to the reader that it's all Kconfig space changes.

Yes, I will. Thank you.

> 
> Thanks,
> 
> 	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> On Wed, 18 Oct 2017, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > On Wed, 18 Oct 2017, Byungchul Park wrote:
> > > >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > > >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > > > +#else
> > > > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > > > +#endif
> > > >  
> > > >  /*
> > > >   * This is for keeping locks waiting for commit so that true dependencies
> > > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > > index e36e652..5c2ddf2 100644
> > > > --- a/kernel/locking/lockdep.c
> > > > +++ b/kernel/locking/lockdep.c
> > > > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> > > >  	xhlock->trace.nr_entries = 0;
> > > >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> > > >  	xhlock->trace.entries = xhlock->trace_entries;
> > > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > > >  	xhlock->trace.skip = 3;
> > > >  	save_stack_trace(&xhlock->trace);
> > > > +#else
> > > > +	xhlock->trace.nr_entries = 1;
> > > > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > > > +#endif
> > > 
> > > Hmm. Would it be possible to have this switchable at boot time via a
> > > command line parameter? So in case of a splat with no stack trace, one
> > > could just reboot and set something like 'lockdep_fullstack' on the kernel
> > > command line to get the full data without having to recompile the kernel.
> > 
> > Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
> > option as well - i.e. let's have both.
> 
> That makes sense. Like we have with debug objects:
> DEBUG_OBJECTS_ENABLE_DEFAULT.

Thank you very much for the suggestion. I will work for it.

> Which reminds me that I wanted to convert them to static_key so they are
> zero overhead when disabled. Sigh, why are todo lists growth only?
> 
> Thanks,
> 
> 	tglx

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Wed, Oct 18, 2017 at 03:36:05PM +0200, Thomas Gleixner wrote:
> On Wed, 18 Oct 2017, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > On Wed, 18 Oct 2017, Byungchul Park wrote:
> > > >  #ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > > >  #define MAX_XHLOCK_TRACE_ENTRIES 5
> > > > +#else
> > > > +#define MAX_XHLOCK_TRACE_ENTRIES 1
> > > > +#endif
> > > >  
> > > >  /*
> > > >   * This is for keeping locks waiting for commit so that true dependencies
> > > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > > index e36e652..5c2ddf2 100644
> > > > --- a/kernel/locking/lockdep.c
> > > > +++ b/kernel/locking/lockdep.c
> > > > @@ -4863,8 +4863,13 @@ static void add_xhlock(struct held_lock *hlock)
> > > >  	xhlock->trace.nr_entries = 0;
> > > >  	xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> > > >  	xhlock->trace.entries = xhlock->trace_entries;
> > > > +#ifdef CONFIG_CROSSRELEASE_STACK_TRACE
> > > >  	xhlock->trace.skip = 3;
> > > >  	save_stack_trace(&xhlock->trace);
> > > > +#else
> > > > +	xhlock->trace.nr_entries = 1;
> > > > +	xhlock->trace.entries[0] = hlock->acquire_ip;
> > > > +#endif
> > > 
> > > Hmm. Would it be possible to have this switchable at boot time via a
> > > command line parameter? So in case of a splat with no stack trace, one
> > > could just reboot and set something like 'lockdep_fullstack' on the kernel
> > > command line to get the full data without having to recompile the kernel.
> > 
> > Yeah, and I'd suggest keeping the Kconfig option to default-enable that boot 
> > option as well - i.e. let's have both.
> 
> That makes sense. Like we have with debug objects:
> DEBUG_OBJECTS_ENABLE_DEFAULT.

Thank you very much for the suggestion. I will work for it.

> Which reminds me that I wanted to convert them to static_key so they are
> zero overhead when disabled. Sigh, why are todo lists growth only?
> 
> Thanks,
> 
> 	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> than 2-4 are rarely required to disambiguate the calling context.

I did it for you. Let me show you the result.

1. No lockdep

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.756558155 seconds time elapsed                    ( +-  0.09% )

2. Lockdep

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.968710420 seconds time elapsed                    ( +-  0.12% )

3. Lockdep + Crossrelease 5 entries

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       3.153839636 seconds time elapsed                    ( +-  0.31% )

4. Lockdep + Crossrelease 3 entries

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       3.137205534 seconds time elapsed                    ( +-  0.87% )

5. Lockdep + Crossrelease + This patch

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.963669551 seconds time elapsed                    ( +-  0.11% )

And I will add the result in commit message at the next spin.

Thanks,
Byungchul

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> than 2-4 are rarely required to disambiguate the calling context.

I did it for you. Let me show you the result.

1. No lockdep

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.756558155 seconds time elapsed                    ( +-  0.09% )

2. Lockdep

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.968710420 seconds time elapsed                    ( +-  0.12% )

3. Lockdep + Crossrelease 5 entries

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       3.153839636 seconds time elapsed                    ( +-  0.31% )

4. Lockdep + Crossrelease 3 entries

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       3.137205534 seconds time elapsed                    ( +-  0.87% )

5. Lockdep + Crossrelease + This patch

 Performance counter stats for 'qemu_booting_time.sh bzImage' (10 runs):

       2.963669551 seconds time elapsed                    ( +-  0.11% )

And I will add the result in commit message at the next spin.

Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > than 2-4 are rarely required to disambiguate the calling context.
> 
> I did it for you. Let me show you the result.
> 
> 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )

I think the lockdep + crossrelease + full-stack numbers are missing?

But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.

That's very reasonable and we can keep the single-entry cross-release feature 
enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
and false positives are fixed by the next merge window.

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > than 2-4 are rarely required to disambiguate the calling context.
> 
> I did it for you. Let me show you the result.
> 
> 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )

I think the lockdep + crossrelease + full-stack numbers are missing?

But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.

That's very reasonable and we can keep the single-entry cross-release feature 
enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
and false positives are fixed by the next merge window.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > than 2-4 are rarely required to disambiguate the calling context.
> > 
> > I did it for you. Let me show you the result.
> > 
> > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> 
> I think the lockdep + crossrelease + full-stack numbers are missing?

Ah, the last version of crossrelease merged into vanilla, records 5
entries, since I thought it overloads too much if full stack is used,
and 5 entries are enough. Don't you think so?

> But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> 
> That's very reasonable and we can keep the single-entry cross-release feature 
> enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 

BTW, is there any crash by cross-release I don't know? Of course, I know
cases of false positives, but I don't about crash.

Thanks,
Byungchul

> and false positives are fixed by the next merge window.
> 
> Thanks,
> 
> 	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > than 2-4 are rarely required to disambiguate the calling context.
> > 
> > I did it for you. Let me show you the result.
> > 
> > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> 
> I think the lockdep + crossrelease + full-stack numbers are missing?

Ah, the last version of crossrelease merged into vanilla, records 5
entries, since I thought it overloads too much if full stack is used,
and 5 entries are enough. Don't you think so?

> But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> 
> That's very reasonable and we can keep the single-entry cross-release feature 
> enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 

BTW, is there any crash by cross-release I don't know? Of course, I know
cases of false positives, but I don't about crash.

Thanks,
Byungchul

> and false positives are fixed by the next merge window.
> 
> Thanks,
> 
> 	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > 
> > * Byungchul Park <byungchul.park@lge.com> wrote:
> > 
> > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > than 2-4 are rarely required to disambiguate the calling context.
> > > 
> > > I did it for you. Let me show you the result.
> > > 
> > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > 
> > I think the lockdep + crossrelease + full-stack numbers are missing?
> 
> Ah, the last version of crossrelease merged into vanilla, records 5
> entries, since I thought it overloads too much if full stack is used,
> and 5 entries are enough. Don't you think so?

Ok, fair enough, I missed that limitation!

> > That's very reasonable and we can keep the single-entry cross-release feature 
> > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> 
> BTW, is there any crash by cross-release I don't know? Of course, I know
> cases of false positives, but I don't about crash.

There's no current crash regression that I know of - I'm just outlining the 
conditions of getting all this re-enabled in the next merge window.

Instead of sending two series, could you please send a series that includes both 
these fixing + re-enabling patches, plus the false positive fixes?

In particular I think the cross-release re-enabling should be done as the last 
patch, so that any future bisections of new false positives won't be made more 
difficult by re-introducing the old false positives near the end of the bisection.

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > 
> > * Byungchul Park <byungchul.park@lge.com> wrote:
> > 
> > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > than 2-4 are rarely required to disambiguate the calling context.
> > > 
> > > I did it for you. Let me show you the result.
> > > 
> > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > 
> > I think the lockdep + crossrelease + full-stack numbers are missing?
> 
> Ah, the last version of crossrelease merged into vanilla, records 5
> entries, since I thought it overloads too much if full stack is used,
> and 5 entries are enough. Don't you think so?

Ok, fair enough, I missed that limitation!

> > That's very reasonable and we can keep the single-entry cross-release feature 
> > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> 
> BTW, is there any crash by cross-release I don't know? Of course, I know
> cases of false positives, but I don't about crash.

There's no current crash regression that I know of - I'm just outlining the 
conditions of getting all this re-enabled in the next merge window.

Instead of sending two series, could you please send a series that includes both 
these fixing + re-enabling patches, plus the false positive fixes?

In particular I think the cross-release re-enabling should be done as the last 
patch, so that any future bisections of new false positives won't be made more 
difficult by re-introducing the old false positives near the end of the bisection.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 03:11:12PM +0900, Byungchul Park wrote:
> On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > 
> > * Byungchul Park <byungchul.park@lge.com> wrote:
> > 
> > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > than 2-4 are rarely required to disambiguate the calling context.
> > > 
> > > I did it for you. Let me show you the result.
> > > 
> > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > 
> > I think the lockdep + crossrelease + full-stack numbers are missing?
> 
> Ah, the last version of crossrelease merged into vanilla, records 5
> entries, since I thought it overloads too much if full stack is used,
> and 5 entries are enough. Don't you think so?
> 
> > But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> > performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> > 
> > That's very reasonable and we can keep the single-entry cross-release feature 
> > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> 
> BTW, is there any crash by cross-release I don't know? Of course, I know
> cases of false positives, but I don't about crash.

Are you talking about the oops by 'null pointer dereference' by unwinder a
few weeks ago?

At the time, cross-release was falsely accused. AFAIK, cross-release has
not crashed system yet.

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 03:11:12PM +0900, Byungchul Park wrote:
> On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > 
> > * Byungchul Park <byungchul.park@lge.com> wrote:
> > 
> > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > than 2-4 are rarely required to disambiguate the calling context.
> > > 
> > > I did it for you. Let me show you the result.
> > > 
> > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > 
> > I think the lockdep + crossrelease + full-stack numbers are missing?
> 
> Ah, the last version of crossrelease merged into vanilla, records 5
> entries, since I thought it overloads too much if full stack is used,
> and 5 entries are enough. Don't you think so?
> 
> > But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> > performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> > 
> > That's very reasonable and we can keep the single-entry cross-release feature 
> > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> 
> BTW, is there any crash by cross-release I don't know? Of course, I know
> cases of false positives, but I don't about crash.

Are you talking about the oops by 'null pointer dereference' by unwinder a
few weeks ago?

At the time, cross-release was falsely accused. AFAIK, cross-release has
not crashed system yet.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 08:22:12AM +0200, Ingo Molnar wrote:
> There's no current crash regression that I know of - I'm just outlining the 
> conditions of getting all this re-enabled in the next merge window.
> 
> Instead of sending two series, could you please send a series that includes both 
> these fixing + re-enabling patches, plus the false positive fixes?
> 
> In particular I think the cross-release re-enabling should be done as the last 
> patch, so that any future bisections of new false positives won't be made more 
> difficult by re-introducing the old false positives near the end of the bisection.

I agree. But I already sent v2 before you told me..

Do you want me to send patches fixing false positives in the thread
fixing performance regression?

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Byungchul Park]

On Thu, Oct 19, 2017 at 08:22:12AM +0200, Ingo Molnar wrote:
> There's no current crash regression that I know of - I'm just outlining the 
> conditions of getting all this re-enabled in the next merge window.
> 
> Instead of sending two series, could you please send a series that includes both 
> these fixing + re-enabling patches, plus the false positive fixes?
> 
> In particular I think the cross-release re-enabling should be done as the last 
> patch, so that any future bisections of new false positives won't be made more 
> difficult by re-introducing the old false positives near the end of the bisection.

I agree. But I already sent v2 before you told me..

Do you want me to send patches fixing false positives in the thread
fixing performance regression?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 08:22:12AM +0200, Ingo Molnar wrote:
> > There's no current crash regression that I know of - I'm just outlining the 
> > conditions of getting all this re-enabled in the next merge window.
> > 
> > Instead of sending two series, could you please send a series that includes both 
> > these fixing + re-enabling patches, plus the false positive fixes?
> > 
> > In particular I think the cross-release re-enabling should be done as the last 
> > patch, so that any future bisections of new false positives won't be made more 
> > difficult by re-introducing the old false positives near the end of the bisection.
> 
> I agree. But I already sent v2 before you told me..
> 
> Do you want me to send patches fixing false positives in the thread
> fixing performance regression?

No need, I'll reorder them and let you know if there's any problem left.

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 08:22:12AM +0200, Ingo Molnar wrote:
> > There's no current crash regression that I know of - I'm just outlining the 
> > conditions of getting all this re-enabled in the next merge window.
> > 
> > Instead of sending two series, could you please send a series that includes both 
> > these fixing + re-enabling patches, plus the false positive fixes?
> > 
> > In particular I think the cross-release re-enabling should be done as the last 
> > patch, so that any future bisections of new false positives won't be made more 
> > difficult by re-introducing the old false positives near the end of the bisection.
> 
> I agree. But I already sent v2 before you told me..
> 
> Do you want me to send patches fixing false positives in the thread
> fixing performance regression?

No need, I'll reorder them and let you know if there's any problem left.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 03:11:12PM +0900, Byungchul Park wrote:
> > On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > > 
> > > * Byungchul Park <byungchul.park@lge.com> wrote:
> > > 
> > > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > > than 2-4 are rarely required to disambiguate the calling context.
> > > > 
> > > > I did it for you. Let me show you the result.
> > > > 
> > > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > > 
> > > I think the lockdep + crossrelease + full-stack numbers are missing?
> > 
> > Ah, the last version of crossrelease merged into vanilla, records 5
> > entries, since I thought it overloads too much if full stack is used,
> > and 5 entries are enough. Don't you think so?
> > 
> > > But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> > > performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> > > 
> > > That's very reasonable and we can keep the single-entry cross-release feature 
> > > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> > 
> > BTW, is there any crash by cross-release I don't know? Of course, I know
> > cases of false positives, but I don't about crash.
> 
> Are you talking about the oops by 'null pointer dereference' by unwinder a
> few weeks ago?
> 
> At the time, cross-release was falsely accused. AFAIK, cross-release has
> not crashed system yet.

I'm talking about the crash fixed here:

  8b405d5c5d09: locking/lockdep: Fix stacktrace mess

Which was introduced by your patch:

  ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle external stack_trace

... which was a preparatory patch for cross-release. So 'technically' it's not a 
cross-release crash, but was very much related. It even says so in the changelog:

  Actually crossrelease needs to do other than saving a stack_trace.
  So pass a stack_trace and callback to handle it, to check_prev_add().

... so let's not pretend it wasn't related, ok?

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* Byungchul Park <byungchul.park@lge.com> wrote:

> On Thu, Oct 19, 2017 at 03:11:12PM +0900, Byungchul Park wrote:
> > On Thu, Oct 19, 2017 at 07:57:30AM +0200, Ingo Molnar wrote:
> > > 
> > > * Byungchul Park <byungchul.park@lge.com> wrote:
> > > 
> > > > On Wed, Oct 18, 2017 at 12:09:44PM +0200, Ingo Molnar wrote:
> > > > > BTW., have you attempted limiting the depth of the stack traces? I suspect more 
> > > > > than 2-4 are rarely required to disambiguate the calling context.
> > > > 
> > > > I did it for you. Let me show you the result.
> > > > 
> > > > 1. No lockdep:				2.756558155 seconds time elapsed                ( +-  0.09% )
> > > > 2. Lockdep:					2.968710420 seconds time elapsed		( +-  0.12% )
> > > > 3. Lockdep + Crossrelease 5 entries:		3.153839636 seconds time elapsed                ( +-  0.31% )
> > > > 4. Lockdep + Crossrelease 3 entries:		3.137205534 seconds time elapsed                ( +-  0.87% )
> > > > 5. Lockdep + Crossrelease + This patch:	2.963669551 seconds time elapsed		( +-  0.11% )
> > > 
> > > I think the lockdep + crossrelease + full-stack numbers are missing?
> > 
> > Ah, the last version of crossrelease merged into vanilla, records 5
> > entries, since I thought it overloads too much if full stack is used,
> > and 5 entries are enough. Don't you think so?
> > 
> > > But yeah, looks like single-entry-stacktrace crossrelease only has a +0.2% 
> > > performance cost (with 0.1% noise), while lockdep itself has a +7.7% cost.
> > > 
> > > That's very reasonable and we can keep the single-entry cross-release feature 
> > > enabled by default as part of CONFIG_PROVE_LOCKING=y - assuming all the crashes 
> > 
> > BTW, is there any crash by cross-release I don't know? Of course, I know
> > cases of false positives, but I don't about crash.
> 
> Are you talking about the oops by 'null pointer dereference' by unwinder a
> few weeks ago?
> 
> At the time, cross-release was falsely accused. AFAIK, cross-release has
> not crashed system yet.

I'm talking about the crash fixed here:

  8b405d5c5d09: locking/lockdep: Fix stacktrace mess

Which was introduced by your patch:

  ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle external stack_trace

... which was a preparatory patch for cross-release. So 'technically' it's not a 
cross-release crash, but was very much related. It even says so in the changelog:

  Actually crossrelease needs to do other than saving a stack_trace.
  So pass a stack_trace and callback to handle it, to check_prev_add().

... so let's not pretend it wasn't related, ok?

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)]

> -----Original Message-----
> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
> 
> > At the time, cross-release was falsely accused. AFAIK, cross-release has
> > not crashed system yet.
> 
> I'm talking about the crash fixed here:
> 
>   8b405d5c5d09: locking/lockdep: Fix stacktrace mess
> 
> Which was introduced by your patch:
> 
>   ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle
> external stack_trace
> 
> ... which was a preparatory patch for cross-release. So 'technically' it's not a
> cross-release crash, but was very much related. It even says so in the changelog:
> 
>   Actually crossrelease needs to do other than saving a stack_trace.
>   So pass a stack_trace and callback to handle it, to check_prev_add().
> 
> ... so let's not pretend it wasn't related, ok?

I don't want to pretend I'm perfect. Of course, I can make mistakes.
I'm just saying that *I have not seen* any crash by cross-release.

In that case you pointed out, likewise, the crash was caused by ae813308f:
lockdep: Avoid creating redundant links, which is not related to the feature
actually. It was also falsely accused at the time again...

Of course, it's my fault not to have made the design more robust so that
others can modify lockdep code caring less after cross-release commit.
That's what I'm sorry for.

I already mentioned the above in the thread talking about the issue you
are pointing now. Of course, I basically appreciate all comments and
suggestions you have given, but you seem to have mis-understood some
issues wrt cross-release feature.

Thanks,
Byungchul

RE: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="ks_c_5601-1987", Size: 1749 bytes --]

> -----Original Message-----
> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
> 
> > At the time, cross-release was falsely accused. AFAIK, cross-release has
> > not crashed system yet.
> 
> I'm talking about the crash fixed here:
> 
>   8b405d5c5d09: locking/lockdep: Fix stacktrace mess
> 
> Which was introduced by your patch:
> 
>   ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle
> external stack_trace
> 
> ... which was a preparatory patch for cross-release. So 'technically' it's not a
> cross-release crash, but was very much related. It even says so in the changelog:
> 
>   Actually crossrelease needs to do other than saving a stack_trace.
>   So pass a stack_trace and callback to handle it, to check_prev_add().
> 
> ... so let's not pretend it wasn't related, ok?

I don't want to pretend I'm perfect. Of course, I can make mistakes.
I'm just saying that *I have not seen* any crash by cross-release.

In that case you pointed out, likewise, the crash was caused by ae813308f:
lockdep: Avoid creating redundant links, which is not related to the feature
actually. It was also falsely accused at the time again...

Of course, it's my fault not to have made the design more robust so that
others can modify lockdep code caring less after cross-release commit.
That's what I'm sorry for.

I already mentioned the above in the thread talking about the issue you
are pointing now. Of course, I basically appreciate all comments and
suggestions you have given, but you seem to have mis-understood some
issues wrt cross-release feature.

Thanks,
Byungchul
N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îÆi¢žØ^n‡r¶‰šŽŠÝ¢j$½§$¢¸\x05¢¹¨­è§~Š'.)îÄÃ,yèm¶ŸÿÃ\f%Š{±šj+ƒðèž×¦j)Z†·Ÿ

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* �ں�ö/���ӿ�����/SW Platform(��)AOT��(byungchul.park@lge.com) <byungchul.park@lge.com> wrote:

> I don't want to pretend I'm perfect. Of course, I can make mistakes.
> I'm just saying that *I have not seen* any crash by cross-release.
> 
> In that case you pointed out, likewise, the crash was caused by ae813308f:
> lockdep: Avoid creating redundant links, which is not related to the feature
> actually. It was also falsely accused at the time again...
> 
> Of course, it's my fault not to have made the design more robust so that
> others can modify lockdep code caring less after cross-release commit.
> That's what I'm sorry for.
> 
> I already mentioned the above in the thread talking about the issue you
> are pointing now. Of course, I basically appreciate all comments and
> suggestions you have given, but you seem to have mis-understood some
> issues wrt cross-release feature.

Two different cross-release commits got bisected to with kernel crashes:

  Sep 30 kernel test rob    | ce07a9415f ("locking/lockdep: Make check_prev_add() able to .."):  BUG: unable to handle kernel NULL pointer dereference at 00000020
  Oct 03 Fengguang Wu       | [lockdep] b09be676e0 BUG: unable to handle kernel NULL pointer dereference at 000001f2

The first crash was bisected to:

  ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle external stack_trace

The second crash was bisected to:

  b09be676e0ff: locking/lockdep: Implement the 'crossrelease' feature

... and unless your argument that both bisections were bad, it doesn't matter 
where the root cause ended up being, fact is that it was not a problem free series 
and let's not pretend it was.

Note that to me it *really* does not matter that a commit causes a crash: bugs 
happen, they are part of software development done by humans - so as long as it's 
not a pattern of underlying carelessness or some development process error it's 
not something to get emotional about.

Ok?

Thanks,

	Ingo

Re: [PATCH 1/2] lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as default

[Ingo Molnar]

* i? 1/2 Uoi? 1/2 A?/i? 1/2 i? 1/2 i? 1/2 O?i? 1/2 i? 1/2 i? 1/2 i? 1/2 i? 1/2 /SW Platform(i? 1/2 i? 1/2 )AOTi? 1/2 i? 1/2 (byungchul.park@lge.com) <byungchul.park@lge.com> wrote:

> I don't want to pretend I'm perfect. Of course, I can make mistakes.
> I'm just saying that *I have not seen* any crash by cross-release.
> 
> In that case you pointed out, likewise, the crash was caused by ae813308f:
> lockdep: Avoid creating redundant links, which is not related to the feature
> actually. It was also falsely accused at the time again...
> 
> Of course, it's my fault not to have made the design more robust so that
> others can modify lockdep code caring less after cross-release commit.
> That's what I'm sorry for.
> 
> I already mentioned the above in the thread talking about the issue you
> are pointing now. Of course, I basically appreciate all comments and
> suggestions you have given, but you seem to have mis-understood some
> issues wrt cross-release feature.

Two different cross-release commits got bisected to with kernel crashes:

  Sep 30 kernel test rob    | ce07a9415f ("locking/lockdep: Make check_prev_add() able to .."):  BUG: unable to handle kernel NULL pointer dereference at 00000020
  Oct 03 Fengguang Wu       | [lockdep] b09be676e0 BUG: unable to handle kernel NULL pointer dereference at 000001f2

The first crash was bisected to:

  ce07a9415f26: locking/lockdep: Make check_prev_add() able to handle external stack_trace

The second crash was bisected to:

  b09be676e0ff: locking/lockdep: Implement the 'crossrelease' feature

... and unless your argument that both bisections were bad, it doesn't matter 
where the root cause ended up being, fact is that it was not a problem free series 
and let's not pretend it was.

Note that to me it *really* does not matter that a commit causes a crash: bugs 
happen, they are part of software development done by humans - so as long as it's 
not a pattern of underlying carelessness or some development process error it's 
not something to get emotional about.

Ok?

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>