[PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[Vijay Kumar]

cpu_poke is a low latency path to resume the target cpu if suspended
using cpu_yield. Use cpu poke to resume cpu if supported by hypervisor.

	     hackbench results (lower is better):
Number of		
Process:		w/o fix		with fix
1  			0.012		 0.010
10			0.021		 0.019
100			0.151		 0.148

Vijay Kumar (2):
  sparc64: Add a new hypercall CPU_POKE
  sparc64: Use cpu_poke to resume idle cpu

 arch/sparc/include/asm/hypervisor.h |   17 ++++++++
 arch/sparc/include/asm/smp_64.h     |    5 ++
 arch/sparc/kernel/hvapi.c           |    9 ++++
 arch/sparc/kernel/hvcalls.S         |   11 +++++
 arch/sparc/kernel/process_64.c      |    7 +++-
 arch/sparc/kernel/setup_64.c        |    1 +
 arch/sparc/kernel/smp_64.c          |   75 +++++++++++++++++++++++++++++++++-
 7 files changed, 121 insertions(+), 4 deletions(-)
--

[PATCH 1/2] sparc64: Add a new hypercall CPU_POKE

[Vijay Kumar]

This adds a new hypercall CPU_POKE for quickly waking up an idle CPU.
CPU POKE should only be sent to valid  non-local CPUs.

Signed-off-by: Rob Gardner <rob.gardner@oracle.com>
Signed-off-by: Vijay Kumar <vijay.ac.kumar@oracle.com>
Reviewed-by: Anthony Yznaga <anthony.yznaga@oracle.com>
---
 arch/sparc/include/asm/hypervisor.h |   18 ++++++++++++++++++
 arch/sparc/kernel/hvcalls.S         |   11 +++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/include/asm/hypervisor.h b/arch/sparc/include/asm/hypervisor.h
index 73cb897..3dc9215 100644
--- a/arch/sparc/include/asm/hypervisor.h
+++ b/arch/sparc/include/asm/hypervisor.h
@@ -298,6 +298,24 @@ unsigned long sun4v_cpu_start(unsigned long cpuid,
 unsigned long sun4v_cpu_yield(void);
 #endif
 
+/* cpu_poke()
+ * TRAP:	HV_FAST_TRAP
+ * FUNCTION:	HV_FAST_CPU_POKE
+ * RET0:	status
+ * ERRORS:	ENOCPU		cpuid refers to a CPU that does not exist
+ *		EINVAL		cpuid is current CPU
+ *
+ * Poke CPU cpuid. If the target CPU is currently suspended having
+ * invoked the cpu-yield service, that vCPU will be resumed.
+ * Poke interrupts may only be sent to valid, non-local CPUs.
+ * It is not legal to poke the current vCPU.
+ */
+#define HV_FAST_CPU_POKE                0x13
+
+#ifndef __ASSEMBLY__
+unsigned long sun4v_cpu_poke(unsigned long cpuid);
+#endif
+
 /* cpu_qconf()
  * TRAP:	HV_FAST_TRAP
  * FUNCTION:	HV_FAST_CPU_QCONF
diff --git a/arch/sparc/kernel/hvcalls.S b/arch/sparc/kernel/hvcalls.S
index 4116ee5..e57007f 100644
--- a/arch/sparc/kernel/hvcalls.S
+++ b/arch/sparc/kernel/hvcalls.S
@@ -106,6 +106,17 @@ ENTRY(sun4v_cpu_yield)
 	 nop
 ENDPROC(sun4v_cpu_yield)
 
+	/* %o0: cpuid
+	 *
+	 * returns %o0:	status
+	 */
+ENTRY(sun4v_cpu_poke)
+	mov     HV_FAST_CPU_POKE, %o5
+	ta      HV_FAST_TRAP
+	retl
+	 nop
+ENDPROC(sun4v_cpu_poke)
+
 	/* %o0:	type
 	 * %o1:	queue paddr
 	 * %o2:	num queue entries
-- 
1.7.1

[PATCH 2/2] sparc64: Use cpu_poke to resume idle cpu

[Vijay Kumar]

Use cpu_poke hypervisor call to resume idle cpu if supported.

Signed-off-by: Vijay Kumar <vijay.ac.kumar@oracle.com>
Reviewed-by: Anthony Yznaga <anthony.yznaga@oracle.com>
---
 arch/sparc/include/asm/smp_64.h |    5 ++
 arch/sparc/kernel/hvapi.c       |    2 +-
 arch/sparc/kernel/process_64.c  |    7 +++-
 arch/sparc/kernel/setup_64.c    |    1 +
 arch/sparc/kernel/smp_64.c      |   80 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/arch/sparc/include/asm/smp_64.h b/arch/sparc/include/asm/smp_64.h
index ce2233f..a750892 100644
--- a/arch/sparc/include/asm/smp_64.h
+++ b/arch/sparc/include/asm/smp_64.h
@@ -33,6 +33,9 @@
 DECLARE_PER_CPU(cpumask_t, cpu_sibling_map);
 extern cpumask_t cpu_core_map[NR_CPUS];
 
+void smp_init_cpu_poke(void);
+void scheduler_poke(void);
+
 void arch_send_call_function_single_ipi(int cpu);
 void arch_send_call_function_ipi_mask(const struct cpumask *mask);
 
@@ -74,6 +77,8 @@
 #define smp_fetch_global_regs() do { } while (0)
 #define smp_fetch_global_pmu() do { } while (0)
 #define smp_fill_in_cpu_possible_map() do { } while (0)
+#define smp_init_cpu_poke() do { } while (0)
+#define scheduler_poke() do { } while (0)
 
 #endif /* !(CONFIG_SMP) */
 
diff --git a/arch/sparc/kernel/hvapi.c b/arch/sparc/kernel/hvapi.c
index 2677312..0b070d5 100644
--- a/arch/sparc/kernel/hvapi.c
+++ b/arch/sparc/kernel/hvapi.c
@@ -189,7 +189,7 @@ void __init sun4v_hvapi_init(void)
 
 	group = HV_GRP_CORE;
 	major = 1;
-	minor = 1;
+	minor = 6; /* CPU POKE */
 	if (sun4v_hvapi_register(group, major, &minor))
 		goto bad;
 
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index 1badc49..92448af 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -77,8 +77,13 @@ void arch_cpu_idle(void)
 			: "=&r" (pstate)
 			: "i" (PSTATE_IE));
 
-		if (!need_resched() && !cpu_is_offline(smp_processor_id()))
+		if (!need_resched() && !cpu_is_offline(smp_processor_id())) {
 			sun4v_cpu_yield();
+			/* If resumed by cpu_poke then we need to explicitly
+			 * call scheduler_ipi().
+			 */
+			scheduler_poke();
+		}
 
 		/* Re-enable interrupts. */
 		__asm__ __volatile__(
diff --git a/arch/sparc/kernel/setup_64.c b/arch/sparc/kernel/setup_64.c
index 422b178..4ff9fd8 100644
--- a/arch/sparc/kernel/setup_64.c
+++ b/arch/sparc/kernel/setup_64.c
@@ -356,6 +356,7 @@ void __init start_early_boot(void)
 	check_if_starfire();
 	per_cpu_patch();
 	sun4v_patch();
+	smp_init_cpu_poke();
 
 	cpu = hard_smp_processor_id();
 	if (cpu >= NR_CPUS) {
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
index fdf3104..9c3131b 100644
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -74,6 +74,9 @@
 
 static cpumask_t smp_commenced_mask;
 
+static DEFINE_PER_CPU(bool, poke);
+static bool cpu_poke;
+
 void smp_info(struct seq_file *m)
 {
 	int i;
@@ -1394,15 +1397,86 @@ void __init smp_cpus_done(unsigned int max_cpus)
 {
 }
 
+static void send_cpu_ipi(int cpu)
+{
+	xcall_deliver((u64) &xcall_receive_signal,
+			0, 0, cpumask_of(cpu));
+}
+
+void scheduler_poke(void)
+{
+	if (!cpu_poke)
+		return;
+
+	if (!__this_cpu_read(poke))
+		return;
+
+	__this_cpu_write(poke, false);
+	set_softint(1 << PIL_SMP_RECEIVE_SIGNAL);
+}
+
+static unsigned long send_cpu_poke(int cpu)
+{
+	unsigned long hv_err;
+
+	per_cpu(poke, cpu) = true;
+	hv_err = sun4v_cpu_poke(cpu);
+	if (hv_err != HV_EOK) {
+		per_cpu(poke, cpu) = false;
+		pr_err_ratelimited("%s: sun4v_cpu_poke() fails err=%lu\n",
+				    __func__, hv_err);
+	}
+
+	return hv_err;
+}
+
 void smp_send_reschedule(int cpu)
 {
 	if (cpu == smp_processor_id()) {
 		WARN_ON_ONCE(preemptible());
 		set_softint(1 << PIL_SMP_RECEIVE_SIGNAL);
-	} else {
-		xcall_deliver((u64) &xcall_receive_signal,
-			      0, 0, cpumask_of(cpu));
+		return;
+	}
+
+	/* Use cpu poke to resume idle cpu if supported*/
+	if (cpu_poke && idle_cpu(cpu)) {
+		unsigned long ret;
+
+		ret = send_cpu_poke(cpu);
+		if (ret == HV_EOK)
+			return;
 	}
+
+	/* Use IPI in following cases:
+	 * - cpu poke not supported
+	 * - cpu not idle
+	 * - send_cpu_poke() returns with error.
+	 */
+	send_cpu_ipi(cpu);
+}
+
+void smp_init_cpu_poke(void)
+{
+	unsigned long major;
+	unsigned long minor;
+	int ret;
+
+	if (tlb_type != hypervisor)
+		return;
+
+	ret = sun4v_hvapi_get(HV_GRP_CORE, &major, &minor);
+	if (ret) {
+		pr_debug("HV_GRP_CORE is not registered\n");
+		return;
+	}
+
+	if (major == 1 && minor >= 6) {
+		/*cpu poke is registered. */
+		cpu_poke = true;
+		return;
+	}
+
+	pr_debug("CPU_POKE not supported\n");
 }
 
 void __irq_entry smp_receive_signal_client(int irq, struct pt_regs *regs)
-- 
1.7.1

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[David Miller]

From: Vijay Kumar <vijay.ac.kumar@oracle.com>
Date: Sat,  8 Jul 2017 14:23:42 -0600

> cpu_poke is a low latency path to resume the target cpu if suspended
> using cpu_yield. Use cpu poke to resume cpu if supported by hypervisor.
> 
> 	     hackbench results (lower is better):
> Number of		
> Process:		w/o fix		with fix
> 1  			0.012		 0.010
> 10			0.021		 0.019
> 100			0.151		 0.148

So this only works for a cpu which has yielded.

The kernel sends reschedule events to both idle and non-idle cpus.
That's why you have to have that fallback code to still send the
mondo IPI right?

For the case where POKE works, it seems like completely unnecessary
overhead to set the PIL interrupt.  Just disable local cpu interrupts
and call schedule_ipi() directly.

I bet that improves your benchmark even more.

Re: [PATCH 2/2] sparc64: Use cpu_poke to resume idle cpu

[David Miller]

From: Vijay Kumar <vijay.ac.kumar@oracle.com>
Date: Sat,  8 Jul 2017 14:23:44 -0600

> diff --git a/arch/sparc/kernel/hvapi.c b/arch/sparc/kernel/hvapi.c
> index 2677312..0b070d5 100644
> --- a/arch/sparc/kernel/hvapi.c
> +++ b/arch/sparc/kernel/hvapi.c
> @@ -189,7 +189,7 @@ void __init sun4v_hvapi_init(void)
>  
>  	group = HV_GRP_CORE;
>  	major = 1;
> -	minor = 1;
> +	minor = 6; /* CPU POKE */
>  	if (sun4v_hvapi_register(group, major, &minor))
>  		goto bad;

That CPU POKE comment will not stand the test of time, please remove it.

> +	/* Use cpu poke to resume idle cpu if supported*/

Please put a space at the end of the comment and before the "*/"

> +		/*cpu poke is registered. */

Please put a space at the beginning of the comment.

And you should decide which way you want to consistently write.
Either capitalize the first word and finish the sentence with
a '.', or don't.  Do it the same way each time.

Thanks.

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[Vijay Kumar]

On 7/20/2017 2:57 PM, David Miller wrote:
> From: Vijay Kumar <vijay.ac.kumar@oracle.com>
> Date: Sat,  8 Jul 2017 14:23:42 -0600
>
>> cpu_poke is a low latency path to resume the target cpu if suspended
>> using cpu_yield. Use cpu poke to resume cpu if supported by hypervisor.
>>
>> 	     hackbench results (lower is better):
>> Number of		
>> Process:		w/o fix		with fix
>> 1  			0.012		 0.010
>> 10			0.021		 0.019
>> 100			0.151		 0.148
> So this only works for a cpu which has yielded.
>
> The kernel sends reschedule events to both idle and non-idle cpus.
> That's why you have to have that fallback code to still send the
> mondo IPI right?
That is correct.

>
> For the case where POKE works, it seems like completely unnecessary
> overhead to set the PIL interrupt.  Just disable local cpu interrupts
> and call schedule_ipi() directly.
>
> I bet that improves your benchmark even more.

I had same thoughts initially but I had to go with this approach as 
scheduler_ipi is wrapped with irq_enter() and irq_exit(). Whereas POKE 
resumes the cpu in process context.

Comments in scheduler_ipi():

          * Not all reschedule IPI handlers call irq_enter/irq_exit, since
          * traditionally all their work was done from the interrupt return
          * path. Now that we actually do some work, we need to make sure
          * we do call them.
          *
          * Some archs already do call them, luckily irq_enter/exit nest
          * properly.
          *
          * Arguably we should visit all archs and update all handlers,
          * however a fair share of IPIs are still resched only so this 
would
          * somewhat pessimize the simple resched case.
          */
         irq_enter();


-Vijay

Re: [PATCH 2/2] sparc64: Use cpu_poke to resume idle cpu

[Vijay Kumar]

On 7/20/2017 2:58 PM, David Miller wrote:
> From: Vijay Kumar <vijay.ac.kumar@oracle.com>
> Date: Sat,  8 Jul 2017 14:23:44 -0600
>
>> diff --git a/arch/sparc/kernel/hvapi.c b/arch/sparc/kernel/hvapi.c
>> index 2677312..0b070d5 100644
>> --- a/arch/sparc/kernel/hvapi.c
>> +++ b/arch/sparc/kernel/hvapi.c
>> @@ -189,7 +189,7 @@ void __init sun4v_hvapi_init(void)
>>   
>>   	group = HV_GRP_CORE;
>>   	major = 1;
>> -	minor = 1;
>> +	minor = 6; /* CPU POKE */
>>   	if (sun4v_hvapi_register(group, major, &minor))
>>   		goto bad;
> That CPU POKE comment will not stand the test of time, please remove it.
>
>> +	/* Use cpu poke to resume idle cpu if supported*/
> Please put a space at the end of the comment and before the "*/"
>
>> +		/*cpu poke is registered. */
> Please put a space at the beginning of the comment.
>
> And you should decide which way you want to consistently write.
> Either capitalize the first word and finish the sentence with
> a '.', or don't.  Do it the same way each time.
>
> Thanks.
Sure, I will fix these in v2.

Thanks,
-Vijay

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[David Miller]

From: Vijay Kumar <vijay.ac.kumar@oracle.com>
Date: Thu, 20 Jul 2017 21:44:24 -0500

> I had same thoughts initially but I had to go with this approach as
> scheduler_ipi is wrapped with irq_enter() and irq_exit(). Whereas POKE
> resumes the cpu in process context.
> 
> Comments in scheduler_ipi():
> 
>          * Not all reschedule IPI handlers call irq_enter/irq_exit, since
>          * traditionally all their work was done from the interrupt return
>          * path. Now that we actually do some work, we need to make sure
>          * we do call them.
>          *
>          * Some archs already do call them, luckily irq_enter/exit nest
>          * properly.
>          *
>          * Arguably we should visit all archs and update all handlers,
>          * however a fair share of IPIs are still resched only so this would
>          * somewhat pessimize the simple resched case.
>          */
>         irq_enter();
> 

I still think we should be able to fake the state such that this
direct schedule_ipi() call will work.

I could be wrong :)

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[Vijay Kumar]

On 7/20/2017 9:55 PM, David Miller wrote:
> From: Vijay Kumar <vijay.ac.kumar@oracle.com>
> Date: Thu, 20 Jul 2017 21:44:24 -0500
>
>> I had same thoughts initially but I had to go with this approach as
>> scheduler_ipi is wrapped with irq_enter() and irq_exit(). Whereas POKE
>> resumes the cpu in process context.
>>
>> Comments in scheduler_ipi():
>>
>>           * Not all reschedule IPI handlers call irq_enter/irq_exit, since
>>           * traditionally all their work was done from the interrupt return
>>           * path. Now that we actually do some work, we need to make sure
>>           * we do call them.
>>           *
>>           * Some archs already do call them, luckily irq_enter/exit nest
>>           * properly.
>>           *
>>           * Arguably we should visit all archs and update all handlers,
>>           * however a fair share of IPIs are still resched only so this would
>>           * somewhat pessimize the simple resched case.
>>           */
>>          irq_enter();
>>
> I still think we should be able to fake the state such that this
> direct schedule_ipi() call will work.
>
> I could be wrong :)
I can give a try :). But looks to me one thing that will go wrong is irq 
accounting done in __irq_enter() and rcu_irq_enter().

Thanks,
Vijay

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[David Miller]

From: Vijay Kumar <vijay.ac.kumar@oracle.com>
Date: Thu, 20 Jul 2017 22:36:42 -0500

> I can give a try :). But looks to me one thing that will go wrong is
> irq accounting done in __irq_enter() and rcu_irq_enter().

Actually, the bigger problem is that scheduler_ipi() can raise a
software interrupt, and nothing will invoke it.

It's turning quite ugly to avoid the IRQ overhead, I must admit.
So ignore this for now.

In the longer term a probably cleaner way to do this is to have
a special direct version of scheduler_ipi() that invokes all the
necessary work, even the rebalance softirq, directly rather than
indirectly.

Re: [PATCH 0/2] sparc64: Use low latency path to resume idle cpu

[Vijay Kumar]

On 7/20/2017 10:45 PM, David Miller wrote:
> From: Vijay Kumar <vijay.ac.kumar@oracle.com>
> Date: Thu, 20 Jul 2017 22:36:42 -0500
>
>> I can give a try :). But looks to me one thing that will go wrong is
>> irq accounting done in __irq_enter() and rcu_irq_enter().
> Actually, the bigger problem is that scheduler_ipi() can raise a
> software interrupt, and nothing will invoke it.
Yes, I see your point.

> It's turning quite ugly to avoid the IRQ overhead, I must admit.
> So ignore this for now.
>
> In the longer term a probably cleaner way to do this is to have
> a special direct version of scheduler_ipi() that invokes all the
> necessary work, even the rebalance softirq, directly rather than
> indirectly.

Sure. Thanks.