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

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

[Byungchul Park]

To peter,

Does it work?

Chagnes from v1
 - Add a completion initialization function with a lockdep map
 - Enhance readability of the workqueue code

----->8-----
>From e148617e20ebc9c9eefe7bb222b9bba07cb963bc Mon Sep 17 00:00:00 2001
From: Byungchul Park <byungchul.park@lge.com>
Date: Fri, 8 Sep 2017 17:39:48 +0900
Subject: [PATCH v2 1/2] completion: Add support for initializing completion
 with lockdep_map

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 2/2] 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 db6dc9d..1bef13e 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)
@@ -398,7 +398,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 ab3c0dc..72f68b1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2497,15 +2497,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;
 
 	/*
@@ -2611,16 +2604,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);
 
 	/*
@@ -2883,9 +2874,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

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

[Byungchul Park]

On Fri, Sep 08, 2017 at 05:46:24PM +0900, Byungchul Park wrote:
> To peter,
> 
> Does it work?
> 
> Chagnes from v1
>  - Add a completion initialization function with a lockdep map
>  - Enhance readability of the workqueue code

Do not you like it becasue it introduces another init_completion_xxx()?

Anyway, I think the redundant acquisitions in the workqueue code should
be removed. Right?

> ----->8-----
> >From e148617e20ebc9c9eefe7bb222b9bba07cb963bc Mon Sep 17 00:00:00 2001
> From: Byungchul Park <byungchul.park@lge.com>
> Date: Fri, 8 Sep 2017 17:39:48 +0900
> Subject: [PATCH v2 1/2] completion: Add support for initializing completion
>  with lockdep_map
> 
> 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

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

[Byungchul Park]

On Fri, Sep 08, 2017 at 05:46:25PM +0900, Byungchul Park wrote:
> 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.

Now, lock_map_acquire() is already embedded in wait_for_completion(),
manual acquisitions for the same purpose should be removed, since they
would be duplicated. Consider this, please.

> 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 db6dc9d..1bef13e 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)
> @@ -398,7 +398,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 ab3c0dc..72f68b1 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2497,15 +2497,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;
>  
>  	/*
> @@ -2611,16 +2604,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);
>  
>  	/*
> @@ -2883,9 +2874,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

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

[Byungchul Park]

On Fri, Sep 08, 2017 at 05:46:24PM +0900, Byungchul Park wrote:
> To peter,
> 
> Does it work?
> 
> Chagnes from v1
>  - Add a completion initialization function with a lockdep map
>  - Enhance readability of the workqueue code

Again, this does not introduce any new things but only does what it
should be done, now that wait_for_completion() includes its lockdep_map.

For 'might' things I introduced, please take a look at another mail
thread where I want to discuss it.

> ----->8-----
> >From e148617e20ebc9c9eefe7bb222b9bba07cb963bc Mon Sep 17 00:00:00 2001
> From: Byungchul Park <byungchul.park@lge.com>
> Date: Fri, 8 Sep 2017 17:39:48 +0900
> Subject: [PATCH v2 1/2] completion: Add support for initializing completion
>  with lockdep_map
> 
> 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