Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

From: Juergen Gross <jgross@suse.com>
To: Dario Faggioli <dfaggioli@suse.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED
Date: Mon, 25 Mar 2019 13:29:30 +0100	[thread overview]
Message-ID: <ded64901-abf7-ee07-a7d5-105697d60795@suse.com> (raw)
In-Reply-To: <37954eb9e9efb9b95c603458b298bc1a51b6354f.camel@suse.com>

On 25/03/2019 13:21, Dario Faggioli wrote:
> On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
>> --- a/xen/include/xen/cpu.h
>> +++ b/xen/include/xen/cpu.h
>> @@ -32,23 +32,25 @@ void register_cpu_notifier(struct notifier_block
>> *nb);
>>   *  (a) A CPU is going down; or (b) CPU_UP_CANCELED
>>   */
>>  /* CPU_UP_PREPARE: Preparing to bring CPU online. */
>> -#define CPU_UP_PREPARE   (0x0001 | NOTIFY_FORWARD)
>> +#define CPU_UP_PREPARE    (0x0001 | NOTIFY_FORWARD)
>>
> In the comment block before these definitions, there's this:
> 
>  * Possible event sequences for a given CPU:
>  *  CPU_UP_PREPARE -> CPU_UP_CANCELLED           -- failed CPU up
>  *  CPU_UP_PREPARE -> CPU_STARTING -> CPU_ONLINE -- successful CPU up
>  *  CPU_DOWN_PREPARE -> CPU_DOWN_FAILED          -- failed CPU down
>  *  CPU_DOWN_PREPARE -> CPU_DYING -> CPU_DEAD    -- successful CPU down
> 
> Shouldn't we add a line for this new hook? Something, IIUIC, like:
> 
>  CPU_UP_PREPARE -> CPU_UP_CANCELLED -> CPU_RESUME_FAILED --CPU not resuming

Fine with me.

> 
> With this, FWIW,
> 
> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> 
> One more (minor) thing...
> 
>>  /* CPU_REMOVE: CPU was removed. */
>> -#define CPU_REMOVE       (0x0009 | NOTIFY_REVERSE)
>> +#define CPU_REMOVE        (0x0009 | NOTIFY_REVERSE)
>> +/* CPU_RESUME_FAILED: CPU failed to come up in resume, all other CPUs up. */
>> +#define CPU_RESUME_FAILED (0x000a | NOTIFY_REVERSE)
>>  
> ... technically, when we're dealing with CPU_RESUME_FAILED on one CPU,
> we don't know if _all_ others really went up, so I think I'd remove
> what follows the ','.

The point is that for the CPU_RESUME_FAILED case we can be sure that no
cpu will come up due to resume just a little bit later. So we can test
for e.g. a cpupool suddenly having no more cpus available. This is in
contrast to CPU_UP_CANCELLED being signalled just after the one cpu
failing to come up, but before the next cpu is triggered to come up.

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel