Re: [PATCH 12/13] powerpc/64: runlatch CTRL[RUN] set optimisation

2017-06-15 Thread Michael Ellerman
Nicholas Piggin  writes:
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index baae104b16c7..f587c1fdf981 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1960,11 +1960,25 @@ void show_stack(struct task_struct *tsk, unsigned 
> long *stack)
>  void notrace __ppc64_runlatch_on(void)
>  {
>   struct thread_info *ti = current_thread_info();
> - unsigned long ctrl;
>  
> - ctrl = mfspr(SPRN_CTRLF);
> - ctrl |= CTRL_RUNLATCH;
> - mtspr(SPRN_CTRLT, ctrl);
> + if (cpu_has_feature(CPU_FTR_ARCH_206)) {
> + /*
> +  * Least significant bit (RUN) is the only writable bit of
> +  * the CTRL register, so we can avoid mfspr. 2.06 is not the
> +  * earliest ISA where this is the case, but it's convenient.
> +  */
> + mtspr(SPRN_CTRLT, CTRL_RUNLATCH);
> + } else {
> + unsigned long ctrl;
> +
> + /*
> +  * Some architectures (e.g., Cell) have writable fields other
> +  * than RUN, so do the read-modify-write.
> +  */
> + ctrl = mfspr(SPRN_CTRLF);
> + ctrl |= CTRL_RUNLATCH;
> + mtspr(SPRN_CTRLT, ctrl);
> + }

Does the generated code look any good if you do something like:

unsigned long ctrl;

ctrl = 0;
if (!cpu_has_feature(CPU_FTR_ARCH_206))
ctrl = mfspr(SPRN_CTRLF);

ctrl |= CTRL_RUNLATCH;
mtspr(SPRN_CTRLT, ctrl);


That would offend me slightly less ;)

cheers


Re: [PATCH 12/13] powerpc/64: runlatch CTRL[RUN] set optimisation

2017-06-14 Thread Nicholas Piggin
On Wed, 14 Jun 2017 21:38:36 +1000
Michael Ellerman  wrote:

> Nicholas Piggin  writes:
> 
> > The CTRL register is read-only except bit 63 which is the run latch
> > control. This means it can be updated with a mtspr rather than
> > mfspr/mtspr.  
> 
> Turns out this doesn't work on Cell.
> 
> There's an extra field in there:
> 
>   Thread enable bits (Read/Write)
>   The hypervisor state can suspend its own thread by setting the TE bit
>   for its thread to '0’. The hypervisor state can resume the opposite
>   thread by setting the TE bit for the opposite thread to '1'. The
>   hypervisor state cannot suspend the opposite thread by setting the TE
>   bit for the opposite thread to ‘0’. This setting is ignored and does not
>   cause an error.
>   
>   TE0 is the thread enable bit for thread 0.
>   TE1 is the thread enable bit for thread 1.
>   
>   If thread 0 executes the mtctrl instruction, these are the bit values:
>   
>   [TE0 TE1] Description
> 0   0   Disable or suspend thread 0; thread 1 unchanged.
> 0   1   Disable or suspend thread 0; enable or resume thread 1 if it was 
> disabled.
> 1   0   Unchanged.
> 1   1   Enable or resume thread 1 if it was disabled.
>   
>   If thread 1 executes the mtctrl instruction, these are the bit values:
>   
>   [TE0 TE1] Description
> 0   0Thread 0 unchanged; disable or suspend thread 1.
> 0   1Unchanged.
> 1   0Enable or resume thread 0 if it was disabled; disable or suspend 
> thread 1.
> 1   1Enable or resume thread 0 if it was disabled.
> 
> 
> So writing either 0 or CTRL_RUNLATCH (1) will disable the thread that
> does the write - :D
> 
> For now I'll just drop this.

Ouch, good catch, thanks. I'll go with something like this (below)
instead, but I'll do some testing and get numbers first. So dropping
it should be fine.

---
 arch/powerpc/kernel/process.c | 35 +++
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index baae104b16c7..f587c1fdf981 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1960,11 +1960,25 @@ void show_stack(struct task_struct *tsk, unsigned long 
*stack)
 void notrace __ppc64_runlatch_on(void)
 {
struct thread_info *ti = current_thread_info();
-   unsigned long ctrl;
 
-   ctrl = mfspr(SPRN_CTRLF);
-   ctrl |= CTRL_RUNLATCH;
-   mtspr(SPRN_CTRLT, ctrl);
+   if (cpu_has_feature(CPU_FTR_ARCH_206)) {
+   /*
+* Least significant bit (RUN) is the only writable bit of
+* the CTRL register, so we can avoid mfspr. 2.06 is not the
+* earliest ISA where this is the case, but it's convenient.
+*/
+   mtspr(SPRN_CTRLT, CTRL_RUNLATCH);
+   } else {
+   unsigned long ctrl;
+
+   /*
+* Some architectures (e.g., Cell) have writable fields other
+* than RUN, so do the read-modify-write.
+*/
+   ctrl = mfspr(SPRN_CTRLF);
+   ctrl |= CTRL_RUNLATCH;
+   mtspr(SPRN_CTRLT, ctrl);
+   }
 
ti->local_flags |= _TLF_RUNLATCH;
 }
@@ -1973,13 +1987,18 @@ void notrace __ppc64_runlatch_on(void)
 void notrace __ppc64_runlatch_off(void)
 {
struct thread_info *ti = current_thread_info();
-   unsigned long ctrl;
 
ti->local_flags &= ~_TLF_RUNLATCH;
 
-   ctrl = mfspr(SPRN_CTRLF);
-   ctrl &= ~CTRL_RUNLATCH;
-   mtspr(SPRN_CTRLT, ctrl);
+   if (cpu_has_feature(CPU_FTR_ARCH_206)) {
+   mtspr(SPRN_CTRLT, 0);
+   } else {
+   unsigned long ctrl;
+
+   ctrl = mfspr(SPRN_CTRLF);
+   ctrl &= ~CTRL_RUNLATCH;
+   mtspr(SPRN_CTRLT, ctrl);
+   }
 }
 #endif /* CONFIG_PPC64 */
 
-- 
2.11.0



Re: [PATCH 12/13] powerpc/64: runlatch CTRL[RUN] set optimisation

2017-06-14 Thread Michael Ellerman
Nicholas Piggin  writes:

> The CTRL register is read-only except bit 63 which is the run latch
> control. This means it can be updated with a mtspr rather than
> mfspr/mtspr.

Turns out this doesn't work on Cell.

There's an extra field in there:

  Thread enable bits (Read/Write)
  The hypervisor state can suspend its own thread by setting the TE bit
  for its thread to '0’. The hypervisor state can resume the opposite
  thread by setting the TE bit for the opposite thread to '1'. The
  hypervisor state cannot suspend the opposite thread by setting the TE
  bit for the opposite thread to ‘0’. This setting is ignored and does not
  cause an error.
  
  TE0 is the thread enable bit for thread 0.
  TE1 is the thread enable bit for thread 1.
  
  If thread 0 executes the mtctrl instruction, these are the bit values:
  
  [TE0 TE1] Description
0   0   Disable or suspend thread 0; thread 1 unchanged.
0   1   Disable or suspend thread 0; enable or resume thread 1 if it was 
disabled.
1   0   Unchanged.
1   1   Enable or resume thread 1 if it was disabled.
  
  If thread 1 executes the mtctrl instruction, these are the bit values:
  
  [TE0 TE1] Description
0   0Thread 0 unchanged; disable or suspend thread 1.
0   1Unchanged.
1   0Enable or resume thread 0 if it was disabled; disable or suspend 
thread 1.
1   1Enable or resume thread 0 if it was disabled.


So writing either 0 or CTRL_RUNLATCH (1) will disable the thread that
does the write - :D

For now I'll just drop this.

cheers


[PATCH 12/13] powerpc/64: runlatch CTRL[RUN] set optimisation

2017-06-13 Thread Nicholas Piggin
The CTRL register is read-only except bit 63 which is the run latch
control. This means it can be updated with a mtspr rather than
mfspr/mtspr.

Reviewed-by: Vaidyanathan Srinivasan 
Signed-off-by: Nicholas Piggin 
---
 arch/powerpc/kernel/process.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index baae104b16c7..a44ea034c226 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1960,12 +1960,8 @@ void show_stack(struct task_struct *tsk, unsigned long 
*stack)
 void notrace __ppc64_runlatch_on(void)
 {
struct thread_info *ti = current_thread_info();
-   unsigned long ctrl;
-
-   ctrl = mfspr(SPRN_CTRLF);
-   ctrl |= CTRL_RUNLATCH;
-   mtspr(SPRN_CTRLT, ctrl);
 
+   mtspr(SPRN_CTRLT, CTRL_RUNLATCH);
ti->local_flags |= _TLF_RUNLATCH;
 }
 
@@ -1973,13 +1969,9 @@ void notrace __ppc64_runlatch_on(void)
 void notrace __ppc64_runlatch_off(void)
 {
struct thread_info *ti = current_thread_info();
-   unsigned long ctrl;
 
ti->local_flags &= ~_TLF_RUNLATCH;
-
-   ctrl = mfspr(SPRN_CTRLF);
-   ctrl &= ~CTRL_RUNLATCH;
-   mtspr(SPRN_CTRLT, ctrl);
+   mtspr(SPRN_CTRLT, 0);
 }
 #endif /* CONFIG_PPC64 */
 
-- 
2.11.0