[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 10/05/18 16:49, Mirela Simonovic wrote:
Hi Julien,

Hi,

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.

I was aware about it since the beginning. The whole point of the conversation was we should avoid to take the decision at the lower level and let the upper layer decide what to do.

If the system is failing today then that's fine and still fit what I said in my first e-mail of that thread. For reminder:

"We should really avoid to use panic(...) if this is something the system can survive. In that specific case, it would only affect the current CPU. So it would be better to return an error and let the caller decide what to do."


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 :)

What's the problem to escalate the error? That's how it works in most of the case. Imagine a driver failing, are you going to decide to crash there? Very likely no, you will report the error and let the upper layer here.

Also, while I suggest that it could be replaced by stop_cpu() in the common code, I also suggested that notifier_cpu_starting() could return an error then the architecture specific code can decide what to do.

On x86 it would still be a BUG_ON(notifier_cpu_starting()). On Arm we can decide what to do. But it is not part of that discussion here.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.