Re: [PATCH 1/2] common/kexec: Prevent deadlock on reentry to the crash path.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/11/13 13:39, Jan Beulich wrote:
>>>> On 25.11.13 at 14:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 25/11/13 13:28, Jan Beulich wrote:
>>>>>> On 15.11.13 at 21:32, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> In some cases, such as suffering a queued-invalidation timeout while
>>>> performing an iommu_crash_shutdown(), Xen can end up reentering the crash
>>>> path. Previously, this would result in a deadlock in one_cpu_only(), as the
>>>> test_and_set_bit() would fail.
>>>>
>>>> The crash path is not reentrant, and even if it could be made to be so, it 
>> is
>>>> almost certain that we would fall over the same reentry condition again.
>>>>
>>>> The new code can distinguish a reentry case from multiple cpus racing down 
>> the
>>>> crash path.  In the case that a reentry is detected, return back out to the
>>>> nested panic() call, which will maybe_reboot() on our behalf.  This requires 
>> a
>>>> bit of return plumbing back up to kexec_crash().
>>>>
>>>> While fixing this deadlock, also fix up an minor niggle seen recently from a
>>>> XenServer crash report.  The report was from a Bank 8 MCE, which had managed
>>>> to crash on all cpus at once.  The result was a lot of stack traces with 
>> cpus
>>>> in kexec_common_shutdown(), which was infact the inlined version of
>>>> one_cpu_only().  The kexec crash path is not a hotpath, so we can easily
>>>> afford to prevent inlining for the sake of clarity in the stack traces.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Keir Fraser <keir@xen.org>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Tim Deegan <tim@xen.org>
>>>> CC: David Vrabel <david.vrabel@citrix.com>
>>>> ---
>>>>  xen/common/kexec.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 44 insertions(+), 7 deletions(-)
>>> David, you being the maintainer of this code now, I don't think I've
>>> seen a response from you on this patch, despite - iirc - Andrew
>>> having pinged you on it already too.
>>>
>>> Jan
>>>
>> David is out of the office for a week on vacation at the moment.
>>
>> I did get code-review from him before submitting it upstream, but I
>> guess that doesn't count for much as a formal ack.
> Then why did you not include his Reviewed-by with the patch?
> That would have sufficed (and put you in a bad light in case he
> came back and said he didn't do any such review - which I trust
> he did if you say so).
>
> Jan
>

I think we had agreed that he would formally review it on-list, but it
was late one evening of a busy week, so I suspect it just got forgotten.

~Andrew