Waiman Long
2015-Mar-19 02:45 UTC
[PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock
On 03/16/2015 09:16 AM, Peter Zijlstra wrote:> Implement the paravirt qspinlock for x86-kvm. > > We use the regular paravirt call patching to switch between: > > native_queue_spin_lock_slowpath() __pv_queue_spin_lock_slowpath() > native_queue_spin_unlock() __pv_queue_spin_unlock() > > We use a callee saved call for the unlock function which reduces the > i-cache footprint and allows 'inlining' of SPIN_UNLOCK functions > again. > > We further optimize the unlock path by patching the direct call with a > "movb $0,%arg1" if we are indeed using the native unlock code. This > makes the unlock code almost as fast as the !PARAVIRT case. > > This significantly lowers the overhead of having > CONFIG_PARAVIRT_SPINLOCKS enabled, even for native code. > > > Signed-off-by: Peter Zijlstra (Intel)<peterz at infradead.org>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. -Longman -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20150318/2edb54cc/attachment-0001.html>
Peter Zijlstra
2015-Mar-19 10:01 UTC
[PATCH 9/9] qspinlock,x86,kvm: Implement KVM support for paravirt qspinlock
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'.
Waiman Long
2015-Mar-19 21:08 UTC
[PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock
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
Apparently Analagous Threads
- [PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock
- [PATCH 9/9] qspinlock,x86,kvm: Implement KVM support for paravirt qspinlock
- [PATCH 9/9] qspinlock,x86,kvm: Implement KVM support for paravirt qspinlock
- [PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock
- [PATCH 9/9] qspinlock, x86, kvm: Implement KVM support for paravirt qspinlock