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(-)
diff --git a/include/linux/plist.h b/include/linux/plist.h
index c9b9f32..bd670f6 100644
--- a/include/linux/plist.h
+++ b/include/linux/plist.h
@@ -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
}
diff --git a/lib/plist.c b/lib/plist.c
index 0ae7e64..64bd937 100644
--- a/lib/plist.c
+++ b/lib/plist.c
@@ -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);
--
1.7.3.1
On Wed, 2011-06-29 at 12:33 -0700, Dima Zavin wrote:
> +#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));
Spin locks are NOPs on SMP, but mutexes are not. Are you sure you want
this as WARN_ON_SMP()?
-- Steve
> if (!plist_head_empty(head))
> plist_check_list(&plist_first(head)->prio_list);
> plist_check_list(&head->node_list);
On Wed, Jun 29, 2011 at 1:01 PM, Steven Rostedt <[email protected]> wrote:
> On Wed, 2011-06-29 at 12:33 -0700, Dima Zavin wrote:
>
>> +#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));
>
> 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
>
> -- Steve
>
>> ? ? ? if (!plist_head_empty(head))
>> ? ? ? ? ? ? ? plist_check_list(&plist_first(head)->prio_list);
>> ? ? ? plist_check_list(&head->node_list);
>
>
>
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(-)
diff --git a/include/linux/plist.h b/include/linux/plist.h
index c9b9f32..bd670f6 100644
--- a/include/linux/plist.h
+++ b/include/linux/plist.h
@@ -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
}
diff --git a/lib/plist.c b/lib/plist.c
index 0ae7e64..f3c2cad 100644
--- a/lib/plist.c
+++ b/lib/plist.c
@@ -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);
--
1.7.3.1
Dima Zavin <[email protected]> writes:
> 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
--
[email protected] -- Speaking for myself only
On Wed, Jun 29, 2011 at 1:13 PM, Andi Kleen <[email protected]> wrote:
> Dima Zavin <[email protected]> writes:
>
>> 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.
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
>
> -Andi
>
>
> --
> [email protected] -- Speaking for myself only
>
On Wed, 2011-06-29 at 13:13 -0700, Andi Kleen wrote:
> Dima Zavin <[email protected]> writes:
>
> > 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.
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
On Wed, 2011-06-29 at 13:34 -0700, Dima Zavin wrote:
> 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
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
On Wed, Jun 29, 2011 at 1:43 PM, Steven Rostedt <[email protected]> wrote:
> On Wed, 2011-06-29 at 13:34 -0700, Dima Zavin wrote:
>
>> 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
>
>
>
On Thu, 2011-06-30 at 15:14 -0700, Dima Zavin wrote:
> 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
> On Wed, Jun 29, 2011 at 1:43 PM, Steven Rostedt <[email protected]> wrote:
> > On Wed, 2011-06-29 at 13:34 -0700, Dima Zavin wrote:
> >
> >> 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
> >
> >
> >