[PATCH v2 0/3] crossrelease: make it not unwind by default

[PATCH v2 0/3] crossrelease: make it not unwind by default

[Byungchul Park]

Change from v1
- Fix kconfig description as Ingo suggested
- Fix commit message writing out CONFIG_ variable
- Introduce a new kernel parameter, crossrelease_fullstack
- Replace the number with the output of *perf*

Byungchul Park (3):
  lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as
    default
  lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE
  lockdep: Add a kernel parameter, crossrelease_fullstack

 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 include/linux/lockdep.h                         |  4 ++++
 kernel/locking/lockdep.c                        | 23 +++++++++++++++++++++--
 lib/Kconfig.debug                               | 20 ++++++++++++++++++--
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v2 0/3] crossrelease: make it not unwind by default

[Byungchul Park]

Change from v1
- Fix kconfig description as Ingo suggested
- Fix commit message writing out CONFIG_ variable
- Introduce a new kernel parameter, crossrelease_fullstack
- Replace the number with the output of *perf*

Byungchul Park (3):
  lockdep: Introduce CROSSRELEASE_STACK_TRACE and make it not unwind as
    default
  lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE
  lockdep: Add a kernel parameter, crossrelease_fullstack

 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 include/linux/lockdep.h                         |  4 ++++
 kernel/locking/lockdep.c                        | 23 +++++++++++++++++++++--
 lib/Kconfig.debug                               | 20 ++++++++++++++++++--
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
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 v2 1/3] 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 booting times with 'perf stat --null --repeat 10
$QEMU', where $QEMU launchs a kernel with init=/bin/true:

1. No lockdep enabled

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

       2.756558155 seconds time elapsed                    ( +-  0.09% )

2. Lockdep enabled

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

       2.968710420 seconds time elapsed                    ( +-  0.12% )

3. Lockdep enabled + crossrelease enabled

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

       3.153839636 seconds time elapsed                    ( +-  0.31% )

4. Lockdep enabled + crossrelease enabled + this patch applied

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

       2.963669551 seconds time elapsed                    ( +-  0.11% )

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..90ea784 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
+	 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.
+
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
 	depends on DEBUG_KERNEL && LOCKDEP
-- 
1.9.1

[PATCH v2 1/3] 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 booting times with 'perf stat --null --repeat 10
$QEMU', where $QEMU launchs a kernel with init=/bin/true:

1. No lockdep enabled

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

       2.756558155 seconds time elapsed                    ( +-  0.09% )

2. Lockdep enabled

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

       2.968710420 seconds time elapsed                    ( +-  0.12% )

3. Lockdep enabled + crossrelease enabled

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

       3.153839636 seconds time elapsed                    ( +-  0.31% )

4. Lockdep enabled + crossrelease enabled + this patch applied

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

       2.963669551 seconds time elapsed                    ( +-  0.11% )

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..90ea784 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
+	 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.
+
 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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

Now the performance regression was fixed, re-enable
CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

Now the performance regression was fixed, re-enable
CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Bart Van Assche]

On Thu, 2017-10-19 at 14:55 +0900, Byungchul Park wrote:
> Now the performance regression was fixed, re-enable
> CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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

I do not agree with this patch. Although the traditional lock validation
code can be proven not to produce false positives, that is not the case for
the cross-release checks. These checks are prone to produce false positives.
Many kernel developers, including myself, are not interested in spending
time on analyzing false positive deadlock reports. So I think that it is
wrong to enable cross-release checking unconditionally if PROVE_LOCKING has
been enabled. What I think that should happen is that either the cross-
release checking code is removed from the kernel or that
LOCKDEP_CROSSRELEASE becomes a new kernel configuration option. That will
give kernel developers who choose to enable PROVE_LOCKING the freedom to
decide whether or not to enable LOCKDEP_CROSSRELEASE.

Bart.

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

[Bart Van Assche]

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

On Thu, 2017-10-19 at 14:55 +0900, Byungchul Park wrote:
> Now the performance regression was fixed, re-enable
> CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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

I do not agree with this patch. Although the traditional lock validation
code can be proven not to produce false positives, that is not the case for
the cross-release checks. These checks are prone to produce false positives.
Many kernel developers, including myself, are not interested in spending
time on analyzing false positive deadlock reports. So I think that it is
wrong to enable cross-release checking unconditionally if PROVE_LOCKING has
been enabled. What I think that should happen is that either the cross-
release checking code is removed from the kernel or that
LOCKDEP_CROSSRELEASE becomes a new kernel configuration option. That will
give kernel developers who choose to enable PROVE_LOCKING the freedom to
decide whether or not to enable LOCKDEP_CROSSRELEASE.

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

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:

> On Thu, 2017-10-19 at 14:55 +0900, Byungchul Park wrote:
> > Now the performance regression was fixed, re-enable
> > CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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
> 
> I do not agree with this patch. Although the traditional lock validation
> code can be proven not to produce false positives, that is not the case for
> the cross-release checks. These checks are prone to produce false positives.
> Many kernel developers, including myself, are not interested in spending
> time on analyzing false positive deadlock reports. So I think that it is
> wrong to enable cross-release checking unconditionally if PROVE_LOCKING has
> been enabled. What I think that should happen is that either the cross-
> release checking code is removed from the kernel or that
> LOCKDEP_CROSSRELEASE becomes a new kernel configuration option. That will
> give kernel developers who choose to enable PROVE_LOCKING the freedom to
> decide whether or not to enable LOCKDEP_CROSSRELEASE.

I really disagree with your reasoning completely

1) When lockdep was introduced more than ten years ago it was far from
   perfect and we spent a reasonable amount of time to improve it, analyze
   false positives and add the missing annotations all over the tree. That
   was a process which took years.

2) Surely nobody is interested in wasting time on analyzing false
   positives, but your (and other peoples) attidute of 'none of my
   business' is what makes kernel development extremly frustrating.

   It should be in the interest of everybody involved in kernel development
   to help with improving such features and not to lean back and wait for
   others to bring it into a shape which allows you to use it as you see
   fit.

That's not how community works and lockdep would not be in the shape it is
today, if only a handful of people would have used and improved it. Such
things only work when used widely and when we get enough information so we
can address the weak spots.

Thanks,

	tglx


   

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:

> On Thu, 2017-10-19 at 14:55 +0900, Byungchul Park wrote:
> > Now the performance regression was fixed, re-enable
> > CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_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 90ea784..fe8fceb 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
> 
> I do not agree with this patch. Although the traditional lock validation
> code can be proven not to produce false positives, that is not the case for
> the cross-release checks. These checks are prone to produce false positives.
> Many kernel developers, including myself, are not interested in spending
> time on analyzing false positive deadlock reports. So I think that it is
> wrong to enable cross-release checking unconditionally if PROVE_LOCKING has
> been enabled. What I think that should happen is that either the cross-
> release checking code is removed from the kernel or that
> LOCKDEP_CROSSRELEASE becomes a new kernel configuration option. That will
> give kernel developers who choose to enable PROVE_LOCKING the freedom to
> decide whether or not to enable LOCKDEP_CROSSRELEASE.

I really disagree with your reasoning completely

1) When lockdep was introduced more than ten years ago it was far from
   perfect and we spent a reasonable amount of time to improve it, analyze
   false positives and add the missing annotations all over the tree. That
   was a process which took years.

2) Surely nobody is interested in wasting time on analyzing false
   positives, but your (and other peoples) attidute of 'none of my
   business' is what makes kernel development extremly frustrating.

   It should be in the interest of everybody involved in kernel development
   to help with improving such features and not to lean back and wait for
   others to bring it into a shape which allows you to use it as you see
   fit.

That's not how community works and lockdep would not be in the shape it is
today, if only a handful of people would have used and improved it. Such
things only work when used widely and when we get enough information so we
can address the weak spots.

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Bart Van Assche]

On Thu, 2017-10-19 at 17:34 +0200, Thomas Gleixner wrote:
> I really disagree with your reasoning completely
> 
> 1) When lockdep was introduced more than ten years ago it was far from
>    perfect and we spent a reasonable amount of time to improve it, analyze
>    false positives and add the missing annotations all over the tree. That
>    was a process which took years.
> 
> 2) Surely nobody is interested in wasting time on analyzing false
>    positives, but your (and other peoples) attidute of 'none of my
>    business' is what makes kernel development extremly frustrating.
> 
>    It should be in the interest of everybody involved in kernel development
>    to help with improving such features and not to lean back and wait for
>    others to bring it into a shape which allows you to use it as you see
>    fit.
> 
> That's not how community works and lockdep would not be in the shape it is
> today, if only a handful of people would have used and improved it. Such
> things only work when used widely and when we get enough information so we
> can address the weak spots.

Hello Thomas,

It seems like you are missing my point. Cross-release checking is really
*broken* as a concept. It is impossible to improve it to the same reliability
level as the kernel v4.13 lockdep code. Hence my request to make it possible
to disable cross-release checking if PROVE_LOCKING is enabled.

Consider the following example from the cross-release documentation:

   TASK X			   TASK Y
   ------			   ------
				   acquire AX
   acquire B /* A dependency 'AX -> B' exists */
   release B
   release AX held by Y

My understanding is that the cross-release code will add (AX, B) to the lock
order graph after having encountered the above code. I think that's wrong
because if the following sequence (Y: acquire AX, X: acquire B, X: release B)
is encountered again that there is no guarantee that AX can only be released
by X. Any task other than X could release that synchronization object too.

Bart.

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

[Thomas Gleixner]

Bart,

On Thu, 19 Oct 2017, Bart Van Assche wrote:

> It seems like you are missing my point.

That might be a perception problem. 

> Cross-release checking is really *broken* as a concept. It is impossible
> to improve it to the same reliability level as the kernel v4.13 lockdep
> code. Hence my request to make it possible to disable cross-release
> checking if PROVE_LOCKING is enabled.

I did not read it as a request. If you'd had said:

  I have doubts about the concept and I think that it's impossible to
  handle the false positives up to the point where the existing lockdep
  infrastructure can do. Therefore I request that the feature gets an extra
  Kconfig entry (default y) or a command line parameter which allows to
  disable it in case of hard to fix false positive warnings, so the issue
  can be reported and normal lockdep testing can be resumed until the issue
  is fixed.

Then I would have said: That makes sense, as long as its default on and
people actually report the problems so the responsible developers can
tackle them.

What tripped me over was your statement:

  Many kernel developers, including myself, are not interested in spending
  time on analyzing false positive deadlock reports.

Which sends a completely different message.

> Consider the following example from the cross-release documentation:
> 
>    TASK X			   TASK Y
>    ------			   ------
> 				   acquire AX
>    acquire B /* A dependency 'AX -> B' exists */
>    release B
>    release AX held by Y
> 
> My understanding is that the cross-release code will add (AX, B) to the lock
> order graph after having encountered the above code. I think that's wrong
> because if the following sequence (Y: acquire AX, X: acquire B, X: release B)
> is encountered again that there is no guarantee that AX can only be released
> by X. Any task other than X could release that synchronization object too.

Emphasis on could.

That's not a lockdep problem and neither can the pure locking dependency
tracking know that a particular deadlock is not possible by design. It can
merily record the dependency chains and detect circular dependencies.

There is enough code which is obviously correct in terms of locking which
has lockdep annotations in one form or the other (nesting, different
lock_class_keys etc.). These annotations are there to teach lockdep about
false positives. It's pretty much the same with the cross release feature
and we won't get these annotations into the code when people disable it 

Thanks,

	tglx

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

[Thomas Gleixner]

Bart,

On Thu, 19 Oct 2017, Bart Van Assche wrote:

> It seems like you are missing my point.

That might be a perception problem. 

> Cross-release checking is really *broken* as a concept. It is impossible
> to improve it to the same reliability level as the kernel v4.13 lockdep
> code. Hence my request to make it possible to disable cross-release
> checking if PROVE_LOCKING is enabled.

I did not read it as a request. If you'd had said:

  I have doubts about the concept and I think that it's impossible to
  handle the false positives up to the point where the existing lockdep
  infrastructure can do. Therefore I request that the feature gets an extra
  Kconfig entry (default y) or a command line parameter which allows to
  disable it in case of hard to fix false positive warnings, so the issue
  can be reported and normal lockdep testing can be resumed until the issue
  is fixed.

Then I would have said: That makes sense, as long as its default on and
people actually report the problems so the responsible developers can
tackle them.

What tripped me over was your statement:

  Many kernel developers, including myself, are not interested in spending
  time on analyzing false positive deadlock reports.

Which sends a completely different message.

> Consider the following example from the cross-release documentation:
> 
>    TASK X			   TASK Y
>    ------			   ------
> 				   acquire AX
>    acquire B /* A dependency 'AX -> B' exists */
>    release B
>    release AX held by Y
> 
> My understanding is that the cross-release code will add (AX, B) to the lock
> order graph after having encountered the above code. I think that's wrong
> because if the following sequence (Y: acquire AX, X: acquire B, X: release B)
> is encountered again that there is no guarantee that AX can only be released
> by X. Any task other than X could release that synchronization object too.

Emphasis on could.

That's not a lockdep problem and neither can the pure locking dependency
tracking know that a particular deadlock is not possible by design. It can
merily record the dependency chains and detect circular dependencies.

There is enough code which is obviously correct in terms of locking which
has lockdep annotations in one form or the other (nesting, different
lock_class_keys etc.). These annotations are there to teach lockdep about
false positives. It's pretty much the same with the cross release feature
and we won't get these annotations into the code when people disable it 

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Thomas Gleixner]

On Thu, 19 Oct 2017, Thomas Gleixner wrote:
> That's not a lockdep problem and neither can the pure locking dependency
> tracking know that a particular deadlock is not possible by design. It can
> merily record the dependency chains and detect circular dependencies.
> 
> There is enough code which is obviously correct in terms of locking which
> has lockdep annotations in one form or the other (nesting, different
> lock_class_keys etc.). These annotations are there to teach lockdep about
> false positives. It's pretty much the same with the cross release feature
> and we won't get these annotations into the code when people disable it 

And just for the record, I wasted enough of my time already to decode 'can
not happen' dead locks where completions or other wait primitives have been
involved. I rather spend time annotating stuff after analyzing it proper
than chasing happens once in a blue moon lockups which are completely
unexplainable.

That's why lockdep exists in the first place. Ingo, Steven, myself and
others spent an insane amount of time to fix locking bugs all over the tree
when we started the preempt RT work. Lockdep was a rescue because it forced
people to look at their own crap and if it was 100% clear that lockdep
tripped a false positive either lockdep was fixed or the code in question
annotated, which is a good thing because that's documentation at the same
time.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Thomas Gleixner wrote:
> That's not a lockdep problem and neither can the pure locking dependency
> tracking know that a particular deadlock is not possible by design. It can
> merily record the dependency chains and detect circular dependencies.
> 
> There is enough code which is obviously correct in terms of locking which
> has lockdep annotations in one form or the other (nesting, different
> lock_class_keys etc.). These annotations are there to teach lockdep about
> false positives. It's pretty much the same with the cross release feature
> and we won't get these annotations into the code when people disable it 

And just for the record, I wasted enough of my time already to decode 'can
not happen' dead locks where completions or other wait primitives have been
involved. I rather spend time annotating stuff after analyzing it proper
than chasing happens once in a blue moon lockups which are completely
unexplainable.

That's why lockdep exists in the first place. Ingo, Steven, myself and
others spent an insane amount of time to fix locking bugs all over the tree
when we started the preempt RT work. Lockdep was a rescue because it forced
people to look at their own crap and if it was 100% clear that lockdep
tripped a false positive either lockdep was fixed or the code in question
annotated, which is a good thing because that's documentation at the same
time.

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Bart Van Assche]

On Thu, 2017-10-19 at 21:12 +0200, Thomas Gleixner wrote:
> And just for the record, I wasted enough of my time already to decode 'can
> not happen' dead locks where completions or other wait primitives have been
> involved. I rather spend time annotating stuff after analyzing it proper
> than chasing happens once in a blue moon lockups which are completely
> unexplainable.
> 
> That's why lockdep exists in the first place. Ingo, Steven, myself and
> others spent an insane amount of time to fix locking bugs all over the tree
> when we started the preempt RT work. Lockdep was a rescue because it forced
> people to look at their own crap and if it was 100% clear that lockdep
> tripped a false positive either lockdep was fixed or the code in question
> annotated, which is a good thing because that's documentation at the same
> time.

Hello Thomas,

In case it wouldn't be clear, your work and the work of others on lockdep
and preempt-rt is highly appreciated. Sorry that I missed the discussion
about the cross-release functionality when it went upstream. I have several
questions about that functionality:
* How many lock inversion problems have been found so far thanks to the
  cross-release checking? How many false positives have the cross-release
  checks triggered so far? Does the number of real issues that has been
  found outweigh the effort spent on suppressing false positives?
* What alternatives have been considered other than enabling cross-release
  checking for all locking objects that support releasing from the context
  of another task than the context from which the lock was obtained? Has it
  e.g. been considered to introduce two versions of the lock objects that
  support cross-releases - one version for which lock inversion checking is
  always enabled and another version for which lock inversion checking is
  always disabled?
* How much review has the Documentation/locking/crossrelease.txt received
  before it went upstream? At least to me that document seems much harder
  to read than other kernel documentation due to weird use of the English
  grammar.

Thanks,

Bart.

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

[Bart Van Assche]

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

On Thu, 2017-10-19 at 21:12 +0200, Thomas Gleixner wrote:
> And just for the record, I wasted enough of my time already to decode 'can
> not happen' dead locks where completions or other wait primitives have been
> involved. I rather spend time annotating stuff after analyzing it proper
> than chasing happens once in a blue moon lockups which are completely
> unexplainable.
> 
> That's why lockdep exists in the first place. Ingo, Steven, myself and
> others spent an insane amount of time to fix locking bugs all over the tree
> when we started the preempt RT work. Lockdep was a rescue because it forced
> people to look at their own crap and if it was 100% clear that lockdep
> tripped a false positive either lockdep was fixed or the code in question
> annotated, which is a good thing because that's documentation at the same
> time.

Hello Thomas,

In case it wouldn't be clear, your work and the work of others on lockdep
and preempt-rt is highly appreciated. Sorry that I missed the discussion
about the cross-release functionality when it went upstream. I have several
questions about that functionality:
* How many lock inversion problems have been found so far thanks to the
  cross-release checking? How many false positives have the cross-release
  checks triggered so far? Does the number of real issues that has been
  found outweigh the effort spent on suppressing false positives?
* What alternatives have been considered other than enabling cross-release
  checking for all locking objects that support releasing from the context
  of another task than the context from which the lock was obtained? Has it
  e.g. been considered to introduce two versions of the lock objects that
  support cross-releases - one version for which lock inversion checking is
  always enabled and another version for which lock inversion checking is
  always disabled?
* How much review has the Documentation/locking/crossrelease.txt received
  before it went upstream? At least to me that document seems much harder
  to read than other kernel documentation due to weird use of the English
  grammar.

Thanks,

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

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

[Matthew Wilcox]

On Thu, Oct 19, 2017 at 08:21:56PM +0000, Bart Van Assche wrote:
> In case it wouldn't be clear, your work and the work of others on lockdep
> and preempt-rt is highly appreciated. Sorry that I missed the discussion
> about the cross-release functionality when it went upstream. I have several
> questions about that functionality:
> * How many lock inversion problems have been found so far thanks to the
>   cross-release checking? How many false positives have the cross-release
>   checks triggered so far? Does the number of real issues that has been
>   found outweigh the effort spent on suppressing false positives?
> * What alternatives have been considered other than enabling cross-release
>   checking for all locking objects that support releasing from the context
>   of another task than the context from which the lock was obtained? Has it
>   e.g. been considered to introduce two versions of the lock objects that
>   support cross-releases - one version for which lock inversion checking is
>   always enabled and another version for which lock inversion checking is
>   always disabled?
> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

While interesting, I think this list of questions misses an important one:

 * How many bugs is this going to catch in the future?

For example, the page lock is not annotatable with lockdep -- we return
to userspace with it held, for heaven's sake!  So it is quite easy for
someone not familiar with the MM locking hierarchy to inadvertently
introduce an ABBA deadlock against the page lock.  (ie me.  I did that.)
Right now, that has to be caught by a human reviewer; if cross-release
checking can catch that, then it's worth having.

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

[Matthew Wilcox]

On Thu, Oct 19, 2017 at 08:21:56PM +0000, Bart Van Assche wrote:
> In case it wouldn't be clear, your work and the work of others on lockdep
> and preempt-rt is highly appreciated. Sorry that I missed the discussion
> about the cross-release functionality when it went upstream. I have several
> questions about that functionality:
> * How many lock inversion problems have been found so far thanks to the
>   cross-release checking? How many false positives have the cross-release
>   checks triggered so far? Does the number of real issues that has been
>   found outweigh the effort spent on suppressing false positives?
> * What alternatives have been considered other than enabling cross-release
>   checking for all locking objects that support releasing from the context
>   of another task than the context from which the lock was obtained? Has it
>   e.g. been considered to introduce two versions of the lock objects that
>   support cross-releases - one version for which lock inversion checking is
>   always enabled and another version for which lock inversion checking is
>   always disabled?
> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

While interesting, I think this list of questions misses an important one:

 * How many bugs is this going to catch in the future?

For example, the page lock is not annotatable with lockdep -- we return
to userspace with it held, for heaven's sake!  So it is quite easy for
someone not familiar with the MM locking hierarchy to inadvertently
introduce an ABBA deadlock against the page lock.  (ie me.  I did that.)
Right now, that has to be caught by a human reviewer; if cross-release
checking can catch that, then it's worth having.

--
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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Bart Van Assche]

On Thu, 2017-10-19 at 13:33 -0700, Matthew Wilcox wrote:
> For example, the page lock is not annotatable with lockdep -- we return
> to userspace with it held, for heaven's sake!  So it is quite easy for
> someone not familiar with the MM locking hierarchy to inadvertently
> introduce an ABBA deadlock against the page lock.  (ie me.  I did that.)
> Right now, that has to be caught by a human reviewer; if cross-release
> checking can catch that, then it's worth having.

Hello Matthew,

Although I agree that enabling lock inversion checking for page locks is
useful, I think my questions still apply to other locking objects than page
locks.

Thanks,

Bart.

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:
> On Thu, 2017-10-19 at 13:33 -0700, Matthew Wilcox wrote:
> > For example, the page lock is not annotatable with lockdep -- we return
> > to userspace with it held, for heaven's sake!  So it is quite easy for
> > someone not familiar with the MM locking hierarchy to inadvertently
> > introduce an ABBA deadlock against the page lock.  (ie me.  I did that.)
> > Right now, that has to be caught by a human reviewer; if cross-release
> > checking can catch that, then it's worth having.
> 
> Hello Matthew,
> 
> Although I agree that enabling lock inversion checking for page locks is
> useful, I think my questions still apply to other locking objects than page
> locks.

Why are other objects any different?

    lock(L)   ->      wait_for_completion(A)
    lock(L)   ->      complete(A)

is a simple ABBA and they exist and have not been caught for a long time
until they choked a production machine.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:
> On Thu, 2017-10-19 at 13:33 -0700, Matthew Wilcox wrote:
> > For example, the page lock is not annotatable with lockdep -- we return
> > to userspace with it held, for heaven's sake!  So it is quite easy for
> > someone not familiar with the MM locking hierarchy to inadvertently
> > introduce an ABBA deadlock against the page lock.  (ie me.  I did that.)
> > Right now, that has to be caught by a human reviewer; if cross-release
> > checking can catch that, then it's worth having.
> 
> Hello Matthew,
> 
> Although I agree that enabling lock inversion checking for page locks is
> useful, I think my questions still apply to other locking objects than page
> locks.

Why are other objects any different?

    lock(L)   ->      wait_for_completion(A)
    lock(L)   ->      complete(A)

is a simple ABBA and they exist and have not been caught for a long time
until they choked a production machine.

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:
> * How many lock inversion problems have been found so far thanks to the
>   cross-release checking? How many false positives have the cross-release
>   checks triggered so far? Does the number of real issues that has been
>   found outweigh the effort spent on suppressing false positives?

That's bean counting which is completely irrelevant. Real issues and false
positives are both problems which need to be looked at carefully.

- The deadlock needs to be fixed, which is obvious.

- The false positive needs to be annotated, which is a good thing in
  several aspects:

  It proofs that this was done intentional and is correct and the
  annotation documents it at the same time in the code.

  I'm pretty sure that except for a few obvious ones the effort to prove
  that a false positive is a false positive is substantial, but not proving
  it would either be arrogant or outright stupid.

So it's not a N > M question. Even if the number of false positives is
higher than the number of real deadlocks, then everyone out in the field
who had to stare at his server once a year not making progress and not
telling why will appreciate that these obscure issues are gone.

> * What alternatives have been considered other than enabling cross-release
>   checking for all locking objects that support releasing from the context
>   of another task than the context from which the lock was obtained? Has it
>   e.g. been considered to introduce two versions of the lock objects that
>   support cross-releases - one version for which lock inversion checking is
>   always enabled and another version for which lock inversion checking is
>   always disabled?

That would just make the door open for evading lockdep. This has been
discussed when lockdep was introduced and with a lot of other 'annoying'
debug features we've seen the same discussion happening.

When they get introduced the number of real issues and false positives is
high, but once the dust settles it's just business as usual and the overall
code quality improves and the number of hard to decode problems shrinks.

> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

It was reviewed, and yes it could do with some polishing, but it's a good
start.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 19 Oct 2017, Bart Van Assche wrote:
> * How many lock inversion problems have been found so far thanks to the
>   cross-release checking? How many false positives have the cross-release
>   checks triggered so far? Does the number of real issues that has been
>   found outweigh the effort spent on suppressing false positives?

That's bean counting which is completely irrelevant. Real issues and false
positives are both problems which need to be looked at carefully.

- The deadlock needs to be fixed, which is obvious.

- The false positive needs to be annotated, which is a good thing in
  several aspects:

  It proofs that this was done intentional and is correct and the
  annotation documents it at the same time in the code.

  I'm pretty sure that except for a few obvious ones the effort to prove
  that a false positive is a false positive is substantial, but not proving
  it would either be arrogant or outright stupid.

So it's not a N > M question. Even if the number of false positives is
higher than the number of real deadlocks, then everyone out in the field
who had to stare at his server once a year not making progress and not
telling why will appreciate that these obscure issues are gone.

> * What alternatives have been considered other than enabling cross-release
>   checking for all locking objects that support releasing from the context
>   of another task than the context from which the lock was obtained? Has it
>   e.g. been considered to introduce two versions of the lock objects that
>   support cross-releases - one version for which lock inversion checking is
>   always enabled and another version for which lock inversion checking is
>   always disabled?

That would just make the door open for evading lockdep. This has been
discussed when lockdep was introduced and with a lot of other 'annoying'
debug features we've seen the same discussion happening.

When they get introduced the number of real issues and false positives is
high, but once the dust settles it's just business as usual and the overall
code quality improves and the number of hard to decode problems shrinks.

> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

It was reviewed, and yes it could do with some polishing, but it's a good
start.

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Ingo Molnar]

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

> That would just make the door open for evading lockdep. This has been
> discussed when lockdep was introduced and with a lot of other 'annoying'
> debug features we've seen the same discussion happening.
> 
> When they get introduced the number of real issues and false positives is
> high, but once the dust settles it's just business as usual and the overall
> code quality improves and the number of hard to decode problems shrinks.

Yes, also note that it's typical that the proportion of false positives 
*increases* once a lock debugging feature enters a more mature period of its 
existence, because real deadlocks tend to be fixed at the development stage 
without us ever seeing them.

I.e. for every lockdep-debugged bug fixed upstream I'm pretty sure there are at 
least 10 times as many bugs that were fixed in earlier stages of development, 
without ever hitting the upstream kernel. At least that's the rough proportion
for locking bugs I introduce ;-)

So even false positives are not a problem as long as their annotation improves the 
code or documents it better.

Thanks,

	Ingo

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

[Ingo Molnar]

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

> That would just make the door open for evading lockdep. This has been
> discussed when lockdep was introduced and with a lot of other 'annoying'
> debug features we've seen the same discussion happening.
> 
> When they get introduced the number of real issues and false positives is
> high, but once the dust settles it's just business as usual and the overall
> code quality improves and the number of hard to decode problems shrinks.

Yes, also note that it's typical that the proportion of false positives 
*increases* once a lock debugging feature enters a more mature period of its 
existence, because real deadlocks tend to be fixed at the development stage 
without us ever seeing them.

I.e. for every lockdep-debugged bug fixed upstream I'm pretty sure there are at 
least 10 times as many bugs that were fixed in earlier stages of development, 
without ever hitting the upstream kernel. At least that's the rough proportion
for locking bugs I introduce ;-)

So even false positives are not a problem as long as their annotation improves the 
code or documents it better.

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 v2 2/3] lockdep: Remove BROKEN flag of LOCKDEP_CROSSRELEASE

[Byungchul Park]

On Thu, Oct 19, 2017 at 08:21:56PM +0000, Bart Van Assche wrote:
> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

Sorry for the bad English. Please help it enhanced.

For others, Thomas and Matthew already did exactly what to say, well.

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

[Byungchul Park]

On Thu, Oct 19, 2017 at 08:21:56PM +0000, Bart Van Assche wrote:
> * How much review has the Documentation/locking/crossrelease.txt received
>   before it went upstream? At least to me that document seems much harder
>   to read than other kernel documentation due to weird use of the English
>   grammar.

Sorry for the bad English. Please help it enhanced.

For others, Thomas and Matthew already did exactly what to say, well.

--
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 v2 3/3] lockdep: Add a kernel parameter, crossrelease_fullstack

[Byungchul Park]

Make whether to allow recording full stack, in cross-release feature,
switchable at boot time via a kernel parameter, 'crossrelease_fullstack'.
In case of a splat with no stack trace, one could just reboot and set
the kernel parameter to get the full data without having to recompile
the kernel.

Change CONFIG_CROSSRELEASE_STACK_TRACE default from N to Y, and
introduce the new kernel parameter.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 kernel/locking/lockdep.c                        | 18 ++++++++++++++++--
 lib/Kconfig.debug                               |  5 +++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ead7f40..4107b01 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -709,6 +709,9 @@
 			It will be ignored when crashkernel=X,high is not used
 			or memory reserved is below 4G.
 
+	crossrelease_fullstack
+			[KNL] Allow to record full stack trace in cross-release
+
 	cryptomgr.notests
                         [KNL] Disable crypto self-tests
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 5c2ddf2..feba887 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -76,6 +76,15 @@
 #define lock_stat 0
 #endif
 
+static int crossrelease_fullstack;
+static int __init allow_crossrelease_fullstack(char *str)
+{
+	crossrelease_fullstack = 1;
+	return 0;
+}
+
+early_param("crossrelease_fullstack", allow_crossrelease_fullstack);
+
 /*
  * lockdep_lock: protects the lockdep graph, the hashes and the
  *               class/list/hash allocators.
@@ -4864,8 +4873,13 @@ static void add_xhlock(struct held_lock *hlock)
 	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);
+	if (crossrelease_fullstack) {
+		xhlock->trace.skip = 3;
+		save_stack_trace(&xhlock->trace);
+	} else {
+		xhlock->trace.nr_entries = 1;
+		xhlock->trace.entries[0] = hlock->acquire_ip;
+	}
 #else
 	xhlock->trace.nr_entries = 1;
 	xhlock->trace.entries[0] = hlock->acquire_ip;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fe8fceb..132536d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1228,7 +1228,7 @@ config LOCKDEP_COMPLETIONS
 config CROSSRELEASE_STACK_TRACE
 	bool "Record more than one entity of stack trace in crossrelease"
 	depends on LOCKDEP_CROSSRELEASE
-	default n
+	default y
 	help
 	 The lockdep "cross-release" feature needs to record stack traces
 	 (of calling functions) for all acquisitions, for eventual later
@@ -1238,7 +1238,8 @@ config CROSSRELEASE_STACK_TRACE
 	 full analysis. This option turns on the saving of the full stack
 	 trace entries.
 
-	 If unsure, say N.
+	 To make the feature actually on, set "crossrelease_fullstack"
+	 kernel parameter, too.
 
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
-- 
1.9.1

[PATCH v2 3/3] lockdep: Add a kernel parameter, crossrelease_fullstack

[Byungchul Park]

Make whether to allow recording full stack, in cross-release feature,
switchable at boot time via a kernel parameter, 'crossrelease_fullstack'.
In case of a splat with no stack trace, one could just reboot and set
the kernel parameter to get the full data without having to recompile
the kernel.

Change CONFIG_CROSSRELEASE_STACK_TRACE default from N to Y, and
introduce the new kernel parameter.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 kernel/locking/lockdep.c                        | 18 ++++++++++++++++--
 lib/Kconfig.debug                               |  5 +++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ead7f40..4107b01 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -709,6 +709,9 @@
 			It will be ignored when crashkernel=X,high is not used
 			or memory reserved is below 4G.
 
+	crossrelease_fullstack
+			[KNL] Allow to record full stack trace in cross-release
+
 	cryptomgr.notests
                         [KNL] Disable crypto self-tests
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 5c2ddf2..feba887 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -76,6 +76,15 @@
 #define lock_stat 0
 #endif
 
+static int crossrelease_fullstack;
+static int __init allow_crossrelease_fullstack(char *str)
+{
+	crossrelease_fullstack = 1;
+	return 0;
+}
+
+early_param("crossrelease_fullstack", allow_crossrelease_fullstack);
+
 /*
  * lockdep_lock: protects the lockdep graph, the hashes and the
  *               class/list/hash allocators.
@@ -4864,8 +4873,13 @@ static void add_xhlock(struct held_lock *hlock)
 	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);
+	if (crossrelease_fullstack) {
+		xhlock->trace.skip = 3;
+		save_stack_trace(&xhlock->trace);
+	} else {
+		xhlock->trace.nr_entries = 1;
+		xhlock->trace.entries[0] = hlock->acquire_ip;
+	}
 #else
 	xhlock->trace.nr_entries = 1;
 	xhlock->trace.entries[0] = hlock->acquire_ip;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fe8fceb..132536d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1228,7 +1228,7 @@ config LOCKDEP_COMPLETIONS
 config CROSSRELEASE_STACK_TRACE
 	bool "Record more than one entity of stack trace in crossrelease"
 	depends on LOCKDEP_CROSSRELEASE
-	default n
+	default y
 	help
 	 The lockdep "cross-release" feature needs to record stack traces
 	 (of calling functions) for all acquisitions, for eventual later
@@ -1238,7 +1238,8 @@ config CROSSRELEASE_STACK_TRACE
 	 full analysis. This option turns on the saving of the full stack
 	 trace entries.
 
-	 If unsure, say N.
+	 To make the feature actually on, set "crossrelease_fullstack"
+	 kernel parameter, too.
 
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
-- 
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 v2 0/4] Fix false positives by cross-release feature

[Byungchul Park]

I attached this patchset into another thread for patches fixing a
performance regression by cross-release, so that the cross-release can
be re-enabled easily as the last, after fixing false positives as well.

Changes from v1
- Separate a patch removing white space

Byungchul Park (4):
  completion: Add support for initializing completion with lockdep_map
  lockdep: Remove unnecessary acquisitions wrt workqueue flush
  genhd.h: Remove trailing white space
  lockdep: Assign a lock_class per gendisk used for
    wait_for_completion()

 block/bio.c                |  2 +-
 block/genhd.c              | 13 +++++--------
 include/linux/completion.h |  8 ++++++++
 include/linux/genhd.h      | 26 ++++++++++++++++++++++----
 include/linux/workqueue.h  |  4 ++--
 kernel/workqueue.c         | 20 ++++----------------
 6 files changed, 42 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH v2 0/4] Fix false positives by cross-release feature

[Byungchul Park]

I attached this patchset into another thread for patches fixing a
performance regression by cross-release, so that the cross-release can
be re-enabled easily as the last, after fixing false positives as well.

Changes from v1
- Separate a patch removing white space

Byungchul Park (4):
  completion: Add support for initializing completion with lockdep_map
  lockdep: Remove unnecessary acquisitions wrt workqueue flush
  genhd.h: Remove trailing white space
  lockdep: Assign a lock_class per gendisk used for
    wait_for_completion()

 block/bio.c                |  2 +-
 block/genhd.c              | 13 +++++--------
 include/linux/completion.h |  8 ++++++++
 include/linux/genhd.h      | 26 ++++++++++++++++++++++----
 include/linux/workqueue.h  |  4 ++--
 kernel/workqueue.c         | 20 ++++----------------
 6 files changed, 42 insertions(+), 31 deletions(-)

-- 
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 v2 1/4] completion: Add support for initializing completion with lockdep_map

[Byungchul Park]

Sometimes, we want to initialize completions with sparate lockdep maps
to assign lock classes under control. For example, the workqueue code
manages lockdep maps, as it can classify lockdep maps properly.
Provided a function for that purpose.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/completion.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index cae5400..182d56e 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -49,6 +49,13 @@ static inline void complete_release_commit(struct completion *x)
 	lock_commit_crosslock((struct lockdep_map *)&x->map);
 }
 
+#define init_completion_with_map(x, m)					\
+do {									\
+	lockdep_init_map_crosslock((struct lockdep_map *)&(x)->map,	\
+			(m)->name, (m)->key, 0);				\
+	__init_completion(x);						\
+} while (0)
+
 #define init_completion(x)						\
 do {									\
 	static struct lock_class_key __key;				\
@@ -58,6 +65,7 @@ static inline void complete_release_commit(struct completion *x)
 	__init_completion(x);						\
 } while (0)
 #else
+#define init_completion_with_map(x, m) __init_completion(x)
 #define init_completion(x) __init_completion(x)
 static inline void complete_acquire(struct completion *x) {}
 static inline void complete_release(struct completion *x) {}
-- 
1.9.1

[PATCH v2 1/4] completion: Add support for initializing completion with lockdep_map

[Byungchul Park]

Sometimes, we want to initialize completions with sparate lockdep maps
to assign lock classes under control. For example, the workqueue code
manages lockdep maps, as it can classify lockdep maps properly.
Provided a function for that purpose.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/completion.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index cae5400..182d56e 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -49,6 +49,13 @@ static inline void complete_release_commit(struct completion *x)
 	lock_commit_crosslock((struct lockdep_map *)&x->map);
 }
 
+#define init_completion_with_map(x, m)					\
+do {									\
+	lockdep_init_map_crosslock((struct lockdep_map *)&(x)->map,	\
+			(m)->name, (m)->key, 0);				\
+	__init_completion(x);						\
+} while (0)
+
 #define init_completion(x)						\
 do {									\
 	static struct lock_class_key __key;				\
@@ -58,6 +65,7 @@ static inline void complete_release_commit(struct completion *x)
 	__init_completion(x);						\
 } while (0)
 #else
+#define init_completion_with_map(x, m) __init_completion(x)
 #define init_completion(x) __init_completion(x)
 static inline void complete_acquire(struct completion *x) {}
 static inline void complete_release(struct completion *x) {}
-- 
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 v2 2/4] lockdep: Remove unnecessary acquisitions wrt workqueue flush

[Byungchul Park]

The workqueue added manual acquisitions to catch deadlock cases.
Now crossrelease was introduced, some of those are redundant, since
wait_for_completion() already includes the acquisition for itself.
Removed it.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/workqueue.h |  4 ++--
 kernel/workqueue.c        | 20 ++++----------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index f3c47a0..1455b5e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -218,7 +218,7 @@ static inline void destroy_delayed_work_on_stack(struct delayed_work *work) { }
 									\
 		__init_work((_work), _onstack);				\
 		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
-		lockdep_init_map(&(_work)->lockdep_map, #_work, &__key, 0); \
+		lockdep_init_map(&(_work)->lockdep_map, "(complete)"#_work, &__key, 0); \
 		INIT_LIST_HEAD(&(_work)->entry);			\
 		(_work)->func = (_func);				\
 	} while (0)
@@ -399,7 +399,7 @@ enum {
 	static struct lock_class_key __key;				\
 	const char *__lock_name;					\
 									\
-	__lock_name = #fmt#args;					\
+	__lock_name = "(complete)"#fmt#args;				\
 									\
 	__alloc_workqueue_key((fmt), (flags), (max_active),		\
 			      &__key, __lock_name, ##args);		\
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c77fdf6..8cef533 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2496,15 +2496,8 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
 	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
 
-	/*
-	 * Explicitly init the crosslock for wq_barrier::done, make its lock
-	 * key a subkey of the corresponding work. As a result we won't
-	 * build a dependency between wq_barrier::done and unrelated work.
-	 */
-	lockdep_init_map_crosslock((struct lockdep_map *)&barr->done.map,
-				   "(complete)wq_barr::done",
-				   target->lockdep_map.key, 1);
-	__init_completion(&barr->done);
+	init_completion_with_map(&barr->done, &target->lockdep_map);
+
 	barr->task = current;
 
 	/*
@@ -2610,16 +2603,14 @@ void flush_workqueue(struct workqueue_struct *wq)
 	struct wq_flusher this_flusher = {
 		.list = LIST_HEAD_INIT(this_flusher.list),
 		.flush_color = -1,
-		.done = COMPLETION_INITIALIZER_ONSTACK(this_flusher.done),
 	};
 	int next_color;
 
+	init_completion_with_map(&this_flusher.done, &wq->lockdep_map);
+
 	if (WARN_ON(!wq_online))
 		return;
 
-	lock_map_acquire(&wq->lockdep_map);
-	lock_map_release(&wq->lockdep_map);
-
 	mutex_lock(&wq->mutex);
 
 	/*
@@ -2882,9 +2873,6 @@ bool flush_work(struct work_struct *work)
 	if (WARN_ON(!wq_online))
 		return false;
 
-	lock_map_acquire(&work->lockdep_map);
-	lock_map_release(&work->lockdep_map);
-
 	if (start_flush_work(work, &barr)) {
 		wait_for_completion(&barr.done);
 		destroy_work_on_stack(&barr.work);
-- 
1.9.1

[PATCH v2 2/4] lockdep: Remove unnecessary acquisitions wrt workqueue flush

[Byungchul Park]

The workqueue added manual acquisitions to catch deadlock cases.
Now crossrelease was introduced, some of those are redundant, since
wait_for_completion() already includes the acquisition for itself.
Removed it.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/workqueue.h |  4 ++--
 kernel/workqueue.c        | 20 ++++----------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index f3c47a0..1455b5e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -218,7 +218,7 @@ static inline void destroy_delayed_work_on_stack(struct delayed_work *work) { }
 									\
 		__init_work((_work), _onstack);				\
 		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
-		lockdep_init_map(&(_work)->lockdep_map, #_work, &__key, 0); \
+		lockdep_init_map(&(_work)->lockdep_map, "(complete)"#_work, &__key, 0); \
 		INIT_LIST_HEAD(&(_work)->entry);			\
 		(_work)->func = (_func);				\
 	} while (0)
@@ -399,7 +399,7 @@ enum {
 	static struct lock_class_key __key;				\
 	const char *__lock_name;					\
 									\
-	__lock_name = #fmt#args;					\
+	__lock_name = "(complete)"#fmt#args;				\
 									\
 	__alloc_workqueue_key((fmt), (flags), (max_active),		\
 			      &__key, __lock_name, ##args);		\
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c77fdf6..8cef533 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2496,15 +2496,8 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
 	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
 
-	/*
-	 * Explicitly init the crosslock for wq_barrier::done, make its lock
-	 * key a subkey of the corresponding work. As a result we won't
-	 * build a dependency between wq_barrier::done and unrelated work.
-	 */
-	lockdep_init_map_crosslock((struct lockdep_map *)&barr->done.map,
-				   "(complete)wq_barr::done",
-				   target->lockdep_map.key, 1);
-	__init_completion(&barr->done);
+	init_completion_with_map(&barr->done, &target->lockdep_map);
+
 	barr->task = current;
 
 	/*
@@ -2610,16 +2603,14 @@ void flush_workqueue(struct workqueue_struct *wq)
 	struct wq_flusher this_flusher = {
 		.list = LIST_HEAD_INIT(this_flusher.list),
 		.flush_color = -1,
-		.done = COMPLETION_INITIALIZER_ONSTACK(this_flusher.done),
 	};
 	int next_color;
 
+	init_completion_with_map(&this_flusher.done, &wq->lockdep_map);
+
 	if (WARN_ON(!wq_online))
 		return;
 
-	lock_map_acquire(&wq->lockdep_map);
-	lock_map_release(&wq->lockdep_map);
-
 	mutex_lock(&wq->mutex);
 
 	/*
@@ -2882,9 +2873,6 @@ bool flush_work(struct work_struct *work)
 	if (WARN_ON(!wq_online))
 		return false;
 
-	lock_map_acquire(&work->lockdep_map);
-	lock_map_release(&work->lockdep_map);
-
 	if (start_flush_work(work, &barr)) {
 		wait_for_completion(&barr.done);
 		destroy_work_on_stack(&barr.work);
-- 
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 v2 3/4] genhd.h: Remove trailing white space

[Byungchul Park]

Trailing white space is not accepted in kernel coding style. Remove
them.

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

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index ea652bf..6d85a75 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -3,7 +3,7 @@
 
 /*
  * 	genhd.h Copyright (C) 1992 Drew Eckhardt
- *	Generic hard disk header file by  
+ *	Generic hard disk header file by
  * 		Drew Eckhardt
  *
  *		<drew@colorado.edu>
@@ -471,7 +471,7 @@ struct bsd_disklabel {
 	__s16	d_type;			/* drive type */
 	__s16	d_subtype;		/* controller/d_type specific */
 	char	d_typename[16];		/* type name, e.g. "eagle" */
-	char	d_packname[16];			/* pack identifier */ 
+	char	d_packname[16];			/* pack identifier */
 	__u32	d_secsize;		/* # of bytes per sector */
 	__u32	d_nsectors;		/* # of data sectors per track */
 	__u32	d_ntracks;		/* # of tracks per cylinder */
-- 
1.9.1

[PATCH v2 3/4] genhd.h: Remove trailing white space

[Byungchul Park]

Trailing white space is not accepted in kernel coding style. Remove
them.

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

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index ea652bf..6d85a75 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -3,7 +3,7 @@
 
 /*
  * 	genhd.h Copyright (C) 1992 Drew Eckhardt
- *	Generic hard disk header file by  
+ *	Generic hard disk header file by
  * 		Drew Eckhardt
  *
  *		<drew@colorado.edu>
@@ -471,7 +471,7 @@ struct bsd_disklabel {
 	__s16	d_type;			/* drive type */
 	__s16	d_subtype;		/* controller/d_type specific */
 	char	d_typename[16];		/* type name, e.g. "eagle" */
-	char	d_packname[16];			/* pack identifier */ 
+	char	d_packname[16];			/* pack identifier */
 	__u32	d_secsize;		/* # of bytes per sector */
 	__u32	d_nsectors;		/* # of data sectors per track */
 	__u32	d_ntracks;		/* # of tracks per cylinder */
-- 
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

Darrick and Dave Chinner posted the following warning:

> ======================================================
> WARNING: possible circular locking dependency detected
> 4.14.0-rc1-fixes #1 Tainted: G        W
> ------------------------------------------------------
> loop0/31693 is trying to acquire lock:
>  (&(&ip->i_mmaplock)->mr_lock){++++}, at: [<ffffffffa00f1b0c>] xfs_ilock+0x23c/0x330 [xfs]
>
> but now in release context of a crosslock acquired at the following:
>  ((complete)&ret.event){+.+.}, at: [<ffffffff81326c1f>] submit_bio_wait+0x7f/0xb0
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 ((complete)&ret.event){+.+.}:
>        lock_acquire+0xab/0x200
>        wait_for_completion_io+0x4e/0x1a0
>        submit_bio_wait+0x7f/0xb0
>        blkdev_issue_zeroout+0x71/0xa0
>        xfs_bmapi_convert_unwritten+0x11f/0x1d0 [xfs]
>        xfs_bmapi_write+0x374/0x11f0 [xfs]
>        xfs_iomap_write_direct+0x2ac/0x430 [xfs]
>        xfs_file_iomap_begin+0x20d/0xd50 [xfs]
>        iomap_apply+0x43/0xe0
>        dax_iomap_rw+0x89/0xf0
>        xfs_file_dax_write+0xcc/0x220 [xfs]
>        xfs_file_write_iter+0xf0/0x130 [xfs]
>        __vfs_write+0xd9/0x150
>        vfs_write+0xc8/0x1c0
>        SyS_write+0x45/0xa0
>        entry_SYSCALL_64_fastpath+0x1f/0xbe
>
> -> #1 (&xfs_nondir_ilock_class){++++}:
>        lock_acquire+0xab/0x200
>        down_write_nested+0x4a/0xb0
>        xfs_ilock+0x263/0x330 [xfs]
>        xfs_setattr_size+0x152/0x370 [xfs]
>        xfs_vn_setattr+0x6b/0x90 [xfs]
>        notify_change+0x27d/0x3f0
>        do_truncate+0x5b/0x90
>        path_openat+0x237/0xa90
>        do_filp_open+0x8a/0xf0
>        do_sys_open+0x11c/0x1f0
>        entry_SYSCALL_64_fastpath+0x1f/0xbe
>
> -> #0 (&(&ip->i_mmaplock)->mr_lock){++++}:
>        up_write+0x1c/0x40
>        xfs_iunlock+0x1d0/0x310 [xfs]
>        xfs_file_fallocate+0x8a/0x310 [xfs]
>        loop_queue_work+0xb7/0x8d0
>        kthread_worker_fn+0xb9/0x1f0
>
> Chain exists of:
>   &(&ip->i_mmaplock)->mr_lock --> &xfs_nondir_ilock_class --> (complete)&ret.event
>
>  Possible unsafe locking scenario by crosslock:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&xfs_nondir_ilock_class);
>   lock((complete)&ret.event);
>                                lock(&(&ip->i_mmaplock)->mr_lock);
>                                unlock((complete)&ret.event);
>
>                *** DEADLOCK ***

The warning is a false positive, caused by the fact that all
wait_for_completion()s in submit_bio_wait() are waiting with the same
lock class.

However, some bios have nothing to do with others, for example, the case
might happen while using loop devices, between bios of an upper device
and a lower device(=loop device).

The safest way to assign different lock classes to different devices is
to do it for each gendisk. In other words, this patch assigns a
lockdep_map per gendisk and uses it when initializing completion in
submit_bio_wait().

Of course, it might be too conservative. But, making it safest for now
and extended by block layer experts later is good, atm.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 block/bio.c           |  2 +-
 block/genhd.c         | 13 +++++--------
 include/linux/genhd.h | 22 ++++++++++++++++++++--
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 101c2a9..6dd640d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
 {
 	struct submit_bio_ret ret;
 
-	init_completion(&ret.event);
+	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
 	bio->bi_private = &ret;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
diff --git a/block/genhd.c b/block/genhd.c
index dd305c6..f195d22 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1354,13 +1354,7 @@ dev_t blk_lookup_devt(const char *name, int partno)
 }
 EXPORT_SYMBOL(blk_lookup_devt);
 
-struct gendisk *alloc_disk(int minors)
-{
-	return alloc_disk_node(minors, NUMA_NO_NODE);
-}
-EXPORT_SYMBOL(alloc_disk);
-
-struct gendisk *alloc_disk_node(int minors, int node_id)
+struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name)
 {
 	struct gendisk *disk;
 	struct disk_part_tbl *ptbl;
@@ -1409,9 +1403,12 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
 		disk_to_dev(disk)->type = &disk_type;
 		device_initialize(disk_to_dev(disk));
 	}
+
+	lockdep_init_map(&disk->lockdep_map, lock_name, key, 0);
+
 	return disk;
 }
-EXPORT_SYMBOL(alloc_disk_node);
+EXPORT_SYMBOL(__alloc_disk_node);
 
 struct kobject *get_disk(struct gendisk *disk)
 {
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 6d85a75..9832e3c 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -206,6 +206,9 @@ struct gendisk {
 #endif	/* CONFIG_BLK_DEV_INTEGRITY */
 	int node_id;
 	struct badblocks *bb;
+#ifdef CONFIG_LOCKDEP_COMPLETIONS
+	struct lockdep_map lockdep_map;
+#endif
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
@@ -590,8 +593,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);
 
-extern struct gendisk *alloc_disk_node(int minors, int node_id);
-extern struct gendisk *alloc_disk(int minors);
+extern struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name);
 extern struct kobject *get_disk(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
 extern void blk_register_region(dev_t devt, unsigned long range,
@@ -615,6 +617,22 @@ extern ssize_t part_fail_store(struct device *dev,
 			       const char *buf, size_t count);
 #endif /* CONFIG_FAIL_MAKE_REQUEST */
 
+#ifdef CONFIG_LOCKDEP_COMPLETIONS
+#define alloc_disk_node(m, id) \
+({									\
+	static struct lock_class_key __key;				\
+	const char *__lock_name;					\
+									\
+	__lock_name = "(complete)"#m"("#id")";				\
+									\
+	__alloc_disk_node(m, id, &__key, __lock_name);			\
+})
+#else
+#define alloc_disk_node(m, id)	__alloc_disk_node(m, id, NULL, NULL)
+#endif
+
+#define alloc_disk(m)		alloc_disk_node(m, NUMA_NO_NODE)
+
 static inline int hd_ref_init(struct hd_struct *part)
 {
 	if (percpu_ref_init(&part->ref, __delete_partition, 0,
-- 
1.9.1

[PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

Darrick and Dave Chinner posted the following warning:

> ======================================================
> WARNING: possible circular locking dependency detected
> 4.14.0-rc1-fixes #1 Tainted: G        W
> ------------------------------------------------------
> loop0/31693 is trying to acquire lock:
>  (&(&ip->i_mmaplock)->mr_lock){++++}, at: [<ffffffffa00f1b0c>] xfs_ilock+0x23c/0x330 [xfs]
>
> but now in release context of a crosslock acquired at the following:
>  ((complete)&ret.event){+.+.}, at: [<ffffffff81326c1f>] submit_bio_wait+0x7f/0xb0
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 ((complete)&ret.event){+.+.}:
>        lock_acquire+0xab/0x200
>        wait_for_completion_io+0x4e/0x1a0
>        submit_bio_wait+0x7f/0xb0
>        blkdev_issue_zeroout+0x71/0xa0
>        xfs_bmapi_convert_unwritten+0x11f/0x1d0 [xfs]
>        xfs_bmapi_write+0x374/0x11f0 [xfs]
>        xfs_iomap_write_direct+0x2ac/0x430 [xfs]
>        xfs_file_iomap_begin+0x20d/0xd50 [xfs]
>        iomap_apply+0x43/0xe0
>        dax_iomap_rw+0x89/0xf0
>        xfs_file_dax_write+0xcc/0x220 [xfs]
>        xfs_file_write_iter+0xf0/0x130 [xfs]
>        __vfs_write+0xd9/0x150
>        vfs_write+0xc8/0x1c0
>        SyS_write+0x45/0xa0
>        entry_SYSCALL_64_fastpath+0x1f/0xbe
>
> -> #1 (&xfs_nondir_ilock_class){++++}:
>        lock_acquire+0xab/0x200
>        down_write_nested+0x4a/0xb0
>        xfs_ilock+0x263/0x330 [xfs]
>        xfs_setattr_size+0x152/0x370 [xfs]
>        xfs_vn_setattr+0x6b/0x90 [xfs]
>        notify_change+0x27d/0x3f0
>        do_truncate+0x5b/0x90
>        path_openat+0x237/0xa90
>        do_filp_open+0x8a/0xf0
>        do_sys_open+0x11c/0x1f0
>        entry_SYSCALL_64_fastpath+0x1f/0xbe
>
> -> #0 (&(&ip->i_mmaplock)->mr_lock){++++}:
>        up_write+0x1c/0x40
>        xfs_iunlock+0x1d0/0x310 [xfs]
>        xfs_file_fallocate+0x8a/0x310 [xfs]
>        loop_queue_work+0xb7/0x8d0
>        kthread_worker_fn+0xb9/0x1f0
>
> Chain exists of:
>   &(&ip->i_mmaplock)->mr_lock --> &xfs_nondir_ilock_class --> (complete)&ret.event
>
>  Possible unsafe locking scenario by crosslock:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&xfs_nondir_ilock_class);
>   lock((complete)&ret.event);
>                                lock(&(&ip->i_mmaplock)->mr_lock);
>                                unlock((complete)&ret.event);
>
>                *** DEADLOCK ***

The warning is a false positive, caused by the fact that all
wait_for_completion()s in submit_bio_wait() are waiting with the same
lock class.

However, some bios have nothing to do with others, for example, the case
might happen while using loop devices, between bios of an upper device
and a lower device(=loop device).

The safest way to assign different lock classes to different devices is
to do it for each gendisk. In other words, this patch assigns a
lockdep_map per gendisk and uses it when initializing completion in
submit_bio_wait().

Of course, it might be too conservative. But, making it safest for now
and extended by block layer experts later is good, atm.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 block/bio.c           |  2 +-
 block/genhd.c         | 13 +++++--------
 include/linux/genhd.h | 22 ++++++++++++++++++++--
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 101c2a9..6dd640d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
 {
 	struct submit_bio_ret ret;
 
-	init_completion(&ret.event);
+	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
 	bio->bi_private = &ret;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
diff --git a/block/genhd.c b/block/genhd.c
index dd305c6..f195d22 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1354,13 +1354,7 @@ dev_t blk_lookup_devt(const char *name, int partno)
 }
 EXPORT_SYMBOL(blk_lookup_devt);
 
-struct gendisk *alloc_disk(int minors)
-{
-	return alloc_disk_node(minors, NUMA_NO_NODE);
-}
-EXPORT_SYMBOL(alloc_disk);
-
-struct gendisk *alloc_disk_node(int minors, int node_id)
+struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name)
 {
 	struct gendisk *disk;
 	struct disk_part_tbl *ptbl;
@@ -1409,9 +1403,12 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
 		disk_to_dev(disk)->type = &disk_type;
 		device_initialize(disk_to_dev(disk));
 	}
+
+	lockdep_init_map(&disk->lockdep_map, lock_name, key, 0);
+
 	return disk;
 }
-EXPORT_SYMBOL(alloc_disk_node);
+EXPORT_SYMBOL(__alloc_disk_node);
 
 struct kobject *get_disk(struct gendisk *disk)
 {
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 6d85a75..9832e3c 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -206,6 +206,9 @@ struct gendisk {
 #endif	/* CONFIG_BLK_DEV_INTEGRITY */
 	int node_id;
 	struct badblocks *bb;
+#ifdef CONFIG_LOCKDEP_COMPLETIONS
+	struct lockdep_map lockdep_map;
+#endif
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
@@ -590,8 +593,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);
 
-extern struct gendisk *alloc_disk_node(int minors, int node_id);
-extern struct gendisk *alloc_disk(int minors);
+extern struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name);
 extern struct kobject *get_disk(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
 extern void blk_register_region(dev_t devt, unsigned long range,
@@ -615,6 +617,22 @@ extern ssize_t part_fail_store(struct device *dev,
 			       const char *buf, size_t count);
 #endif /* CONFIG_FAIL_MAKE_REQUEST */
 
+#ifdef CONFIG_LOCKDEP_COMPLETIONS
+#define alloc_disk_node(m, id) \
+({									\
+	static struct lock_class_key __key;				\
+	const char *__lock_name;					\
+									\
+	__lock_name = "(complete)"#m"("#id")";				\
+									\
+	__alloc_disk_node(m, id, &__key, __lock_name);			\
+})
+#else
+#define alloc_disk_node(m, id)	__alloc_disk_node(m, id, NULL, NULL)
+#endif
+
+#define alloc_disk(m)		alloc_disk_node(m, NUMA_NO_NODE)
+
 static inline int hd_ref_init(struct hd_struct *part)
 {
 	if (percpu_ref_init(&part->ref, __delete_partition, 0,
-- 
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Christoph Hellwig]

The Subject prefix for this should be "block:".

> @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
>  {
>  	struct submit_bio_ret ret;
>  
> -	init_completion(&ret.event);
> +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);

FYI, I have an outstanding patch to simplify this a lot, which
switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
you pick it up with your series, but we'll need a variant of
DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Patch below for reference:

---
>From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 5 Oct 2017 18:31:02 +0200
Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait

Simplify the code by getting rid of the submit_bio_ret structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 8338304ea256..4e18e959fc0a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -917,17 +917,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 }
 EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
 
-struct submit_bio_ret {
-	struct completion event;
-	int error;
-};
-
 static void submit_bio_wait_endio(struct bio *bio)
 {
-	struct submit_bio_ret *ret = bio->bi_private;
-
-	ret->error = blk_status_to_errno(bio->bi_status);
-	complete(&ret->event);
+	complete(bio->bi_private);
 }
 
 /**
@@ -943,16 +935,15 @@ static void submit_bio_wait_endio(struct bio *bio)
  */
 int submit_bio_wait(struct bio *bio)
 {
-	struct submit_bio_ret ret;
+	DECLARE_COMPLETION_ONSTACK(done);
 
-	init_completion(&ret.event);
-	bio->bi_private = &ret;
+	bio->bi_private = &done;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
-	wait_for_completion_io(&ret.event);
+	wait_for_completion_io(&done);
 
-	return ret.error;
+	return blk_status_to_errno(bio->bi_status);
 }
 EXPORT_SYMBOL(submit_bio_wait);
 
-- 
2.14.2

Re: [PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Christoph Hellwig]

The Subject prefix for this should be "block:".

> @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
>  {
>  	struct submit_bio_ret ret;
>  
> -	init_completion(&ret.event);
> +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);

FYI, I have an outstanding patch to simplify this a lot, which
switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
you pick it up with your series, but we'll need a variant of
DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Patch below for reference:

---

Re: [PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Christoph Hellwig]

The Subject prefix for this should be "block:".

> @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
>  {
>  	struct submit_bio_ret ret;
>  
> -	init_completion(&ret.event);
> +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);

FYI, I have an outstanding patch to simplify this a lot, which
switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
you pick it up with your series, but we'll need a variant of
DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Patch below for reference:

---
>From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 5 Oct 2017 18:31:02 +0200
Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait

Simplify the code by getting rid of the submit_bio_ret structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 8338304ea256..4e18e959fc0a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -917,17 +917,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 }
 EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
 
-struct submit_bio_ret {
-	struct completion event;
-	int error;
-};
-
 static void submit_bio_wait_endio(struct bio *bio)
 {
-	struct submit_bio_ret *ret = bio->bi_private;
-
-	ret->error = blk_status_to_errno(bio->bi_status);
-	complete(&ret->event);
+	complete(bio->bi_private);
 }
 
 /**
@@ -943,16 +935,15 @@ static void submit_bio_wait_endio(struct bio *bio)
  */
 int submit_bio_wait(struct bio *bio)
 {
-	struct submit_bio_ret ret;
+	DECLARE_COMPLETION_ONSTACK(done);
 
-	init_completion(&ret.event);
-	bio->bi_private = &ret;
+	bio->bi_private = &done;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
-	wait_for_completion_io(&ret.event);
+	wait_for_completion_io(&done);
 
-	return ret.error;
+	return blk_status_to_errno(bio->bi_status);
 }
 EXPORT_SYMBOL(submit_bio_wait);
 
-- 
2.14.2

--
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> The Subject prefix for this should be "block:".
> 
> > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> >  {
> >  	struct submit_bio_ret ret;
> >  
> > -	init_completion(&ret.event);
> > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> 
> FYI, I have an outstanding patch to simplify this a lot, which
> switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> you pick it up with your series, but we'll need a variant of
> DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Hello,

I'm sorry for late.

I think your patch makes block code simpler and better. I like it.

But, I just wonder if it's related to my series. Is it proper to add
your patch into my series?

Thanks,
Byungchul

> Patch below for reference:
> 
> ---
> >From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 5 Oct 2017 18:31:02 +0200
> Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait
> 
> Simplify the code by getting rid of the submit_bio_ret structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 8338304ea256..4e18e959fc0a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -917,17 +917,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  }
>  EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
>  
> -struct submit_bio_ret {
> -	struct completion event;
> -	int error;
> -};
> -
>  static void submit_bio_wait_endio(struct bio *bio)
>  {
> -	struct submit_bio_ret *ret = bio->bi_private;
> -
> -	ret->error = blk_status_to_errno(bio->bi_status);
> -	complete(&ret->event);
> +	complete(bio->bi_private);
>  }
>  
>  /**
> @@ -943,16 +935,15 @@ static void submit_bio_wait_endio(struct bio *bio)
>   */
>  int submit_bio_wait(struct bio *bio)
>  {
> -	struct submit_bio_ret ret;
> +	DECLARE_COMPLETION_ONSTACK(done);
>  
> -	init_completion(&ret.event);
> -	bio->bi_private = &ret;
> +	bio->bi_private = &done;
>  	bio->bi_end_io = submit_bio_wait_endio;
>  	bio->bi_opf |= REQ_SYNC;
>  	submit_bio(bio);
> -	wait_for_completion_io(&ret.event);
> +	wait_for_completion_io(&done);
>  
> -	return ret.error;
> +	return blk_status_to_errno(bio->bi_status);
>  }
>  EXPORT_SYMBOL(submit_bio_wait);
>  
> -- 
> 2.14.2

Re: [PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> The Subject prefix for this should be "block:".
> 
> > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> >  {
> >  	struct submit_bio_ret ret;
> >  
> > -	init_completion(&ret.event);
> > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> 
> FYI, I have an outstanding patch to simplify this a lot, which
> switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> you pick it up with your series, but we'll need a variant of
> DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Hello,

I'm sorry for late.

I think your patch makes block code simpler and better. I like it.

But, I just wonder if it's related to my series. Is it proper to add
your patch into my series?

Thanks,
Byungchul

> Patch below for reference:
> 
> ---
> >From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 5 Oct 2017 18:31:02 +0200
> Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait
> 
> Simplify the code by getting rid of the submit_bio_ret structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 8338304ea256..4e18e959fc0a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -917,17 +917,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  }
>  EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
>  
> -struct submit_bio_ret {
> -	struct completion event;
> -	int error;
> -};
> -
>  static void submit_bio_wait_endio(struct bio *bio)
>  {
> -	struct submit_bio_ret *ret = bio->bi_private;
> -
> -	ret->error = blk_status_to_errno(bio->bi_status);
> -	complete(&ret->event);
> +	complete(bio->bi_private);
>  }
>  
>  /**
> @@ -943,16 +935,15 @@ static void submit_bio_wait_endio(struct bio *bio)
>   */
>  int submit_bio_wait(struct bio *bio)
>  {
> -	struct submit_bio_ret ret;
> +	DECLARE_COMPLETION_ONSTACK(done);
>  
> -	init_completion(&ret.event);
> -	bio->bi_private = &ret;
> +	bio->bi_private = &done;
>  	bio->bi_end_io = submit_bio_wait_endio;
>  	bio->bi_opf |= REQ_SYNC;
>  	submit_bio(bio);
> -	wait_for_completion_io(&ret.event);
> +	wait_for_completion_io(&done);
>  
> -	return ret.error;
> +	return blk_status_to_errno(bio->bi_status);
>  }
>  EXPORT_SYMBOL(submit_bio_wait);
>  
> -- 
> 2.14.2

--
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Christoph Hellwig]

On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > The Subject prefix for this should be "block:".
> > 
> > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > >  {
> > >  	struct submit_bio_ret ret;
> > >  
> > > -	init_completion(&ret.event);
> > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > 
> > FYI, I have an outstanding patch to simplify this a lot, which
> > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > you pick it up with your series, but we'll need a variant of
> > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> 
> Hello,
> 
> I'm sorry for late.
> 
> I think your patch makes block code simpler and better. I like it.
> 
> But, I just wonder if it's related to my series.

Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
the gets passed an explicit lockdep map.  And because if it was merged
through a different tree it would create a conflict.

> Is it proper to add
> your patch into my series?

Sure.

Re: [PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Christoph Hellwig]

On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > The Subject prefix for this should be "block:".
> > 
> > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > >  {
> > >  	struct submit_bio_ret ret;
> > >  
> > > -	init_completion(&ret.event);
> > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > 
> > FYI, I have an outstanding patch to simplify this a lot, which
> > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > you pick it up with your series, but we'll need a variant of
> > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> 
> Hello,
> 
> I'm sorry for late.
> 
> I think your patch makes block code simpler and better. I like it.
> 
> But, I just wonder if it's related to my series.

Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
the gets passed an explicit lockdep map.  And because if it was merged
through a different tree it would create a conflict.

> Is it proper to add
> your patch into my series?

Sure.

--
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

On Sun, Oct 22, 2017 at 11:36:30PM -0700, Christoph Hellwig wrote:
> On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> > On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > > The Subject prefix for this should be "block:".
> > > 
> > > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > > >  {
> > > >  	struct submit_bio_ret ret;
> > > >  
> > > > -	init_completion(&ret.event);
> > > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > > 
> > > FYI, I have an outstanding patch to simplify this a lot, which
> > > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > > you pick it up with your series, but we'll need a variant of
> > > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> > 
> > Hello,
> > 
> > I'm sorry for late.
> > 
> > I think your patch makes block code simpler and better. I like it.
> > 
> > But, I just wonder if it's related to my series.
> 
> Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
> the gets passed an explicit lockdep map.  And because if it was merged
> through a different tree it would create a conflict.
> 
> > Is it proper to add
> > your patch into my series?
> 
> Sure.

I will add yours at the next spin.

Thank you.

BTW, to all...

Any additional opinions about these patches?

Re: [PATCH v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[Byungchul Park]

On Sun, Oct 22, 2017 at 11:36:30PM -0700, Christoph Hellwig wrote:
> On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> > On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > > The Subject prefix for this should be "block:".
> > > 
> > > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > > >  {
> > > >  	struct submit_bio_ret ret;
> > > >  
> > > > -	init_completion(&ret.event);
> > > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > > 
> > > FYI, I have an outstanding patch to simplify this a lot, which
> > > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > > you pick it up with your series, but we'll need a variant of
> > > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> > 
> > Hello,
> > 
> > I'm sorry for late.
> > 
> > I think your patch makes block code simpler and better. I like it.
> > 
> > But, I just wonder if it's related to my series.
> 
> Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
> the gets passed an explicit lockdep map.  And because if it was merged
> through a different tree it would create a conflict.
> 
> > Is it proper to add
> > your patch into my series?
> 
> Sure.

I will add yours at the next spin.

Thank you.

BTW, to all...

Any additional opinions about these patches?

--
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 v2 4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hi Byungchul,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc5 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Byungchul-Park/Fix-false-positives-by-cross-release-feature/20171022-022121
config: i386-randconfig-x002-201743 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   block/genhd.c: In function '__alloc_disk_node':
>> block/genhd.c:1407:24: error: 'struct gendisk' has no member named 'lockdep_map'
     lockdep_init_map(&disk->lockdep_map, lock_name, key, 0);
                           ^~

vim +1407 block/genhd.c

  1356	
  1357	struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name)
  1358	{
  1359		struct gendisk *disk;
  1360		struct disk_part_tbl *ptbl;
  1361	
  1362		if (minors > DISK_MAX_PARTS) {
  1363			printk(KERN_ERR
  1364				"block: can't allocated more than %d partitions\n",
  1365				DISK_MAX_PARTS);
  1366			minors = DISK_MAX_PARTS;
  1367		}
  1368	
  1369		disk = kzalloc_node(sizeof(struct gendisk), GFP_KERNEL, node_id);
  1370		if (disk) {
  1371			if (!init_part_stats(&disk->part0)) {
  1372				kfree(disk);
  1373				return NULL;
  1374			}
  1375			disk->node_id = node_id;
  1376			if (disk_expand_part_tbl(disk, 0)) {
  1377				free_part_stats(&disk->part0);
  1378				kfree(disk);
  1379				return NULL;
  1380			}
  1381			ptbl = rcu_dereference_protected(disk->part_tbl, 1);
  1382			rcu_assign_pointer(ptbl->part[0], &disk->part0);
  1383	
  1384			/*
  1385			 * set_capacity() and get_capacity() currently don't use
  1386			 * seqcounter to read/update the part0->nr_sects. Still init
  1387			 * the counter as we can read the sectors in IO submission
  1388			 * patch using seqence counters.
  1389			 *
  1390			 * TODO: Ideally set_capacity() and get_capacity() should be
  1391			 * converted to make use of bd_mutex and sequence counters.
  1392			 */
  1393			seqcount_init(&disk->part0.nr_sects_seq);
  1394			if (hd_ref_init(&disk->part0)) {
  1395				hd_free_part(&disk->part0);
  1396				kfree(disk);
  1397				return NULL;
  1398			}
  1399	
  1400			disk->minors = minors;
  1401			rand_initialize_disk(disk);
  1402			disk_to_dev(disk)->class = &block_class;
  1403			disk_to_dev(disk)->type = &disk_type;
  1404			device_initialize(disk_to_dev(disk));
  1405		}
  1406	
> 1407		lockdep_init_map(&disk->lockdep_map, lock_name, key, 0);
  1408	
  1409		return disk;
  1410	}
  1411	EXPORT_SYMBOL(__alloc_disk_node);
  1412	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26642 bytes --]