diff mbox series

[kvm-unit-tests,v3,4/9] s390x: smp: Rework cpu start and active tracking

Message ID 20200117104640.1983-5-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: smp: Improve smp code and reset checks | expand

Commit Message

Janosch Frank Jan. 17, 2020, 10:46 a.m. UTC 
The architecture specifies that processing sigp orders may be
asynchronous, and this is indeed the case on some hypervisors, so we
need to wait until the cpu runs before we return from the setup/start
function.

As there was a lot of duplicate code, a common function for cpu
restarts has been introduced.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/smp.c | 50 ++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

Comments

David Hildenbrand Jan. 20, 2020, 12:06 p.m. UTC | #1 
On 17.01.20 11:46, Janosch Frank wrote:
> The architecture specifies that processing sigp orders may be
> asynchronous, and this is indeed the case on some hypervisors, so we
> need to wait until the cpu runs before we return from the setup/start
> function.
> 
> As there was a lot of duplicate code, a common function for cpu
> restarts has been introduced.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/smp.c | 50 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index f57f420..84e681d 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -104,35 +104,46 @@ int smp_cpu_stop_store_status(uint16_t addr)
>  	return rc;
>  }
>  
> +static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
> +{
> +	int rc;
> +	struct cpu *cpu = smp_cpu_from_addr(addr);

I'd exchange these two (reverse christmas tree)

> +
> +	if (!cpu)
> +		return -1;

-EINVAL?

> +	if (psw) {
> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
> +	}

Does this make sense to have optional? (the other CPU will execute
random crap if not set, won't it?)

> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
> +	if (rc)
> +		return rc;
> +	/*
> +	 * The order has been accepted, but the actual restart may not
> +	 * have been performed yet, so wait until the cpu is running.
> +	 */
> +	while (!smp_cpu_running(addr))
> +		mb();

Should you make sure to stop the CPU before issuing the restart?
Otherwise you will get false positives if it is still running (but
hasn't processed the RESTART yet)
Thomas Huth Jan. 20, 2020, 1:16 p.m. UTC | #2 
On 20/01/2020 13.06, David Hildenbrand wrote:
> On 17.01.20 11:46, Janosch Frank wrote:
[...]
>> +
>> +	if (!cpu)
>> +		return -1;
> 
> -EINVAL?

-ENOERRNOHEADERINKVMUNITTESTS ;-)

 Thomas
David Hildenbrand Jan. 20, 2020, 1:20 p.m. UTC | #3 
On 20.01.20 14:16, Thomas Huth wrote:
> On 20/01/2020 13.06, David Hildenbrand wrote:
>> On 17.01.20 11:46, Janosch Frank wrote:
> [...]
>>> +
>>> +	if (!cpu)
>>> +		return -1;
>>
>> -EINVAL?
> 
> -ENOERRNOHEADERINKVMUNITTESTS ;-)
> 

-EDAMMIT :)
Janosch Frank Jan. 20, 2020, 2:47 p.m. UTC | #4 
On 1/20/20 1:06 PM, David Hildenbrand wrote:
> On 17.01.20 11:46, Janosch Frank wrote:
>> The architecture specifies that processing sigp orders may be
>> asynchronous, and this is indeed the case on some hypervisors, so we
>> need to wait until the cpu runs before we return from the setup/start
>> function.
>>
>> As there was a lot of duplicate code, a common function for cpu
>> restarts has been introduced.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  lib/s390x/smp.c | 50 ++++++++++++++++++++++++++++---------------------
>>  1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index f57f420..84e681d 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -104,35 +104,46 @@ int smp_cpu_stop_store_status(uint16_t addr)
>>  	return rc;
>>  }
>>  
>> +static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
>> +{
>> +	int rc;
>> +	struct cpu *cpu = smp_cpu_from_addr(addr);
> 
> I'd exchange these two (reverse christmas tree)

Christmas is over

> 
>> +
>> +	if (!cpu)
>> +		return -1;
> 
> -EINVAL?
> 
>> +	if (psw) {
>> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
>> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
>> +	}
> 
> Does this make sense to have optional? (the other CPU will execute
> random crap if not set, won't it?)

Well, I have restarts in the smp test and I don't want to always pass a
psw if I know what the last restart psw was.
Simply restarting into test_func or wait_for_flag is certainly no problem.

> 
>> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
>> +	if (rc)
>> +		return rc;
>> +	/*
>> +	 * The order has been accepted, but the actual restart may not
>> +	 * have been performed yet, so wait until the cpu is running.
>> +	 */
>> +	while (!smp_cpu_running(addr))
>> +		mb();
> 
> Should you make sure to stop the CPU before issuing the restart?
> Otherwise you will get false positives if it is still running (but
> hasn't processed the RESTART yet)

Good point
David Hildenbrand Jan. 20, 2020, 2:53 p.m. UTC | #5 
> Well, I have restarts in the smp test and I don't want to always pass a
> psw if I know what the last restart psw was.
> Simply restarting into test_func or wait_for_flag is certainly no problem.
> 
>>

Makes sense.

>>> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
>>> +	if (rc)
>>> +		return rc;
>>> +	/*
>>> +	 * The order has been accepted, but the actual restart may not
>>> +	 * have been performed yet, so wait until the cpu is running.
>>> +	 */
>>> +	while (!smp_cpu_running(addr))
>>> +		mb();
>>
>> Should you make sure to stop the CPU before issuing the restart?
>> Otherwise you will get false positives if it is still running (but
>> hasn't processed the RESTART yet)
> 
> Good point
>
diff mbox series

Patch

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index f57f420..84e681d 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -104,35 +104,46 @@  int smp_cpu_stop_store_status(uint16_t addr)
 	return rc;
 }
 
+static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
+{
+	int rc;
+	struct cpu *cpu = smp_cpu_from_addr(addr);
+
+	if (!cpu)
+		return -1;
+	if (psw) {
+		cpu->lowcore->restart_new_psw.mask = psw->mask;
+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;
+	/*
+	 * The order has been accepted, but the actual restart may not
+	 * have been performed yet, so wait until the cpu is running.
+	 */
+	while (!smp_cpu_running(addr))
+		mb();
+	cpu->active = true;
+	return 0;
+}
+
 int smp_cpu_restart(uint16_t addr)
 {
-	int rc = -1;
-	struct cpu *cpu;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-		cpu->active = true;
-	}
+	rc = smp_cpu_restart_nolock(addr, NULL);
 	spin_unlock(&lock);
 	return rc;
 }
 
 int smp_cpu_start(uint16_t addr, struct psw psw)
 {
-	int rc = -1;
-	struct cpu *cpu;
-	struct lowcore *lc;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		lc = cpu->lowcore;
-		lc->restart_new_psw.mask = psw.mask;
-		lc->restart_new_psw.addr = psw.addr;
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-	}
+	rc = smp_cpu_restart_nolock(addr, &psw);
 	spin_unlock(&lock);
 	return rc;
 }
@@ -192,10 +203,7 @@  int smp_cpu_setup(uint16_t addr, struct psw psw)
 	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
 	/* Start processing */
-	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);
-	if (!rc)
-		cpu->active = true;
-
+	smp_cpu_restart_nolock(addr, NULL);
 out:
 	spin_unlock(&lock);
 	return rc;