plist: add mutex to the blessed lock type for plists

PATCH In review Dima Zavin 4 participants 11 messages Jun 29, 2011 – Jul 2, 2011 Library code

Share & tools

Dima Zavin

author

2011-06-29 19:34 UTC #0 lore raw

PATCH 1/1 plist: add mutex to the blessed lock type for plists

Currently, plist debugging "enforces" that the plist is locked with either a raw_spinlock or a spinlock. The plist data structure is useful in other places, where spinlocks are unnecessary.

Extend the plist initializers and debug checks to allow the plist to be protected by a mutex

Signed-off-by Dima Zavin <[email protected]>

---
 include/linux/plist.h |   33 +++++++++++++++++++++++++++++++++
 lib/plist.c           |    6 +++++-
 2 files changed, 38 insertions(+), 1 deletions(-)

include/linux/plist.h +33 -0

index c9b9f32..bd670f6 100644
@@ -77,6 +77,7 @@
 
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/spinlock_types.h>
 
 struct plist_head {

@@ -84,6 +85,7 @@ struct plist_head {
 #ifdef CONFIG_DEBUG_PI_LIST
 	raw_spinlock_t *rawlock;
 	spinlock_t *spinlock;
+	struct mutex *mutex;
 #endif
 };

@@ -96,9 +98,11 @@ struct plist_node {
 #ifdef CONFIG_DEBUG_PI_LIST
 # define PLIST_HEAD_LOCK_INIT(_lock)		.spinlock = _lock
 # define PLIST_HEAD_LOCK_INIT_RAW(_lock)	.rawlock = _lock
+# define PLIST_HEAD_LOCK_INIT_MUTEX(_lock)	.mutex = _lock
 #else
 # define PLIST_HEAD_LOCK_INIT(_lock)
 # define PLIST_HEAD_LOCK_INIT_RAW(_lock)
+# define PLIST_HEAD_LOCK_INIT_MUTEX(_lock)
 #endif
 
 #define _PLIST_HEAD_INIT(head)				\

@@ -127,6 +131,17 @@ struct plist_node {
 }
 
 /**
+ * PLIST_HEAD_INIT_MUTEX - static struct plist_head initializer
+ * @head:	struct plist_head variable name
+ * @_lock:	lock to initialize for this list
+ */
+#define PLIST_HEAD_INIT_MUTEX(head, _lock)		\
+{							\
+	_PLIST_HEAD_INIT(head),				\
+	PLIST_HEAD_LOCK_INIT_MUTEX(&(_lock))		\
+}
+
+/**
  * PLIST_NODE_INIT - static struct plist_node initializer
  * @node:	struct plist_node variable name
  * @__prio:	initial node priority

@@ -150,6 +165,7 @@ plist_head_init(struct plist_head *head, spinlock_t *lock)
 #ifdef CONFIG_DEBUG_PI_LIST
 	head->spinlock = lock;
 	head->rawlock = NULL;
+	head->mutex = NULL;
 #endif
 }

@@ -165,6 +181,23 @@ plist_head_init_raw(struct plist_head *head, raw_spinlock_t *lock)
 #ifdef CONFIG_DEBUG_PI_LIST
 	head->rawlock = lock;
 	head->spinlock = NULL;
+	head->mutex = NULL;
+#endif
+}
+
+/**
+ * plist_head_init_mutex - dynamic struct plist_head initializer
+ * @head:	&struct plist_head pointer
+ * @lock:	mutex protecting the list (debugging)
+ */
+static inline void
+plist_head_init_mutex(struct plist_head *head, struct mutex *lock)
+{
+	INIT_LIST_HEAD(&head->node_list);
+#ifdef CONFIG_DEBUG_PI_LIST
+	head->mutex = lock;
+	head->rawlock = NULL;
+	head->spinlock = NULL;
 #endif
 }

lib/plist.c +5 -1

index 0ae7e64..64bd937 100644
@@ -23,6 +23,7 @@
  * information.
  */
 
+#include <linux/mutex.h>
 #include <linux/plist.h>
 #include <linux/spinlock.h>

@@ -56,11 +57,14 @@ static void plist_check_list(struct list_head *top)
 
 static void plist_check_head(struct plist_head *head)
 {
-	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock);
+	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock &&
+		!head->mutex);
 	if (head->rawlock)
 		WARN_ON_SMP(!raw_spin_is_locked(head->rawlock));
 	if (head->spinlock)
 		WARN_ON_SMP(!spin_is_locked(head->spinlock));
+	if (head->mutex)
+		WARN_ON_SMP(!mutex_is_locked(head->mutex));
 	if (!plist_head_empty(head))
 		plist_check_list(&plist_first(head)->prio_list);
 	plist_check_list(&head->node_list);

Signature · 1 line

1.7.3.1

Steven Rostedt

2011-06-29 20:01 UTC #1 lore raw

Quoted 7 lines · Dima Zavin

lib/plist.c +1 -0

+#include <linux/mutex.h>
 #include <linux/plist.h>
 #include <linux/spinlock.h>

@@ -56,11 +57,14 @@ static void plist_check_list(struct list_head *top)
 
 static void plist_check_head(struct plist_head *head)

lib/plist.c static void plist_check_list(struct list_head *top) · line 67

Quoted 10 lines

lib/plist.c +4 -1

 {
-	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock);
+	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock &&
+		!head->mutex);
 	if (head->rawlock)
 		WARN_ON_SMP(!raw_spin_is_locked(head->rawlock));
 	if (head->spinlock)
 		WARN_ON_SMP(!spin_is_locked(head->spinlock));
+	if (head->mutex)
+		WARN_ON_SMP(!mutex_is_locked(head->mutex));

Spin locks are NOPs on SMP, but mutexes are not. Are you sure you want this as WARN_ON_SMP()?

-- Steve

Quoted 3 lines

lib/plist.c +0 -0

 	if (!plist_head_empty(head))
 		plist_check_list(&plist_first(head)->prio_list);
 	plist_check_list(&head->node_list);

Dima Zavin

author

2011-06-29 20:10 UTC #2 lore raw

Quoted 8 lines · Steven Rostedt

Quoted 6 lines · Dima Zavin
+#include <linux/mutex.h>
#include <linux/plist.h>
#include <linux/spinlock.h>
(quoted diff) +0 -0
@@ -56,11 +57,14 @@ static void plist_check_list(struct list_head *top)
 

Quoted 14 lines

lib/plist.c static void plist_check_list(struct list_head *top) · line 67

Quoted 11 lines

lib/plist.c +4 -1

  static void plist_check_head(struct plist_head *head)
  {
-     WARN_ON(head != &test_head && !head->rawlock && !head->spinlock);
+     WARN_ON(head != &test_head && !head->rawlock && !head->spinlock &&
+             !head->mutex);
       if (head->rawlock)
               WARN_ON_SMP(!raw_spin_is_locked(head->rawlock));
       if (head->spinlock)
               WARN_ON_SMP(!spin_is_locked(head->spinlock));
+     if (head->mutex)
+             WARN_ON_SMP(!mutex_is_locked(head->mutex));

Spin locks are NOPs on SMP, but mutexes are not. Are you sure you want this as WARN_ON_SMP()?

Doh. Copy-pasty-itis.. Will switch to plain WARN_ON.

--Dima

Quoted 9 lines

-- Steve

Quoted 3 lines

lib/plist.c +0 -0

       if (!plist_head_empty(head))
               plist_check_list(&plist_first(head)->prio_list);
       plist_check_list(&head->node_list);

Dima Zavin

author

2011-06-29 20:12 UTC #3 lore raw

PATCH 1/1 plist: add mutex to the blessed lock type for plists

Currently, plist debugging "enforces" that the plist is locked with either a raw_spinlock or a spinlock. The plist data structure is useful in other places, where spinlocks are unnecessary.

Extend the plist initializers and debug checks to allow the plist to be protected by a mutex

Signed-off-by Dima Zavin <[email protected]>

---
 include/linux/plist.h |   33 +++++++++++++++++++++++++++++++++
 lib/plist.c           |    6 +++++-
 2 files changed, 38 insertions(+), 1 deletions(-)

include/linux/plist.h +33 -0

index c9b9f32..bd670f6 100644
@@ -77,6 +77,7 @@
 
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/spinlock_types.h>
 
 struct plist_head {

@@ -84,6 +85,7 @@ struct plist_head {
 #ifdef CONFIG_DEBUG_PI_LIST
 	raw_spinlock_t *rawlock;
 	spinlock_t *spinlock;
+	struct mutex *mutex;
 #endif
 };

@@ -96,9 +98,11 @@ struct plist_node {
 #ifdef CONFIG_DEBUG_PI_LIST
 # define PLIST_HEAD_LOCK_INIT(_lock)		.spinlock = _lock
 # define PLIST_HEAD_LOCK_INIT_RAW(_lock)	.rawlock = _lock
+# define PLIST_HEAD_LOCK_INIT_MUTEX(_lock)	.mutex = _lock
 #else
 # define PLIST_HEAD_LOCK_INIT(_lock)
 # define PLIST_HEAD_LOCK_INIT_RAW(_lock)
+# define PLIST_HEAD_LOCK_INIT_MUTEX(_lock)
 #endif
 
 #define _PLIST_HEAD_INIT(head)				\

@@ -127,6 +131,17 @@ struct plist_node {
 }
 
 /**
+ * PLIST_HEAD_INIT_MUTEX - static struct plist_head initializer
+ * @head:	struct plist_head variable name
+ * @_lock:	lock to initialize for this list
+ */
+#define PLIST_HEAD_INIT_MUTEX(head, _lock)		\
+{							\
+	_PLIST_HEAD_INIT(head),				\
+	PLIST_HEAD_LOCK_INIT_MUTEX(&(_lock))		\
+}
+
+/**
  * PLIST_NODE_INIT - static struct plist_node initializer
  * @node:	struct plist_node variable name
  * @__prio:	initial node priority

@@ -150,6 +165,7 @@ plist_head_init(struct plist_head *head, spinlock_t *lock)
 #ifdef CONFIG_DEBUG_PI_LIST
 	head->spinlock = lock;
 	head->rawlock = NULL;
+	head->mutex = NULL;
 #endif
 }

@@ -165,6 +181,23 @@ plist_head_init_raw(struct plist_head *head, raw_spinlock_t *lock)
 #ifdef CONFIG_DEBUG_PI_LIST
 	head->rawlock = lock;
 	head->spinlock = NULL;
+	head->mutex = NULL;
+#endif
+}
+
+/**
+ * plist_head_init_mutex - dynamic struct plist_head initializer
+ * @head:	&struct plist_head pointer
+ * @lock:	mutex protecting the list (debugging)
+ */
+static inline void
+plist_head_init_mutex(struct plist_head *head, struct mutex *lock)
+{
+	INIT_LIST_HEAD(&head->node_list);
+#ifdef CONFIG_DEBUG_PI_LIST
+	head->mutex = lock;
+	head->rawlock = NULL;
+	head->spinlock = NULL;
 #endif
 }

lib/plist.c +5 -1

index 0ae7e64..f3c2cad 100644
@@ -23,6 +23,7 @@
  * information.
  */
 
+#include <linux/mutex.h>
 #include <linux/plist.h>
 #include <linux/spinlock.h>

@@ -56,11 +57,14 @@ static void plist_check_list(struct list_head *top)
 
 static void plist_check_head(struct plist_head *head)
 {
-	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock);
+	WARN_ON(head != &test_head && !head->rawlock && !head->spinlock &&
+		!head->mutex);
 	if (head->rawlock)
 		WARN_ON_SMP(!raw_spin_is_locked(head->rawlock));
 	if (head->spinlock)
 		WARN_ON_SMP(!spin_is_locked(head->spinlock));
+	if (head->mutex)
+		WARN_ON(!mutex_is_locked(head->mutex));
 	if (!plist_head_empty(head))
 		plist_check_list(&plist_first(head)->prio_list);
 	plist_check_list(&head->node_list);

Signature · 1 line

1.7.3.1

Andi Kleen

2011-06-29 20:14 UTC #4 lore raw

In reply to Dima Zavin

Quoted 6 lines · Dima Zavin

Currently, plist debugging "enforces" that the plist is locked with either a raw_spinlock or a spinlock. The plist data structure is useful in other places, where spinlocks are unnecessary.
Extend the plist initializers and debug checks to allow the plist to be protected by a mutex

Seems really ugly and clearly not a godo path.

It's a bit like adding a 11th argument to a function which already has 10.

Perhaps better move out the locking completely to wrappers and remove the knowledge from the core plist code.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only

Dima Zavin

author

2011-06-29 20:34 UTC #5 lore raw

Quoted 9 lines · Andi Kleen

Quoted 6 lines · Dima Zavin
Currently, plist debugging "enforces" that the plist is locked with either a raw_spinlock or a spinlock. The plist data structure is useful in other places, where spinlocks are unnecessary.
Extend the plist initializers and debug checks to allow the plist to be protected by a mutex

Quoted 7 lines

Seems really ugly and clearly not a godo path.
It's a bit like adding a 11th argument to a function which already has 10.
Perhaps better move out the locking completely to wrappers and remove the knowledge from the core plist code.

Yeah, it is pretty ugly. Are you proposing adding new plist types like plist_mutex and plist_spinlock and have the initializers create the wrapper plist types? And then you would have X types, and X different functions for add and del? Unless I'm misunderstanding where you propose putting the wrappers. And then we'll have to audit all the users to know which flavors are currently being used where (raw vs spin).

The whole enforcement of locking inside this code is awkward anyway. We don't enforce locking on rb_trees, or on list_head, etc. Why plists? The funny part is that the test code in plist.c itself has a hack to skip the lock check.

--Dima

Quoted 7 lines

-Andi
--
[email protected] -- Speaking for myself only

Steven Rostedt

2011-06-29 20:43 UTC #6 lore raw

Quoted 4 lines · Dima Zavin

The whole enforcement of locking inside this code is awkward anyway. We don't enforce locking on rb_trees, or on list_head, etc. Why plists? The funny part is that the test code in plist.c itself has a hack to skip the lock check.

It's a legacy from the -rt tree. With the development there, there was always a case where a plist was added without the proper locking, and we spent days debugging it. This test proved very useful. As plists came to mainline, we kept the tests.

Now, getting rid of them maybe the thing to do. I'm not sure how useful they are today.

-- Steve

Dima Zavin

author

2011-06-30 22:14 UTC #7 lore raw

Steve,

So what would do you recommend I do? Is this patch acceptable or do you want me to remove all the debug stuff and modify all the users to not provide a lock?

--Dima

Quoted 19 lines · Steven Rostedt

Quoted 4 lines · Dima Zavin
The whole enforcement of locking inside this code is awkward anyway. We don't enforce locking on rb_trees, or on list_head, etc. Why plists? The funny part is that the test code in plist.c itself has a hack to skip the lock check.
It's a legacy from the -rt tree. With the development there, there was always a case where a plist was added without the proper locking, and we spent days debugging it. This test proved very useful. As plists came to mainline, we kept the tests.
Now, getting rid of them maybe the thing to do. I'm not sure how useful they are today.
-- Steve

Steven Rostedt

2011-06-30 22:45 UTC #8 lore raw

Quoted 6 lines · Dima Zavin

Steve,
So what would do you recommend I do? Is this patch acceptable or do you want me to remove all the debug stuff and modify all the users to not provide a lock?

I'm fine either way. I would like to know what Thomas, Ingo and Peter think.

-- Steve

Quoted 20 lines · Steven Rostedt

Quoted 19 lines · Dima Zavin
Quoted 5 lines
The whole enforcement of locking inside this code is awkward anyway. We don't enforce locking on rb_trees, or on list_head, etc. Why plists? The funny part is that the test code in plist.c itself has a hack to skip the lock check.
It's a legacy from the -rt tree. With the development there, there was always a case where a plist was added without the proper locking, and we spent days debugging it. This test proved very useful. As plists came to mainline, we kept the tests.
Now, getting rid of them maybe the thing to do. I'm not sure how useful they are today.
-- Steve

Peter Zijlstra

2011-07-02 10:11 UTC #9 lore raw

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · Dima Zavin
Steve,
So what would do you recommend I do? Is this patch acceptable or do you want me to remove all the debug stuff and modify all the users to not provide a lock?
I'm fine either way. I would like to know what Thomas, Ingo and Peter think.

I've always been annoyed with that check, just rip it out.

Steven Rostedt

2011-06-29 20:36 UTC #10 lore raw

In reply to Andi Kleen

Quoted 9 lines · Andi Kleen

Quoted 6 lines · Dima Zavin
Currently, plist debugging "enforces" that the plist is locked with either a raw_spinlock or a spinlock. The plist data structure is useful in other places, where spinlocks are unnecessary.
Extend the plist initializers and debug checks to allow the plist to be protected by a mutex

Quoted 7 lines

Seems really ugly and clearly not a godo path.
It's a bit like adding a 11th argument to a function which already has 10.
Perhaps better move out the locking completely to wrappers and remove the knowledge from the core plist code.

I don't think wrappers will help any. You still need to add some hook to let the debugging know what type of lock is protecting a plist. It is also nice annotation to see it. The code isn't that bad, and it is compiled out when DEBUG_PI_LIST is not set.

-- Steve