|
[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 SimonovicPlease 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |