On 12/07/19 09:15, Wanpeng Li wrote:
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b4ab59d..2c46705 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> int me;
> int cpu = vcpu->cpu;
>
> - if (kvm_vcpu_wake_up(vcpu))
> + if (kvm_vcpu_wake_up(vcpu)) {
> + vcpu->preempted = true;
> return;
> + }
>
> me = get_cpu();
> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>
Who is resetting vcpu->preempted to false in this case? This also
applies to s390 in fact.
Paolo
On 18.07.19 09:59, Paolo Bonzini wrote:
> On 12/07/19 09:15, Wanpeng Li wrote:
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index b4ab59d..2c46705 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> int me;
>> int cpu = vcpu->cpu;
>>
>> - if (kvm_vcpu_wake_up(vcpu))
>> + if (kvm_vcpu_wake_up(vcpu)) {
>> + vcpu->preempted = true;
>> return;
>> + }
>>
>> me = get_cpu();
>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>>
>
> Who is resetting vcpu->preempted to false in this case? This also
> applies to s390 in fact.
Isnt that done by the sched_in handler?
On 18/07/19 10:15, Christian Borntraeger wrote:
>
>
> On 18.07.19 09:59, Paolo Bonzini wrote:
>> On 12/07/19 09:15, Wanpeng Li wrote:
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index b4ab59d..2c46705 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>>> int me;
>>> int cpu = vcpu->cpu;
>>>
>>> - if (kvm_vcpu_wake_up(vcpu))
>>> + if (kvm_vcpu_wake_up(vcpu)) {
>>> + vcpu->preempted = true;
>>> return;
>>> + }
>>>
>>> me = get_cpu();
>>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>>>
>>
>> Who is resetting vcpu->preempted to false in this case? This also
>> applies to s390 in fact.
>
> Isnt that done by the sched_in handler?
I am a bit confused because, if it is done by the sched_in later, I
don't understand why the sched_out handler hasn't set vcpu->preempted
already.
The s390 commit message is not very clear, but it talks about "a former
sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
that mean it is in kvm_vcpu_block? But then at least for x86 it would
be after vcpu_load so the preempt notifiers have been registered, and
for s390 too (kvm_arch_vcpu_ioctl_run -> __vcpu_run -> vcpu_post_run ->
kvm_handle_sie_intercept etc.).
Paolo
On Thu, 18 Jul 2019 at 16:34, Paolo Bonzini <[email protected]> wrote:
>
> On 18/07/19 10:15, Christian Borntraeger wrote:
> >
> >
> > On 18.07.19 09:59, Paolo Bonzini wrote:
> >> On 12/07/19 09:15, Wanpeng Li wrote:
> >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >>> index b4ab59d..2c46705 100644
> >>> --- a/virt/kvm/kvm_main.c
> >>> +++ b/virt/kvm/kvm_main.c
> >>> @@ -2404,8 +2404,10 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> >>> int me;
> >>> int cpu = vcpu->cpu;
> >>>
> >>> - if (kvm_vcpu_wake_up(vcpu))
> >>> + if (kvm_vcpu_wake_up(vcpu)) {
> >>> + vcpu->preempted = true;
> >>> return;
> >>> + }
> >>>
> >>> me = get_cpu();
> >>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
> >>>
> >>
> >> Who is resetting vcpu->preempted to false in this case? This also
> >> applies to s390 in fact.
> >
> > Isnt that done by the sched_in handler?
>
> I am a bit confused because, if it is done by the sched_in later, I
> don't understand why the sched_out handler hasn't set vcpu->preempted
> already.
>
> The s390 commit message is not very clear, but it talks about "a former
> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
> that mean it is in kvm_vcpu_block? But then at least for x86 it would
see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will
be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set
vcpu->preempted to true iff current->state == TASK_RUNNING.
Regards,
Wanpeng Li
On 18/07/19 10:43, Wanpeng Li wrote:
>>> Isnt that done by the sched_in handler?
>>
>> I am a bit confused because, if it is done by the sched_in later, I
>> don't understand why the sched_out handler hasn't set vcpu->preempted
>> already.
>>
>> The s390 commit message is not very clear, but it talks about "a former
>> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
>> that mean it is in kvm_vcpu_block? But then at least for x86 it would
>
> see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will
> be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set
> vcpu->preempted to true iff current->state == TASK_RUNNING.
Ok, I was totally blind to that "if" around vcpu->preempted = true, it's
obvious now.
I think we need two flags then, for example vcpu->preempted and vcpu->ready:
- kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING
- kvm_vcpu_kick sets vcpu->ready to true
- kvm_sched_in clears both of them
This way, vmx_vcpu_pi_load can keep looking at preempted only (it
handles voluntary preemption in pi_pre_block/pi_post_block).
Also, kvm_s390_vcpu_wakeup can be changed to use kvm_vcpu_wake_up, which
is nice.
Paolo
On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <[email protected]> wrote:
>
> On 18/07/19 10:43, Wanpeng Li wrote:
> >>> Isnt that done by the sched_in handler?
> >>
> >> I am a bit confused because, if it is done by the sched_in later, I
> >> don't understand why the sched_out handler hasn't set vcpu->preempted
> >> already.
> >>
> >> The s390 commit message is not very clear, but it talks about "a former
> >> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
> >> that mean it is in kvm_vcpu_block? But then at least for x86 it would
> >
> > see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will
> > be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set
> > vcpu->preempted to true iff current->state == TASK_RUNNING.
>
> Ok, I was totally blind to that "if" around vcpu->preempted = true, it's
> obvious now.
>
> I think we need two flags then, for example vcpu->preempted and vcpu->ready:
>
> - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING
>
> - kvm_vcpu_kick sets vcpu->ready to true
>
> - kvm_sched_in clears both of them
>
> This way, vmx_vcpu_pi_load can keep looking at preempted only (it
> handles voluntary preemption in pi_pre_block/pi_post_block).
>
> Also, kvm_s390_vcpu_wakeup can be changed to use kvm_vcpu_wake_up, which
> is nice.
Will do. :)
Wanpeng
On 18/07/19 11:29, Wanpeng Li wrote:
> On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <[email protected]> wrote:
>>
>> On 18/07/19 10:43, Wanpeng Li wrote:
>>>>> Isnt that done by the sched_in handler?
>>>>
>>>> I am a bit confused because, if it is done by the sched_in later, I
>>>> don't understand why the sched_out handler hasn't set vcpu->preempted
>>>> already.
>>>>
>>>> The s390 commit message is not very clear, but it talks about "a former
>>>> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
>>>> that mean it is in kvm_vcpu_block? But then at least for x86 it would
>>>
>>> see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will
>>> be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set
>>> vcpu->preempted to true iff current->state == TASK_RUNNING.
>>
>> Ok, I was totally blind to that "if" around vcpu->preempted = true, it's
>> obvious now.
>>
>> I think we need two flags then, for example vcpu->preempted and vcpu->ready:
>>
>> - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING
>>
>> - kvm_vcpu_kick sets vcpu->ready to true
>>
>> - kvm_sched_in clears both of them
... and also kvm_vcpu_on_spin should check vcpu->ready. vcpu->preempted
remains only for use by vmx_vcpu_pi_put.
Later we could think of removing vcpu->preempted. For example,
kvm_arch_sched_out and kvm_x86_ops->sched_out could get the code
currently in vmx_vcpu_pi_put (testing curent->state == TASK_RUNNING
instead of vcpu->preempted). But for now there's no need and I'm not
sure it's an improvement at all.
Paolo
>> This way, vmx_vcpu_pi_load can keep looking at preempted only (it
>> handles voluntary preemption in pi_pre_block/pi_post_block).
On Thu, 18 Jul 2019 at 17:39, Paolo Bonzini <[email protected]> wrote:
>
> On 18/07/19 11:29, Wanpeng Li wrote:
> > On Thu, 18 Jul 2019 at 17:07, Paolo Bonzini <[email protected]> wrote:
> >>
> >> On 18/07/19 10:43, Wanpeng Li wrote:
> >>>>> Isnt that done by the sched_in handler?
> >>>>
> >>>> I am a bit confused because, if it is done by the sched_in later, I
> >>>> don't understand why the sched_out handler hasn't set vcpu->preempted
> >>>> already.
> >>>>
> >>>> The s390 commit message is not very clear, but it talks about "a former
> >>>> sleeping cpu" that "gave up the cpu voluntarily". Does "voluntarily"
> >>>> that mean it is in kvm_vcpu_block? But then at least for x86 it would
> >>>
> >>> see the prepare_to_swait_exlusive() in kvm_vcpu_block(), the task will
> >>> be set in TASK_INTERRUPTIBLE state, kvm_sched_out will set
> >>> vcpu->preempted to true iff current->state == TASK_RUNNING.
> >>
> >> Ok, I was totally blind to that "if" around vcpu->preempted = true, it's
> >> obvious now.
> >>
> >> I think we need two flags then, for example vcpu->preempted and vcpu->ready:
> >>
> >> - kvm_sched_out sets both of them to true iff current->state == TASK_RUNNING
> >>
> >> - kvm_vcpu_kick sets vcpu->ready to true
> >>
> >> - kvm_sched_in clears both of them
>
> ... and also kvm_vcpu_on_spin should check vcpu->ready. vcpu->preempted
> remains only for use by vmx_vcpu_pi_put.
Done in v2, please have a look. :)
Regards,
Wanpeng Li