Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
Gregory, I guess we should have just read Documentation/memory-barriers.text Here's the snippet: Any atomic operation that modifies some state in memory and returns information about the state (old or new) implies an SMP-conditional general memory barrier (smp_mb()) on each side of the actual operation (with the exception of explicit lock operations, described later). These include: xchg(); cmpxchg(); atomic_cmpxchg(); atomic_inc_return(); atomic_dec_return(); atomic_add_return(); atomic_sub_return(); atomic_inc_and_test(); atomic_dec_and_test(); atomic_sub_and_test(); atomic_add_negative(); atomic_add_unless(); test_and_set_bit(); test_and_clear_bit(); test_and_change_bit(); -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 22 Feb 2008, Ingo Molnar wrote: > > * Gregory Haskins <[EMAIL PROTECTED]> wrote: > > > My assumption is that the xchg() (inside update_current()) acts as an > > effective wmb(). If xchg() does not have this property, then this > > code is broken and patch 6/14 should also add a: > > xchg() is a strong implicit memory barrier, it implies smp_mb(). > (historic sidenote: it was the very first SMP primitive we had in > Linux.) OK, I've been proven wrong ;-) I was just thinking of how an arch would implement it. No need for memory barriers in just an xchg. But if Linux "implies it" then that's another story. Thanks, -- Steve /me learns something new everyday. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 2008-02-22 at 08:35 -0500, Steven Rostedt wrote: > > My assumption is that the xchg() (inside update_current()) acts as an > > effective wmb(). If xchg() does not have this property, then this code > > is broken and patch 6/14 should also add a: > > > > > > + smp_wmb(); > > I believe that the wmb would be needed. I doubt that xchg on all archs > would force any ordering of reads and writes. It only needs to guarantee the > atomic nature of the data exchange. I don't see any reason that it would > imply any type of memory barrier. Documentation/memory-barriers.txt states: Any atomic operation that modifies some state in memory and returns information about the state (old or new) implies an SMP-conditional general memory barrier (smp_mb()) on each side of the actual operation (with the exception of explicit lock operations, described later). These include: xchg(); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
* Gregory Haskins <[EMAIL PROTECTED]> wrote: > My assumption is that the xchg() (inside update_current()) acts as an > effective wmb(). If xchg() does not have this property, then this > code is broken and patch 6/14 should also add a: xchg() is a strong implicit memory barrier, it implies smp_mb(). (historic sidenote: it was the very first SMP primitive we had in Linux.) Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 22 Feb 2008, Gregory Haskins wrote: > Gregory Haskins wrote: > > @@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock) > > > > debug_rt_mutex_print_deadlock(); > > > > - schedule_rt_mutex(lock); > > + update_current(TASK_UNINTERRUPTIBLE, _state); > > I have a question for everyone out there about this particular part of > the code. Patch 6/14 adds an optimization that is predicated on the > order in which we modify the state==TASK_UNINTERRUPTIBLE vs reading the > waiter.task below. > > My assumption is that the xchg() (inside update_current()) acts as an > effective wmb(). If xchg() does not have this property, then this code > is broken and patch 6/14 should also add a: > > > + smp_wmb(); I believe that the wmb would be needed. I doubt that xchg on all archs would force any ordering of reads and writes. It only needs to guarantee the atomic nature of the data exchange. I don't see any reason that it would imply any type of memory barrier. -- Steve > > > > + if (waiter.task) > > + schedule_rt_mutex(lock); > > + else > > + update_current(TASK_RUNNING_MUTEX, _state); > > > > spin_lock_irqsave(>wait_lock, flags); > > current->flags |= saved_flags; > > current->lock_depth = saved_lock_depth; > > - state = xchg(>state, TASK_UNINTERRUPTIBLE); > > - if (unlikely(state == TASK_RUNNING)) > > - saved_state = TASK_RUNNING; > > > Does anyone know the answer to this? > > Regards, > -Greg > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
Gregory Haskins wrote: @@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock) debug_rt_mutex_print_deadlock(); - schedule_rt_mutex(lock); + update_current(TASK_UNINTERRUPTIBLE, _state); I have a question for everyone out there about this particular part of the code. Patch 6/14 adds an optimization that is predicated on the order in which we modify the state==TASK_UNINTERRUPTIBLE vs reading the waiter.task below. My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: + smp_wmb(); + if (waiter.task) + schedule_rt_mutex(lock); + else + update_current(TASK_RUNNING_MUTEX, _state); spin_lock_irqsave(>wait_lock, flags); current->flags |= saved_flags; current->lock_depth = saved_lock_depth; - state = xchg(>state, TASK_UNINTERRUPTIBLE); - if (unlikely(state == TASK_RUNNING)) - saved_state = TASK_RUNNING; Does anyone know the answer to this? Regards, -Greg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 22 Feb 2008, Gregory Haskins wrote: Gregory Haskins wrote: @@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock) debug_rt_mutex_print_deadlock(waiter); - schedule_rt_mutex(lock); + update_current(TASK_UNINTERRUPTIBLE, saved_state); I have a question for everyone out there about this particular part of the code. Patch 6/14 adds an optimization that is predicated on the order in which we modify the state==TASK_UNINTERRUPTIBLE vs reading the waiter.task below. My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: + smp_wmb(); I believe that the wmb would be needed. I doubt that xchg on all archs would force any ordering of reads and writes. It only needs to guarantee the atomic nature of the data exchange. I don't see any reason that it would imply any type of memory barrier. -- Steve + if (waiter.task) + schedule_rt_mutex(lock); + else + update_current(TASK_RUNNING_MUTEX, saved_state); spin_lock_irqsave(lock-wait_lock, flags); current-flags |= saved_flags; current-lock_depth = saved_lock_depth; - state = xchg(current-state, TASK_UNINTERRUPTIBLE); - if (unlikely(state == TASK_RUNNING)) - saved_state = TASK_RUNNING; Does anyone know the answer to this? Regards, -Greg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
* Gregory Haskins [EMAIL PROTECTED] wrote: My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: xchg() is a strong implicit memory barrier, it implies smp_mb(). (historic sidenote: it was the very first SMP primitive we had in Linux.) Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 2008-02-22 at 08:35 -0500, Steven Rostedt wrote: My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: + smp_wmb(); I believe that the wmb would be needed. I doubt that xchg on all archs would force any ordering of reads and writes. It only needs to guarantee the atomic nature of the data exchange. I don't see any reason that it would imply any type of memory barrier. Documentation/memory-barriers.txt states: Any atomic operation that modifies some state in memory and returns information about the state (old or new) implies an SMP-conditional general memory barrier (smp_mb()) on each side of the actual operation (with the exception of explicit lock operations, described later). These include: xchg(); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
On Fri, 22 Feb 2008, Ingo Molnar wrote: * Gregory Haskins [EMAIL PROTECTED] wrote: My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: xchg() is a strong implicit memory barrier, it implies smp_mb(). (historic sidenote: it was the very first SMP primitive we had in Linux.) OK, I've been proven wrong ;-) I was just thinking of how an arch would implement it. No need for memory barriers in just an xchg. But if Linux implies it then that's another story. Thanks, -- Steve /me learns something new everyday. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
Gregory, I guess we should have just read Documentation/memory-barriers.text Here's the snippet: Any atomic operation that modifies some state in memory and returns information about the state (old or new) implies an SMP-conditional general memory barrier (smp_mb()) on each side of the actual operation (with the exception of explicit lock operations, described later). These include: xchg(); cmpxchg(); atomic_cmpxchg(); atomic_inc_return(); atomic_dec_return(); atomic_add_return(); atomic_sub_return(); atomic_inc_and_test(); atomic_dec_and_test(); atomic_sub_and_test(); atomic_add_negative(); atomic_add_unless(); test_and_set_bit(); test_and_clear_bit(); test_and_change_bit(); -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH [RT] 05/14] rearrange rt_spin_lock sleep
Gregory Haskins wrote: @@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock) debug_rt_mutex_print_deadlock(waiter); - schedule_rt_mutex(lock); + update_current(TASK_UNINTERRUPTIBLE, saved_state); I have a question for everyone out there about this particular part of the code. Patch 6/14 adds an optimization that is predicated on the order in which we modify the state==TASK_UNINTERRUPTIBLE vs reading the waiter.task below. My assumption is that the xchg() (inside update_current()) acts as an effective wmb(). If xchg() does not have this property, then this code is broken and patch 6/14 should also add a: + smp_wmb(); + if (waiter.task) + schedule_rt_mutex(lock); + else + update_current(TASK_RUNNING_MUTEX, saved_state); spin_lock_irqsave(lock-wait_lock, flags); current-flags |= saved_flags; current-lock_depth = saved_lock_depth; - state = xchg(current-state, TASK_UNINTERRUPTIBLE); - if (unlikely(state == TASK_RUNNING)) - saved_state = TASK_RUNNING; Does anyone know the answer to this? Regards, -Greg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH [RT] 05/14] rearrange rt_spin_lock sleep
The current logic makes rather coarse adjustments to current->state since
it is planning on sleeping anyway. We want to eventually move to an
adaptive (e.g. optional sleep) algorithm, so we tighten the scope of the
adjustments to bracket the schedule(). This should yield correct behavior
with or without the adaptive features that are added later in the series.
We add it here as a separate patch for greater review clarity on smaller
changes.
Signed-off-by: Gregory Haskins <[EMAIL PROTECTED]>
---
kernel/rtmutex.c | 20 +++-
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/kernel/rtmutex.c b/kernel/rtmutex.c
index a2b00cc..15fc6e6 100644
--- a/kernel/rtmutex.c
+++ b/kernel/rtmutex.c
@@ -661,6 +661,14 @@ rt_spin_lock_fastunlock(struct rt_mutex *lock,
slowfn(lock);
}
+static inline void
+update_current(unsigned long new_state, unsigned long *saved_state)
+{
+ unsigned long state = xchg(>state, new_state);
+ if (unlikely(state == TASK_RUNNING))
+ *saved_state = TASK_RUNNING;
+}
+
/*
* Slow path lock function spin_lock style: this variant is very
* careful not to miss any non-lock wakeups.
@@ -700,7 +708,8 @@ rt_spin_lock_slowlock(struct rt_mutex *lock)
* saved_state accordingly. If we did not get a real wakeup
* then we return with the saved state.
*/
- saved_state = xchg(>state, TASK_UNINTERRUPTIBLE);
+ saved_state = current->state;
+ smp_mb();
for (;;) {
unsigned long saved_flags;
@@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock)
debug_rt_mutex_print_deadlock();
- schedule_rt_mutex(lock);
+ update_current(TASK_UNINTERRUPTIBLE, _state);
+ if (waiter.task)
+ schedule_rt_mutex(lock);
+ else
+ update_current(TASK_RUNNING_MUTEX, _state);
spin_lock_irqsave(>wait_lock, flags);
current->flags |= saved_flags;
current->lock_depth = saved_lock_depth;
- state = xchg(>state, TASK_UNINTERRUPTIBLE);
- if (unlikely(state == TASK_RUNNING))
- saved_state = TASK_RUNNING;
}
state = xchg(>state, saved_state);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH [RT] 05/14] rearrange rt_spin_lock sleep
The current logic makes rather coarse adjustments to current-state since
it is planning on sleeping anyway. We want to eventually move to an
adaptive (e.g. optional sleep) algorithm, so we tighten the scope of the
adjustments to bracket the schedule(). This should yield correct behavior
with or without the adaptive features that are added later in the series.
We add it here as a separate patch for greater review clarity on smaller
changes.
Signed-off-by: Gregory Haskins [EMAIL PROTECTED]
---
kernel/rtmutex.c | 20 +++-
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/kernel/rtmutex.c b/kernel/rtmutex.c
index a2b00cc..15fc6e6 100644
--- a/kernel/rtmutex.c
+++ b/kernel/rtmutex.c
@@ -661,6 +661,14 @@ rt_spin_lock_fastunlock(struct rt_mutex *lock,
slowfn(lock);
}
+static inline void
+update_current(unsigned long new_state, unsigned long *saved_state)
+{
+ unsigned long state = xchg(current-state, new_state);
+ if (unlikely(state == TASK_RUNNING))
+ *saved_state = TASK_RUNNING;
+}
+
/*
* Slow path lock function spin_lock style: this variant is very
* careful not to miss any non-lock wakeups.
@@ -700,7 +708,8 @@ rt_spin_lock_slowlock(struct rt_mutex *lock)
* saved_state accordingly. If we did not get a real wakeup
* then we return with the saved state.
*/
- saved_state = xchg(current-state, TASK_UNINTERRUPTIBLE);
+ saved_state = current-state;
+ smp_mb();
for (;;) {
unsigned long saved_flags;
@@ -732,14 +741,15 @@ rt_spin_lock_slowlock(struct rt_mutex *lock)
debug_rt_mutex_print_deadlock(waiter);
- schedule_rt_mutex(lock);
+ update_current(TASK_UNINTERRUPTIBLE, saved_state);
+ if (waiter.task)
+ schedule_rt_mutex(lock);
+ else
+ update_current(TASK_RUNNING_MUTEX, saved_state);
spin_lock_irqsave(lock-wait_lock, flags);
current-flags |= saved_flags;
current-lock_depth = saved_lock_depth;
- state = xchg(current-state, TASK_UNINTERRUPTIBLE);
- if (unlikely(state == TASK_RUNNING))
- saved_state = TASK_RUNNING;
}
state = xchg(current-state, saved_state);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
