2012-10-03 14:27:05

by Raghavendra K T

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

* Avi Kivity <[email protected]> [2012-09-30 13:13:09]:

> On 09/30/2012 01:07 PM, Gleb Natapov wrote:
> > On Sun, Sep 30, 2012 at 10:18:17AM +0200, Avi Kivity wrote:
> >> On 09/28/2012 08:16 AM, Raghavendra K T wrote:
> >> >
> >> >>
> >> >> +struct pv_sched_info {
> >> >> + unsigned long sched_bitmap;
> >> >
> >> > Thinking, whether we need something similar to cpumask here?
> >> > Only thing is we are representing guest (v)cpumask.
> >> >
> >>
> >> DECLARE_BITMAP(sched_bitmap, KVM_MAX_VCPUS)
> >>
> > vcpu_id can be greater than KVM_MAX_VCPUS.
>
> Use the index into the vcpu table as the bitmap index then. In fact
> it's better because then the lookup to get the vcpu pointer is trivial.

Did you mean, while setting the bitmap,

we should do
for (i = 1..n)
if (kvm->vcpus[i] == vcpu) set ith position in bitmap?

I just wanted to know whether there is any easy way to convert from
vcpu pointer to index in kvm vcpu table.


2012-10-03 14:57:47

by Avi Kivity

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

On 10/03/2012 04:17 PM, Raghavendra K T wrote:
> * Avi Kivity <[email protected]> [2012-09-30 13:13:09]:
>
>> On 09/30/2012 01:07 PM, Gleb Natapov wrote:
>> > On Sun, Sep 30, 2012 at 10:18:17AM +0200, Avi Kivity wrote:
>> >> On 09/28/2012 08:16 AM, Raghavendra K T wrote:
>> >> >
>> >> >>
>> >> >> +struct pv_sched_info {
>> >> >> + unsigned long sched_bitmap;
>> >> >
>> >> > Thinking, whether we need something similar to cpumask here?
>> >> > Only thing is we are representing guest (v)cpumask.
>> >> >
>> >>
>> >> DECLARE_BITMAP(sched_bitmap, KVM_MAX_VCPUS)
>> >>
>> > vcpu_id can be greater than KVM_MAX_VCPUS.
>>
>> Use the index into the vcpu table as the bitmap index then. In fact
>> it's better because then the lookup to get the vcpu pointer is trivial.
>
> Did you mean, while setting the bitmap,
>
> we should do
> for (i = 1..n)
> if (kvm->vcpus[i] == vcpu) set ith position in bitmap?

You can store i in the vcpu itself:

set_bit(vcpu->index, &kvm->preempted);

>
> I just wanted to know whether there is any easy way to convert from
> vcpu pointer to index in kvm vcpu table.
>



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

2012-10-04 07:29:15

by Gleb Natapov

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

On Wed, Oct 03, 2012 at 04:56:57PM +0200, Avi Kivity wrote:
> On 10/03/2012 04:17 PM, Raghavendra K T wrote:
> > * Avi Kivity <[email protected]> [2012-09-30 13:13:09]:
> >
> >> On 09/30/2012 01:07 PM, Gleb Natapov wrote:
> >> > On Sun, Sep 30, 2012 at 10:18:17AM +0200, Avi Kivity wrote:
> >> >> On 09/28/2012 08:16 AM, Raghavendra K T wrote:
> >> >> >
> >> >> >>
> >> >> >> +struct pv_sched_info {
> >> >> >> + unsigned long sched_bitmap;
> >> >> >
> >> >> > Thinking, whether we need something similar to cpumask here?
> >> >> > Only thing is we are representing guest (v)cpumask.
> >> >> >
> >> >>
> >> >> DECLARE_BITMAP(sched_bitmap, KVM_MAX_VCPUS)
> >> >>
> >> > vcpu_id can be greater than KVM_MAX_VCPUS.
> >>
> >> Use the index into the vcpu table as the bitmap index then. In fact
> >> it's better because then the lookup to get the vcpu pointer is trivial.
> >
> > Did you mean, while setting the bitmap,
> >
> > we should do
> > for (i = 1..n)
> > if (kvm->vcpus[i] == vcpu) set ith position in bitmap?
>
> You can store i in the vcpu itself:
>
> set_bit(vcpu->index, &kvm->preempted);
>
This will make the fact that vcpus are stored in an array into API
instead of implementation detail :( There were patches for vcpu
destruction that changed the array to rculist. Well, it will be still
possible to make the array rcu protected and copy it every time vcpu is
deleted/added I guess.

--
Gleb.

2012-10-05 08:40:20

by Raghavendra K T

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

On 10/04/2012 12:59 PM, Gleb Natapov wrote:
> On Wed, Oct 03, 2012 at 04:56:57PM +0200, Avi Kivity wrote:
>> On 10/03/2012 04:17 PM, Raghavendra K T wrote:
>>> * Avi Kivity <[email protected]> [2012-09-30 13:13:09]:
>>>
>>>> On 09/30/2012 01:07 PM, Gleb Natapov wrote:
>>>>> On Sun, Sep 30, 2012 at 10:18:17AM +0200, Avi Kivity wrote:
>>>>>> On 09/28/2012 08:16 AM, Raghavendra K T wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> +struct pv_sched_info {
>>>>>>>> + unsigned long sched_bitmap;
>>>>>>>
>>>>>>> Thinking, whether we need something similar to cpumask here?
>>>>>>> Only thing is we are representing guest (v)cpumask.
>>>>>>>
>>>>>>
>>>>>> DECLARE_BITMAP(sched_bitmap, KVM_MAX_VCPUS)
>>>>>>
>>>>> vcpu_id can be greater than KVM_MAX_VCPUS.
>>>>
>>>> Use the index into the vcpu table as the bitmap index then. In fact
>>>> it's better because then the lookup to get the vcpu pointer is trivial.
>>>
>>> Did you mean, while setting the bitmap,
>>>
>>> we should do
>>> for (i = 1..n)
>>> if (kvm->vcpus[i] == vcpu) set ith position in bitmap?
>>
>> You can store i in the vcpu itself:
>>
>> set_bit(vcpu->index, &kvm->preempted);
>>
> This will make the fact that vcpus are stored in an array into API
> instead of implementation detail :( There were patches for vcpu
> destruction that changed the array to rculist. Well, it will be still
> possible to make the array rcu protected and copy it every time vcpu is
> deleted/added I guess.
>

If IUC, summary is, we are going with
- Let vcpu array be rcu protected.
- we use index inside vcpu and should be updated when a vcpu is
added/deleted.

2012-10-07 09:52:07

by Avi Kivity

[permalink] [raw]
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

On 10/05/2012 10:36 AM, Raghavendra K T wrote:
>>>
>>> You can store i in the vcpu itself:
>>>
>>> set_bit(vcpu->index, &kvm->preempted);
>>>
>> This will make the fact that vcpus are stored in an array into API
>> instead of implementation detail :( There were patches for vcpu
>> destruction that changed the array to rculist. Well, it will be still
>> possible to make the array rcu protected and copy it every time vcpu is
>> deleted/added I guess.
>>
>
> If IUC, summary is, we are going with
> - Let vcpu array be rcu protected.

That's for the future. For now ->vcpus[] is statically allocated.

> - we use index inside vcpu and should be updated when a vcpu is
> added/deleted.

Yes.


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