Re: [Xen-devel] [PATCH v2 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.04.17 at 14:13, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, Apr 19, 2017 at 05:20:50AM -0600, Jan Beulich wrote:
>> >>> On 19.04.17 at 12:56, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Tue, Apr 18, 2017 at 04:49:48AM -0600, Jan Beulich wrote:
>> >> >>> On 17.04.17 at 21:09, <eric.devolder@xxxxxxxxxx> wrote:
>> >> > The spinlock in kexec_swap_images() was removed as
>> >> > this function is only reachable on the kexec hypercall, which is
>> >> > now protected at the top-level in do_kexec_op_internal(),
>> >> > thus the local spinlock is no longer necessary.
>> >>
>> >> But perhaps leave an ASSERT() there, making sure the in-hypercall
>> >> flag is set?
>> >
>> > I am not sure why but if at all I think that we should also consider
>> > other key kexec functions then. Or put ASSERT() into do_kexec_op_internal()
>> > just before "switch ( op )".
>>
>> The point of my placement suggestion was that the ASSERT()
>> effectively replaces the lock acquire - the places you name
>> didn't previously require any synchronization.
> 
> After the first patch of this series kexec_swap_images() cannot be
> started twice in parallel. So, I do not see the point of ASSERT() here.
> Or let's say we wish to have it to double check that "the in-hypercall
> flag is set". AIUI, it is your original idea. However, then I think that
> we should have an ASSERT() at least in kexec_load_slot() because parallel
> loads make issues too. And we can go higher to feel more safe. That is
> why I suggested do_kexec_op_internal() as the final resting place for
> an ASSERT(). Simply it looks to me the safest approach. Am I missing
> something?

The point you're missing is - why don't you then move the ASSERT()
yet one more level up, right next to where the flag is being set? IOW
what you suggest would imo rather mean not adding any assertion
at all.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel