Implement arch primitives for busywait loops
Subsystems (5)
Nicholas Piggin
reviewerRFC PATCH 1/1 Implement arch primitives for busywait loops
Implementing busy wait loops with cpu_relax() in callers poses some difficulties for powerpc.
First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, 'HMT_low() ; HMT_medium();' as cpu_relax() does may have HMT_medium take effect before HMT_low made any (or much) difference.
Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I would not propose to switch all callers immediately, just some core synchronisation primitives.
--- arch/powerpc/include/asm/processor.h | 22 ++++++++++++++++++++++ include/asm-generic/barrier.h | 7 ++----- include/linux/bit_spinlock.h | 5 ++--- include/linux/cgroup.h | 7 ++----- include/linux/seqlock.h | 10 ++++------ 5 files changed, 32 insertions(+), 19 deletions(-)
arch/powerpc/include/asm/processor.h +22 -0
@@ -402,6 +402,28 @@ static inline unsigned long __pack_fe01(unsigned int fpmode)#ifdef CONFIG_PPC64 #define cpu_relax () do { HMT_low(); HMT_medium(); barrier(); } while (0) + +#define spin_do \ +do { \ + HMT_low(); \ + __asm__ __volatile__ ( "1010:"); + +#define spin_while (cond) \ + barrier(); \ + __asm__ __volatile__ ( "cmpdi %0,0 \n\t" \ + "beq- 1010b \n\t" \ + : : "r" (cond)); \ + HMT_medium(); \ +} while (0) + +#define spin_until (cond) \ + barrier(); \ + __asm__ __volatile__ ( "cmpdi %0,0 \n\t" \ + "bne- 1010b \n\t" \ + : : "r" (cond)); \ + HMT_medium(); \ +} while (0) +#else #define cpu_relax () barrier()#endif
include/asm-generic/barrier.h +2 -5
@@ -235,12 +235,9 @@ do { \#define smp_cond_load_acquire (ptr, cond_expr) ({ \ typeof(ptr) __PTR = (ptr); \ typeof(*ptr) VAL; \ - for (;;) { \ + spin_do { \ VAL = READ_ONCE(*__PTR); \ - if (cond_expr) \ - break; \ - cpu_relax(); \ - } \ + } spin_until (cond_expr); \ smp_acquire__after_ctrl_dep(); \ VAL; \ })
include/linux/bit_spinlock.h +2 -3
@@ -25,9 +25,8 @@ static inline void bit_spin_lock(int bitnum, unsigned long *addr)#if defined(CONFIG_SMP ) || defined(CONFIG_DEBUG_SPINLOCK )while (unlikely (test_and_set_bit_lock (bitnum, addr))) {preempt_enable (); -do { -cpu_relax (); - }while (test_bit (bitnum, addr)); + spin_do { + }spin_while (test_bit (bitnum, addr));preempt_disable (); }#endif
include/linux/cgroup.h +2 -5
@@ -450,12 +450,9 @@ task_get_css(struct task_struct *task, int subsys_id)struct cgroup_subsys_state *css;rcu_read_lock (); -while (true) { + spin_do { css =task_css (task, subsys_id); -if (likely (css_tryget_online (css))) -break ; -cpu_relax (); - } + }spin_until (likely (css_tryget_online(css )));rcu_read_unlock ();return css; }
include/linux/seqlock.h +4 -6
@@ -108,12 +108,10 @@ static inline unsigned __read_seqcount_begin(const seqcount_t *s) {unsigned ret; -repeat: - ret =READ_ONCE (s->sequence); -if (unlikely (ret &1 )) { -cpu_relax (); - goto repeat; - } + spin_do { + ret =READ_ONCE (s->sequence); + }spin_while (unlikely (ret &1 )); +return ret; }
Signature · 1 line
2.9.3
From: Nicholas Piggin
Quoted 10 lines
Sent: 16 September 2016 09:58
Implementing busy wait loops withcpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.
Quoted 8 lines
Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I think it will still all go wrong if the conditional isn't trivial. In particular if the condition contains || or && it is likely to have a branch - which could invert the loop.
David
Nicholas Piggin
reviewerQuoted 15 lines · On Fri, 16 Sep 2016 11:30:58 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 14 lines
Sent: 16 September 2016 09:58
Implementing busy wait loops withcpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 8 lines
Quoted 4 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I think it will still all go wrong if the conditional isn't trivial. In particular if the condition contains || or && it is likely to have a branch - which could invert the loop.
I don't know that it will.
Yes, if we have exit condition that requires more branches in order to be computed then we lose our nice property of never taking a branch miss on loop exit. But we still avoid *this* branch miss, and still prevent multiple iterations of the wait loop being speculatively executed concurrently when there's no work to be done.
And C doesn't know about the loop, so it can't do any transformation except to compute the final condition.
Or have I missed something?
Thanks,
Nick
From: Nicholas Piggin
Quoted 36 lines
Sent: 16 September 2016 12:52
Quoted 15 lines · On Fri, 16 Sep 2016 11:30:58 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 14 lines
Sent: 16 September 2016 09:58
Implementing busy wait loops withcpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 8 lines
Quoted 4 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I think it will still all go wrong if the conditional isn't trivial. In particular if the condition contains || or && it is likely to have a branch - which could invert the loop.
I don't know that it will.
Yes, if we have exit condition that requires more branches in order to be computed then we lose our nice property of never taking a branch miss on loop exit. But we still avoid *this* branch miss, and still prevent multiple iterations of the wait loop being speculatively executed concurrently when there's no work to be done.
Quoted 4 lines
And C doesn't know about the loop, so it can't do any transformation except to compute the final condition.
Or have I missed something?
Try putting the code inside a conditional or at the bottom of a loop. gcc can replicate code to remove a branch.
So:
for (;;) {
a;
if (b)
c;
d;
}can become:
x1:
a;
if (b) to x2;
d;
goto x1;
x2:
c;
d;
goto x1;Which won't work.
David
Nicholas Piggin
reviewerQuoted 45 lines · On Fri, 16 Sep 2016 11:57:37 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 36 lines
Sent: 16 September 2016 12:52
Quoted 15 lines · On Fri, 16 Sep 2016 11:30:58 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 14 lines
Sent: 16 September 2016 09:58
Implementing busy wait loops withcpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 8 lines
Quoted 4 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I think it will still all go wrong if the conditional isn't trivial. In particular if the condition contains || or && it is likely to have a branch - which could invert the loop.
I don't know that it will.
Yes, if we have exit condition that requires more branches in order to be computed then we lose our nice property of never taking a branch miss on loop exit. But we still avoid *this* branch miss, and still prevent multiple iterations of the wait loop being speculatively executed concurrently when there's no work to be done.
Quoted 4 lines
And C doesn't know about the loop, so it can't do any transformation except to compute the final condition.
Or have I missed something?
Try putting the code inside a conditional or at the bottom of a loop. gcc can replicate code to remove a branch.
Quoted 7 lines
So:
for (;;) { a; if (b) c; d; }
That's not what this patch does though. The loop is purely asm. gcc has no idea about it. Only thing gcc knows is to evaluate the condition and put it in a register.
Thanks,
Nick
Nicholas Piggin
reviewerQuoted 56 lines · On Fri, 16 Sep 2016 22:06:35 +1000 Nicholas Piggin <[email protected]> wrote:
Quoted 45 lines · On Fri, 16 Sep 2016 11:57:37 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 36 lines
Sent: 16 September 2016 12:52
Quoted 15 lines · On Fri, 16 Sep 2016 11:30:58 +0000 David Laight <[email protected]> wrote:
From: Nicholas Piggin
Quoted 14 lines
Sent: 16 September 2016 09:58
Implementing busy wait loops withcpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 8 lines
Quoted 4 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I think it will still all go wrong if the conditional isn't trivial. In particular if the condition contains || or && it is likely to have a branch - which could invert the loop.
I don't know that it will.
Yes, if we have exit condition that requires more branches in order to be computed then we lose our nice property of never taking a branch miss on loop exit. But we still avoid *this* branch miss, and still prevent multiple iterations of the wait loop being speculatively executed concurrently when there's no work to be done.
Quoted 4 lines
And C doesn't know about the loop, so it can't do any transformation except to compute the final condition.
Or have I missed something?
Try putting the code inside a conditional or at the bottom of a loop. gcc can replicate code to remove a branch.
Quoted 7 lines
So:
for (;;) { a; if (b) c; d; }
Quoted 3 lines
That's not what this patch does though. The loop is purely asm. gcc has no idea about it. Only thing gcc knows is to evaluate the condition and put it in a register.
Oh you're right course -- can't branch to random location. Sorry, I didn't know what you meant at first. It does need to use asm goto I guess.
Thanks,
Nick
Quoted 7 lines
include/linux/bit_spinlock.h +0
-0@@ -25,9 +25,8 @@ static inline void bit_spin_lock(int bitnum, unsigned long *addr)#if defined(CONFIG_SMP ) || defined(CONFIG_DEBUG_SPINLOCK )
include/linux/bit_spinlock.h static inline void bit_spin_lock(int bitnum, unsigned long *addr) · line 32
Quoted 10 lines
include/linux/bit_spinlock.h +2
-3while (unlikely (test_and_set_bit_lock (bitnum, addr))) {preempt_enable (); -do { -cpu_relax (); - }while (test_bit (bitnum, addr)); + spin_do { + }spin_while (test_bit (bitnum, addr));preempt_disable (); }#endif
That usage is strange, with nothing in the do{ }while loop. May be add a macro for usages like this ?
-aneesh
Quoted 9 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.
Quoted 4 lines
Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
IIUC, what you are proposing is that cpu_relax() be split such that on entry we do HMT_low() and on exit do HMT_medium(). I think that makes a lot of sense, in that it allows the required transition time from low to medium
Quoted 21 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I would not propose to switch all callers immediately, just some core synchronisation primitives.
--- arch/powerpc/include/asm/processor.h | 22 ++++++++++++++++++++++ include/asm-generic/barrier.h | 7 ++----- include/linux/bit_spinlock.h | 5 ++--- include/linux/cgroup.h | 7 ++----- include/linux/seqlock.h | 10 ++++------ 5 files changed, 32 insertions(+), 19 deletions(-)arch/powerpc/include/asm/processor.h +0
-0
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 406
Quoted 5 lines
arch/powerpc/include/asm/processor.h +2
-0@@ -402,6 +402,28 @@ static inline unsigned long __pack_fe01(unsigned int fpmode)#ifdef CONFIG_PPC64 #define cpu_relax () do { HMT_low(); HMT_medium(); barrier(); } while (0) + +#define spin_do \
How about cpu_relax_begin()?
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 411
Quoted 5 lines
arch/powerpc/include/asm/processor.h +5
-0+do { \ + HMT_low(); \ + __asm__ __volatile__ ( "1010:"); + +#define spin_while(cond) \
cpu_relax_while()
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 419
Quoted 10 lines
arch/powerpc/include/asm/processor.h +8
-0+barrier (); \ + __asm__ __volatile__ ("cmpdi %0,0 \n\t" \ +"beq- 1010b \n\t" \ + : :"r" (cond)); \ +HMT_medium (); \ +}while (0 ) + +#define spin_until(cond) \
This is just spin_while(!cond) from an implementation perspective right?
cpu_relax_until()
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 426
Quoted 7 lines
arch/powerpc/include/asm/processor.h +7
-0+barrier (); \ + __asm__ __volatile__ ("cmpdi %0,0 \n\t" \ +"bne- 1010b \n\t" \ + : :"r" (cond)); \ +HMT_medium (); \ +}while (0 ) +
Then add cpu_relax_end() that does HMT_medium()
Balbir Singh.
Nicholas Piggin
reviewerQuoted 15 lines · On Mon, 19 Sep 2016 17:45:52 +1000 Balbir Singh <[email protected]> wrote:
Quoted 9 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Quoted 4 lines
Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 4 lines
IIUC, what you are proposing is that
cpu_relax()be split such that on entry we doHMT_low()and on exit doHMT_medium(). I think that makes a lot of sense, in that it allows the required transition time from low to medium
Basically yes, although also allowing the loop exit branch to be overridden by the arch code too. That can possibly benefit some microarchitectures -- e.g., you want loop exit to not take a branch miss if possible but it may be acceptable to branch miss for every other iteration. I'm doing some testing of it now (previous patch was garbage btw, don't try to use it!)
Quoted 29 lines
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 406
Quoted 10 lines
arch/powerpc/include/asm/processor.h +2
-0@@ -402,6 +402,28 @@ static inline unsigned long __pack_fe01(unsigned int fpmode)#ifdef CONFIG_PPC64 #define cpu_relax () do { HMT_low(); HMT_medium(); barrier(); } while (0) + +#define spin_do \How about
cpu_relax_begin()?arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 411
Quoted 5 lines
arch/powerpc/include/asm/processor.h +5
-0+do { \ + HMT_low(); \ + __asm__ __volatile__ ( "1010:"); + +#define spin_while(cond) \
cpu_relax_while()Quoted 7 lines
arch/powerpc/include/asm/processor.h +7
-0+barrier (); \ + __asm__ __volatile__ ("cmpdi %0,0 \n\t" \ +"beq- 1010b \n\t" \ + : :"r" (cond)); \ +HMT_medium (); \ +}while (0 ) +
Quoted 3 lines
arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 419
Quoted 1 line
arch/powerpc/include/asm/processor.h +1
-0+#define spin_until (cond) \This is just
spin_while(!cond)from an implementation perspective right?
Yes, the only reason I put it in was because such spin loops often read a bit better the other way from normal loops (you are interested in the exit condition rather than the loop-again condition).
Quoted 12 lines
cpu_relax_until()arch/powerpc/include/asm/processor.h static inline unsigned long __pack_fe01(unsigned int fpmode) · line 426
Quoted 7 lines
arch/powerpc/include/asm/processor.h +7
-0+barrier (); \ + __asm__ __volatile__ ("cmpdi %0,0 \n\t" \ +"bne- 1010b \n\t" \ + : :"r" (cond)); \ +HMT_medium (); \ +}while (0 ) +Then add
cpu_relax_end()that doesHMT_medium()
Hmm. I see what you mean, but I don't know if we should trust open coded callers to get this right. It could cause weird problems and isn't easily caught. If we can get something that breaks build, perhaps. OTOH, I prefer all the logic including SMT priority to be in the spin loop primitive directly because it's pretty subtle and might need to be runtime patched.
We could *also* have a cpu_relax_begin/cpu_relax_end pair, although I'd like to first see callers that don't suit spin_ primitives and see what should be done.
Thanks,
Nick
Quoted 13 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
Quoted 7 lines
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
I would not propose to switch all callers immediately, just some core synchronisation primitives.
Just a FYA,
On s390 we have a private version of cpu_relax that yields the cpu time slice back to the hypervisor via a hypercall. As this turned out to be problematic in some cases there is also now a cpu_relax_lowlatency.
Now, this seems still problematic as there are too many places still using cpu_relax instead of cpu_relax_lowlatency. So my plan is to do a change of that, make cpu_relax just be a barrier and add a new cpu_relax_yield that gives up the time slice. (so that s390 cpu_relax is just like any other cpu_relax)
As far as I can tell the only place where I want to change cpu_relax to cpu_relax_lowlatency after that change is the stop machine run code, so I hope to have no conflicts with your changes.
Quoted 117 lines
--- arch/powerpc/include/asm/processor.h | 22 ++++++++++++++++++++++ include/asm-generic/barrier.h | 7 ++----- include/linux/bit_spinlock.h | 5 ++--- include/linux/cgroup.h | 7 ++----- include/linux/seqlock.h | 10 ++++------ 5 files changed, 32 insertions(+), 19 deletions(-)arch/powerpc/include/asm/processor.h +22
-0@@ -402,6 +402,28 @@ static inline unsigned long __pack_fe01(unsigned int fpmode)#ifdef CONFIG_PPC64 #define cpu_relax () do { HMT_low(); HMT_medium(); barrier(); } while (0) + +#define spin_do \ +do { \ + HMT_low(); \ + __asm__ __volatile__ ( "1010:"); + +#define spin_while (cond) \ + barrier(); \ + __asm__ __volatile__ ( "cmpdi %0,0 \n\t" \ + "beq- 1010b \n\t" \ + : : "r" (cond)); \ + HMT_medium(); \ +} while (0) + +#define spin_until (cond) \ + barrier(); \ + __asm__ __volatile__ ( "cmpdi %0,0 \n\t" \ + "bne- 1010b \n\t" \ + : : "r" (cond)); \ + HMT_medium(); \ +} while (0) +#else #define cpu_relax () barrier()#endif include/asm-generic/barrier.h +2
-5@@ -235,12 +235,9 @@ do { \#define smp_cond_load_acquire (ptr, cond_expr) ({ \ typeof(ptr) __PTR = (ptr); \ typeof(*ptr) VAL; \ - for (;;) { \ + spin_do { \ VAL = READ_ONCE(*__PTR); \ - if (cond_expr) \ - break; \ - cpu_relax(); \ - } \ + } spin_until (cond_expr); \ smp_acquire__after_ctrl_dep(); \ VAL; \ })include/linux/bit_spinlock.h +2
-3@@ -25,9 +25,8 @@ static inline void bit_spin_lock(int bitnum, unsigned long *addr)#if defined(CONFIG_SMP ) || defined(CONFIG_DEBUG_SPINLOCK )while (unlikely (test_and_set_bit_lock (bitnum, addr))) {preempt_enable (); -do { -cpu_relax (); - }while (test_bit (bitnum, addr)); + spin_do { + }spin_while (test_bit (bitnum, addr));preempt_disable (); }#endif include/linux/cgroup.h +2
-5@@ -450,12 +450,9 @@ task_get_css(struct task_struct *task, int subsys_id)struct cgroup_subsys_state *css;rcu_read_lock (); -while (true) { + spin_do { css =task_css (task, subsys_id); -if (likely (css_tryget_online (css))) -break ; -cpu_relax (); - } + }spin_until (likely (css_tryget_online(css )));rcu_read_unlock ();return css; }include/linux/seqlock.h +4
-6@@ -108,12 +108,10 @@ static inline unsigned __read_seqcount_begin(const seqcount_t *s) {unsigned ret; -repeat: - ret =READ_ONCE (s->sequence); -if (unlikely (ret &1 )) { -cpu_relax (); - goto repeat; - } + spin_do { + ret =READ_ONCE (s->sequence); + }spin_while (unlikely (ret &1 )); +return ret; }
Nicholas Piggin
reviewerQuoted 19 lines · On Tue, 20 Sep 2016 13:19:30 +0200 Christian Borntraeger…
Quoted 18 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
Quoted 6 lines
Quoted 2 lines
I would not propose to switch all callers immediately, just some core synchronisation primitives.
Just a FYA,
On s390 we have a private version of
cpu_relaxthat yields the cpu time slice back to the hypervisor via a hypercall.
The powerpc guest also wants to yield to hypervisor in some busywait situations.
Quoted 9 lines
As this turned out
to be problematic in some cases there is also now acpu_relax_lowlatency.Now, this seems still problematic as there are too many places still using
cpu_relaxinstead ofcpu_relax_lowlatency. So my plan is to do a change of that, makecpu_relaxjust be a barrier and add a newcpu_relax_yieldthat gives up the time slice. (so that s390cpu_relaxis just like any othercpu_relax)
Quoted 3 lines
As far as I can tell the only place where I want to change
cpu_relaxtocpu_relax_lowlatencyafter that change is the stop machine run code, so I hope to have no conflicts with your changes.
I don't think there should be any conflicts, but it would be good to make sure busy wait primitives can be usable by s390. So I can add _yield variants that can do the right thing for s390.
I need to think more about virtualization, so I'm glad you commented. Powerpc would like to be told when a busywait loop knows the CPU it is waiting for. So perhaps also a _yield_to_cpu variant as well.
Something that will work with mutex_spin_on_owner and similar would be nice too. As far as I can tell, powerpc may want to yield to hypervisor when the owner's vcpu is scheduled off in that case too.
Thanks,
Nick
Quoted 41 lines · Nicholas Piggin
Quoted 19 lines · On Tue, 20 Sep 2016 13:19:30 +0200 Christian Borntraeger…
Quoted 18 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
Quoted 6 lines
Quoted 2 lines
I would not propose to switch all callers immediately, just some core synchronisation primitives.
Just a FYA,
On s390 we have a private version of
cpu_relaxthat yields the cpu time slice back to the hypervisor via a hypercall.The powerpc guest also wants to yield to hypervisor in some busywait situations.
Quoted 9 lines
As this turned out
to be problematic in some cases there is also now acpu_relax_lowlatency.Now, this seems still problematic as there are too many places still using
cpu_relaxinstead ofcpu_relax_lowlatency. So my plan is to do a change of that, makecpu_relaxjust be a barrier and add a newcpu_relax_yieldthat gives up the time slice. (so that s390cpu_relaxis just like any othercpu_relax)
Quoted 7 lines
Quoted 3 lines
As far as I can tell the only place where I want to change
cpu_relaxtocpu_relax_lowlatencyafter that change is the stop machine run code, so I hope to have no conflicts with your changes.I don't think there should be any conflicts, but it would be good to make sure busy wait primitives can be usable by s390. So I can add
_yieldvariants that can do the right thing for s390.
I was distracted by "more important work" (TM) but I will put you on CC when ready.
Quoted 4 lines
I need to think more about virtualization, so I'm glad you commented. Powerpc would like to be told when a busywait loop knows the CPU it is waiting for. So perhaps also a
_yield_to_cpuvariant as well.
Yes, we also have 2 hypercalls: one that yields somehow and one that yields to a specific CPU. The latter is strongly preferred.
Quoted 5 lines
Something that will work with
mutex_spin_on_ownerand similar would be nice too. As far as I can tell, powerpc may want to yield to hypervisor when the owner's vcpu is scheduled off in that case too.
Nicholas Piggin
reviewerQuoted 53 lines · On Tue, 20 Sep 2016 14:35:45 +0200 Christian Borntraeger…
Quoted 41 lines · Nicholas Piggin
Quoted 19 lines · On Tue, 20 Sep 2016 13:19:30 +0200 Christian Borntraeger…
Quoted 18 lines · Nicholas Piggin
Implementing busy wait loops with
cpu_relax()in callers poses some difficulties for powerpc.First, we want to put our SMT thread into a low priority mode for the duration of the loop, but then return to normal priority after exiting the loop. Dependong on the CPU design, '
HMT_low();HMT_medium();' ascpu_relax()does may haveHMT_mediumtake effect beforeHMT_lowmade any (or much) difference.Second, it can be beneficial for some implementations to spin on the exit condition with a statically predicted-not-taken branch (i.e., always predict the loop will exit).
This is a quick RFC with a couple of users converted to see what people think. I don't use a C branch with hints, because we don't want the compiler moving the loop body out of line, which makes it a bit messy unfortunately. If there's a better way to do it, I'm all ears.
Quoted 6 lines
Quoted 2 lines
I would not propose to switch all callers immediately, just some core synchronisation primitives.
Just a FYA,
On s390 we have a private version of
cpu_relaxthat yields the cpu time slice back to the hypervisor via a hypercall.The powerpc guest also wants to yield to hypervisor in some busywait situations.
Quoted 9 lines
As this turned out
to be problematic in some cases there is also now acpu_relax_lowlatency.Now, this seems still problematic as there are too many places still using
cpu_relaxinstead ofcpu_relax_lowlatency. So my plan is to do a change of that, makecpu_relaxjust be a barrier and add a newcpu_relax_yieldthat gives up the time slice. (so that s390cpu_relaxis just like any othercpu_relax)Quoted 7 lines
Quoted 3 lines
As far as I can tell the only place where I want to change
cpu_relaxtocpu_relax_lowlatencyafter that change is the stop machine run code, so I hope to have no conflicts with your changes.I don't think there should be any conflicts, but it would be good to make sure busy wait primitives can be usable by s390. So I can add
_yieldvariants that can do the right thing for s390.I was distracted by "more important work" (TM) but I will put you on CC when ready.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
I need to think more about virtualization, so I'm glad you commented. Powerpc would like to be told when a busywait loop knows the CPU it is waiting for. So perhaps also a
_yield_to_cpuvariant as well.Yes, we also have 2 hypercalls: one that yields somehow and one that yields to a specific CPU. The latter is strongly preferred.
Okay, sounds good. I'll send out some updated patches soon too, so I'll cc you on those. It would be good to come up with some basic guidelines for when to use each variant too.
Thanks,
Nick