Messages in this thread | | | Date | Thu, 19 Mar 2015 17:08:31 -0400 | From | Waiman Long <> | Subject | Re: [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
| |