Re: [Xen-devel] [PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock

[Waiman Long <waiman.long@xxxxxx>]

On 03/19/2015 06:01 AM, Peter Zijlstra wrote:
> On Wed, Mar 18, 2015 at 10:45:55PM -0400, Waiman Long wrote:
> > On 03/16/2015 09:16 AM, Peter Zijlstra wrote:
> > I do have some concern about this call site patching mechanism as the
> > modification is not atomic. The spin_unlock() calls are in many places in
> > the kernel. There is a possibility that a thread is calling a certain
> > spin_unlock call site while it is being patched by another one with the
> > alternative() function call.
> >
> > So far, I don't see any problem with bare metal where paravirt_patch_insns()
> > is used to patch it to the move instruction. However, in a virtual guest
> > enivornment where paravirt_patch_call() was used, there were situations
> > where the system panic because of page fault on some invalid memory in the
> > kthread. If you look at the paravirt_patch_call(), you will see:
> >
> >      :
> > b->opcode = 0xe8; /* call */
> > b->delta = delta;
> >
> > If another CPU reads the instruction at the call site at the right moment,
> > it will get the modified call instruction, but not the new delta value. It
> > will then jump to a random location. I believe that was causing the system
> > panic that I saw.
> >
> > So I think it is kind of risky to use it here unless we can guarantee that
> > call site patching is atomic wrt other CPUs.
> Just look at where the patching is done:
>
> init/main.c:start_kernel()
>    check_bugs()
>      alternative_instructions()
>        apply_paravirt()
>
> We're UP and not holding any locks, disable IRQs (see text_poke_early())
> and have NMIs 'disabled'.
You are probably right. The initial apply_paravirt() was done before the 
SMP boot. Subsequent ones were at kernel module load time. I put a 
counter in the __native_queue_spin_unlock() and it registered 26949 
unlock calls in a 16-cpu guest before it got patched out.
The panic that I observed before might be due to some coding error of my 
own.
-Longman

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