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

[Eric DeVolder <eric.devolder@xxxxxxxxxx>]

On 04/19/2017 08:37 AM, Jan Beulich wrote:
> > > > 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.
I've just posted v3 of this patch with the ASSERT in lieu of the 
spin_lock().
Eric



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