Re: [GIT PULL] First batch of KVM changes for 4.1

[Paolo Bonzini]

On 17/04/2015 15:10, Peter Zijlstra wrote:
> On Fri, Apr 17, 2015 at 02:46:57PM +0200, Paolo Bonzini wrote:
>> On 17/04/2015 12:55, Peter Zijlstra wrote:
>>> Also, it looks like you already do exactly this for other things, look
>>> at:
>>>
>>> 	kvm_sched_in()
>>> 	  kvm_arch_vcpu_load()
>>> 	    if (unlikely(vcpu->cpu != cpu) ... )
>>>
>>> So no, I don't believe for one second you need this.
> 
> This [...] brings us back to where we were last
> time. There is _0_ justification for this in the patches, that alone is
> grounds enough to reject it.

Oh, we totally agree on that.  I didn't commit that patch, but I already
said the commit message was insufficient.

> Why should the guest task care about the physical cpu of the vcpu;
> that's a layering fail if ever there was one.

It's totally within your right to not read the code, but then please
don't try commenting at it.

This code:

	kvm_sched_in()
	  kvm_arch_vcpu_load()
 	    if (unlikely(vcpu->cpu != cpu) ... )

runs in the host.  The hypervisor obviously cares if the physical CPU of
the VCPU changes.  It has to tell the source processor (vcpu->cpu) to
release the VCPU's data structure and only then it can use it in the
target processor (cpu).  No layering violation here.

The task migration notifier runs in the guest, whenever the VCPU of
a task changes.

> Furthermore, the only thing that migration handler seems to do is
> increment a variable that is not actually used in that file.

It's used in the vDSO, so you cannot increment it in the file that uses it.

>> And frankly, I think the static key is snake oil.  The cost of task 
>> migration in terms of cache misses and TLB misses is in no way 
>> comparable to the cost of filling in a structure on the stack, 
>> dereferencing the head of the notifiers list and seeing that it's NULL.
> 
> The path this notifier is called from has nothing to do with those
> costs.

How not?  The task is going to incur those costs, it's not like half
a dozen extra instruction make any difference.  But anyway...

> And the fact you're inflicting these costs on _everyone_ for a
> single x86_64-paravirt case is insane.

... that's a valid objection.  Please look at the patch below.

> I've had enough of this, the below goes into sched/urgent and you can
> come back with sane patches if and when you're ready.

Oh, please, cut the alpha male crap.

Paolo

------------------- 8< ----------------