Re: [Xen-devel] [PATCH v3 10/10] xen/arm: Enable errata for secondary CPU on hotplug after the boot

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

On Thu, May 10, 2018 at 5:13 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 10/05/18 16:00, Mirela Simonovic wrote:
>>
>> Hi Dario,
>>
>> On Thu, May 10, 2018 at 4:25 PM, Dario Faggioli <dfaggioli@xxxxxxxx>
>> wrote:
>>>
>>> On Thu, 2018-05-10 at 15:24 +0200, Mirela Simonovic wrote:
>>>>
>>>> On Thu, May 10, 2018 at 1:57 PM, Mirela Simonovic
>>>>
>>>>> Please take a look at function cpu_schedule_callback in schedule.c.
>>>>> Within switch, case CPU_DEAD doesn't have a break, causing the
>>>>> bellow
>>>>> CPU_UP_CANCELED to execute as well when the CPU goes down. This
>>>>> looks
>>>>> wrong to me.
>>>>> Dario, could you please confirm that this is a bug? Otherwise could
>>>>> you please confirm the reasoning beyond?
>>>>>
>>>>
>>>> Dario sorry, this looked wrong in my scenario but actually it is
>>>> correct. I understand the purpose of the missing break now.
>>>>
>>> No problem.
>>>
>>>> For the curious ones (if any) here is detailed description: errata
>>>> notifier added in this patch had the same priority as scheduler
>>>> notifier. I though priority doesn't matter, but I was wrong. In this
>>>> particular scenario where a CPU fails to enable capabilities
>>>> (triggered by errata notifier added in this patch), scheduler
>>>> callback
>>>> executed before the errata callback for CPU_STARTING event.
>>>>
>>> So, you're adding your errata callback to the CPU_STARTING notifier,
>>> right? (Sorry for having to ask, I don't have the patch handy right
>>> now.)
>>>
>>>> In other
>>>> words, scheduler already called init_pdata before the errata callback
>>>> fired (and stopped the CPU).
>>>> Later on when errata callback fired, enabling of capabilities has
>>>> failed, so the erroneous non-boot CPU stopped itself and never
>>>> declared to be online.
>>>> Then CPU#0 fired new notification with CPU_UP_CANCELED event in order
>>>> to clean up for the job done on CPU_STARTING. However, this broke the
>>>> assumption (which is good) made in cpu_schedule_callback. The
>>>> assumption is that the sequence of steps should be
>>>> alloc_pdata-->init_pdata-->deinit_pdata-->free_pdata. In this
>>>> particular case deinit_pdata was not done because this would be done
>>>> only upon CPU_DEAD event which makes no sense in this scenario.
>>>> In order to avoid running into the invalid scenario described above,
>>>> the errata callback should fire before the scheduler callback. If
>>>> enabling capabilities fails, the scheduler callback for CPU_STARTING
>>>> will never execute afterwards, so the following CPU_UP_CANCELED event
>>>> triggered by the CPU#0 will do free_pdata, which is ok because
>>>> init_pdata was not executed and alloc_pdata-->free_pdata flow is also
>>>> valid. Congratulations to the reader who reached this point :)
>>>>
>>> Ok, but the flow is, AFAICR, CPU_UP_PREPARE->CPU_STARTING->CPU_ONLINE.
>>>
>>> If you add your callback to CPU_UP_PREPARE, instead than to
>>> CPU_STARTING, SCHED_OP(init_pdata) wouldn't be called, without having
>>> to fiddle with priorities.
>
> This function will enable capabilities on a given CPU, most of them are
> touching system registers. So it is necessary to add the callback to
> CPU_STARTING.
>
>>>
>>
>> Difference between CPU_UP_PREPARE and CPU_STARTING (in addition to the
>> sequential ordering) is about which CPU executes the callback.
>> In CPU_UP_PREPARE case the CPU which called cpu_up for another CPU
>> will execute the callback. If I have 2 CPUs: CPU#0 executes callback
>> when trying to hotplug CPU#1. I need CPU#1 to execute this callback.
>> In CPU_STARTING case the CPU#1 will execute the callback, that is the
>> reason why it has to be CPU_STARTING event.
>>
>> I tried CPU_UP_PREPARE in my tuned scenario and I needed few moment to
>> realize why the system died (CPU#0 stopped himself :)
>>
>>> Is there any reason why you can't do it that way? It would look more
>>> natural to me, and it's definitely going to be easier debug and
>>> maintain (e.g., look at how many callbacks CPU_UP_PREPARE has, as
>>> compared to CPU_STARTING ;-P).
>>>
>>
>> Julien is going to maintain this :)))
>
>
> I always like to understand what I maintain :). Why do you need to change
> the priority if you just return an error in the notifier?
>

Regardless of the fact that the notifier returns an error or not, I
believe it would be good and safe to set priority and document that
priority zero would cause racing issue in the scenario I debugged
today. I'm pretty sure that this discussion would be forgotten soon
and it really should be documented in code/comment.

In emails above I assumed we'll stop the erroneous CPU. I didn't have
a chance to try returning an error until few minutes ago.
I tried returning an error from the notifier now and the whole system
fails. You realized according to the answer below that this is going
to happen.

> At the moment, notify_cpu_starting has a BUG_ON() in it. But ideally I would
> like to either replace that BUG_ON by cpu_stop or just returning an error to
> give a chance of the architecture deciding what to do (this does not have to
> be done today).
>

I would rather stop CPU because changing notify_cpu_starting affects
x86 as well, I cannot dig into that and it would be really to much for
this series. Since you're fine with stopping cpu as well, please lets
do that instead of escalating this to who knows where :)

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

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