[PATCH] kvm-userland mm count skew

[PATCH] kvm-userland mm count skew

[Andrea Arcangeli]

I still can't see how it could be possibly make a difference for the
mm_count if the kvm module is compiled inside the kernel or as an
external module, the reference counting there hasn't changed since
ages. The mmdrop fires only in the first overflow so even if I'm right
it probably wasn't much destabilizing to go negative given it happened
at mm destruction time.

Signed-off-by: Andrea Arcangeli <andrea-atKUWr5tajBWk0Htik3J/w@public.gmane.org>

diff --git a/kernel/hack-module.awk b/kernel/hack-module.awk
index 5187c96..884bc50 100644
--- a/kernel/hack-module.awk
+++ b/kernel/hack-module.awk
@@ -33,8 +33,6 @@
     vmx_load_host_state = 0
 }
 
-/atomic_inc\(&kvm->mm->mm_count\);/ { $0 = "//" $0 }
-
 /^\t\.fault = / {
     fcn = gensub(/,/, "", "g", $3)
     $0 = "\t.VMA_OPS_FAULT(fault) = VMA_OPS_FAULT_FUNC(" fcn "),"

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm-userland mm count skew

[Avi Kivity]

Andrea Arcangeli wrote:
> I still can't see how it could be possibly make a difference for the
> mm_count if the kvm module is compiled inside the kernel or as an
> external module, the reference counting there hasn't changed since
> ages. The mmdrop fires only in the first overflow so even if I'm right
> it probably wasn't much destabilizing to go negative given it happened
> at mm destruction time.
>
>   

It's this bit:

> /* __mmdrop() is not exported before 2.6.25 */
> #include <linux/sched.h>
>
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,25)
>
> #define mmdrop(x) do { (void)(x); } while (0)
>
> #endif


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm-userland mm count skew

[Andrea Arcangeli]

On Tue, Jan 22, 2008 at 03:41:02PM +0200, Avi Kivity wrote:
> Andrea Arcangeli wrote:
>> I still can't see how it could be possibly make a difference for the
>> mm_count if the kvm module is compiled inside the kernel or as an
>> external module, the reference counting there hasn't changed since
>> ages. The mmdrop fires only in the first overflow so even if I'm right
>> it probably wasn't much destabilizing to go negative given it happened
>> at mm destruction time.
>>
>>   
>
> It's this bit:

Ok. But the atomic_inc removal isn't conditional to < 2.6.25, so it
still doesn't look good to me. it would look better if we would
unconditionally define mmdrop to nop in the external module
compile. The other problem is that I don't see why atomic_inc/mmdrop
are needed at all if the external module is safe, so why don't we drop
them? In ->release->kvm_destroy_vm it seems the kvm->mm is never used
anyway.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm-userland mm count skew

[Avi Kivity]

Andrea Arcangeli wrote:
> On Tue, Jan 22, 2008 at 03:41:02PM +0200, Avi Kivity wrote:
>   
>> Andrea Arcangeli wrote:
>>     
>>> I still can't see how it could be possibly make a difference for the
>>> mm_count if the kvm module is compiled inside the kernel or as an
>>> external module, the reference counting there hasn't changed since
>>> ages. The mmdrop fires only in the first overflow so even if I'm right
>>> it probably wasn't much destabilizing to go negative given it happened
>>> at mm destruction time.
>>>
>>>   
>>>       
>> It's this bit:
>>     
>
> Ok. But the atomic_inc removal isn't conditional to < 2.6.25, so it
> still doesn't look good to me. 

Right.  Not hurting in practice since 2.6.25 has yet to be released, but 
it needs fixing.

> it would look better if we would
> unconditionally define mmdrop to nop in the external module
> compile. The other problem is that I don't see why atomic_inc/mmdrop
> are needed at all if the external module is safe, so why don't we drop
> them? In ->release->kvm_destroy_vm it seems the kvm->mm is never used
> anyway.
>   

The external module isn't safe, it just works in practice.

The meaning of hvas and ->mmap_sem (and mmu notifiers) is dependent on 
->mm, so we must be sure that kvm doesn't get called with the wrong mm.  
Switching to a syscall based API would also cure this, but it's a lot 
more work.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/