Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
(2011/12/27 17:38), Liu Ping Fan wrote:
> From: Liu Ping Fan
>
> Currently, vcpu can be destructed only when kvm instance destroyed.
> Change this to vcpu's destruction before kvm instance, so vcpu MUST
> and CAN be destroyed before kvm's destroy.
I really don't understand why this big change can be justified by only
3 lines.
>
> Signed-off-by: Liu Ping Fan
> ---
> arch/x86/kvm/i8254.c | 10 +++--
> arch/x86/kvm/i8259.c | 17 +--
> arch/x86/kvm/x86.c | 53 +++---
> include/linux/kvm_host.h | 20 +++-
> virt/kvm/irq_comm.c |6 ++-
> virt/kvm/kvm_main.c | 110
> +++---
> 6 files changed, 140 insertions(+), 76 deletions(-)
>
You are introducing kvm_arch_vcpu_zap().
Then, apart from the "zap" naming issue I mentioned last time,
what about other architectures than x86?
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 900c763..b88d418d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -117,6 +117,7 @@ enum {
>
> struct kvm_vcpu {
> struct kvm *kvm;
> + struct list_head list;
> #ifdef CONFIG_PREEMPT_NOTIFIERS
> struct preempt_notifier preempt_notifier;
> #endif
> @@ -251,12 +252,14 @@ struct kvm {
> struct mm_struct *mm; /* userspace tied to this vm */
> struct kvm_memslots *memslots;
> struct srcu_struct srcu;
> + struct srcu_struct srcu_vcpus;
> +
Another srcu. This alone is worth explaining in the changelog IMO.
Takuya
> #ifdef CONFIG_KVM_APIC_ARCHITECTURE
> u32 bsp_vcpu_id;
> #endif
> - struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> + struct list_head vcpus;
> atomic_t online_vcpus;
> - int last_boosted_vcpu;
> + int last_boosted_vcpu_id;
> struct list_head vm_list;
> struct mutex lock;
> struct kvm_io_bus *buses[KVM_NR_BUSES];
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
2011/12/27 Takuya Yoshikawa :
> (2011/12/27 17:38), Liu Ping Fan wrote:
>> From: Liu Ping Fan
>>
>> Currently, vcpu can be destructed only when kvm instance destroyed.
>> Change this to vcpu's destruction before kvm instance, so vcpu MUST
>> and CAN be destroyed before kvm's destroy.
>
> I really don't understand why this big change can be justified by only
> 3 lines.
>
I think just recording what this patch does, not the whole story about
it. Right?
>>
>> Signed-off-by: Liu Ping Fan
>> ---
>> arch/x86/kvm/i8254.c | 10 +++--
>> arch/x86/kvm/i8259.c | 17 +--
>> arch/x86/kvm/x86.c | 53 +++---
>> include/linux/kvm_host.h | 20 +++-
>> virt/kvm/irq_comm.c | 6 ++-
>> virt/kvm/kvm_main.c | 110
>> +++---
>> 6 files changed, 140 insertions(+), 76 deletions(-)
>>
>
> You are introducing kvm_arch_vcpu_zap().
>
> Then, apart from the "zap" naming issue I mentioned last time,
Yes, I will correct "zap", as you said, its meaning is quite different
from destroy. :-)
> what about other architectures than x86?
>
Have not considered it in detail yet. At first step, I just want to
figure out the whole frame, then, I will push them on other arch.
Maybe you foresee some problem when extending this onto other arch,
please tell me, thanks ：-).
>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 900c763..b88d418d 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -117,6 +117,7 @@ enum {
>>
>> struct kvm_vcpu {
>> struct kvm *kvm;
>> + struct list_head list;
>> #ifdef CONFIG_PREEMPT_NOTIFIERS
>> struct preempt_notifier preempt_notifier;
>> #endif
>> @@ -251,12 +252,14 @@ struct kvm {
>> struct mm_struct *mm; /* userspace tied to this vm */
>> struct kvm_memslots *memslots;
>> struct srcu_struct srcu;
>> + struct srcu_struct srcu_vcpus;
>> +
>
> Another srcu. This alone is worth explaining in the changelog IMO.
>
Sorry, but why? I think it is just a srcu, and because it has
different aim and want a independent grace period, so not multiplex
kvm->srcu.
thanks and regards,
ping fan
> Takuya
>
>> #ifdef CONFIG_KVM_APIC_ARCHITECTURE
>> u32 bsp_vcpu_id;
>> #endif
>> - struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
>> + struct list_head vcpus;
>> atomic_t online_vcpus;
>> - int last_boosted_vcpu;
>> + int last_boosted_vcpu_id;
>> struct list_head vm_list;
>> struct mutex lock;
>> struct kvm_io_bus *buses[KVM_NR_BUSES];
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On 12/28/2011 08:54 AM, Liu ping fan wrote:
> >>
> >> struct kvm_vcpu {
> >> struct kvm *kvm;
> >> + struct list_head list;
> >> #ifdef CONFIG_PREEMPT_NOTIFIERS
> >> struct preempt_notifier preempt_notifier;
> >> #endif
> >> @@ -251,12 +252,14 @@ struct kvm {
> >> struct mm_struct *mm; /* userspace tied to this vm */
> >> struct kvm_memslots *memslots;
> >> struct srcu_struct srcu;
> >> + struct srcu_struct srcu_vcpus;
> >> +
> >
> > Another srcu. This alone is worth explaining in the changelog IMO.
> >
> Sorry, but why? I think it is just a srcu, and because it has
> different aim and want a independent grace period, so not multiplex
> kvm->srcu.
There is Documentation/virtual/kvm/locking.txt for that.
btw, why does it have to be srcu? Is rcu insufficient?
Why do we want an independent grace period, is hotunplugging a vcpu that
much different from hotunplugging memory?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On 12/27/2011 10:38 AM, Liu Ping Fan wrote: > From: Liu Ping Fan > > Currently, vcpu can be destructed only when kvm instance destroyed. > Change this to vcpu's destruction before kvm instance, so vcpu MUST > and CAN be destroyed before kvm's destroy. > > Signed-off-by: Liu Ping Fan > --- > arch/x86/kvm/i8254.c | 10 +++-- > arch/x86/kvm/i8259.c | 17 +-- > arch/x86/kvm/x86.c | 53 +++--- > include/linux/kvm_host.h | 20 +++- > virt/kvm/irq_comm.c |6 ++- > virt/kvm/kvm_main.c | 110 > +++--- > Documentation/virtual/kvm/api.txt -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On 12/28/2011 11:53 AM, Avi Kivity wrote: > On 12/27/2011 10:38 AM, Liu Ping Fan wrote: > > From: Liu Ping Fan > > > > Currently, vcpu can be destructed only when kvm instance destroyed. > > Change this to vcpu's destruction before kvm instance, so vcpu MUST > > and CAN be destroyed before kvm's destroy. > > > > Signed-off-by: Liu Ping Fan > > --- > > arch/x86/kvm/i8254.c | 10 +++-- > > arch/x86/kvm/i8259.c | 17 +-- > > arch/x86/kvm/x86.c | 53 +++--- > > include/linux/kvm_host.h | 20 +++- > > virt/kvm/irq_comm.c |6 ++- > > virt/kvm/kvm_main.c | 110 > > +++--- > > > > Documentation/virtual/kvm/api.txt > Oops, that's only needed when the unplug API is introduced. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
(2011/12/28 18:54), Avi Kivity wrote: On 12/28/2011 11:53 AM, Avi Kivity wrote: On 12/27/2011 10:38 AM, Liu Ping Fan wrote: From: Liu Ping Fan Currently, vcpu can be destructed only when kvm instance destroyed. Change this to vcpu's destruction before kvm instance, so vcpu MUST and CAN be destroyed before kvm's destroy. Signed-off-by: Liu Ping Fan --- arch/x86/kvm/i8254.c | 10 +++-- arch/x86/kvm/i8259.c | 17 +-- arch/x86/kvm/x86.c | 53 +++--- include/linux/kvm_host.h | 20 +++- virt/kvm/irq_comm.c |6 ++- virt/kvm/kvm_main.c | 110 +++--- Documentation/virtual/kvm/api.txt Oops, that's only needed when the unplug API is introduced. I think it is OK to to add such an API later on, but I really want the author to write the plan in the changelog. Otherwise people not belonging to Red Hat or IBM cannot know what this commit is aiming at. I am not objecting to this patch itself, but the way this kind of change is being introduced seems not be in a good manner. Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
(2011/12/28 15:54), Liu ping fan wrote:
You are introducing kvm_arch_vcpu_zap().
Then, apart from the "zap" naming issue I mentioned last time,
Yes, I will correct "zap", as you said, its meaning is quite different
from destroy. :-)
what about other architectures than x86?
Have not considered it in detail yet. At first step, I just want to
figure out the whole frame, then, I will push them on other arch.
Maybe you foresee some problem when extending this onto other arch,
please tell me, thanks ：-).
I do not mind if those are supported or not now.
But if not, you should write what is currently supported and what should
be done in the future.
Of course, you should at least make sure that other architectures will
not be broken by your patch.
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 900c763..b88d418d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -117,6 +117,7 @@ enum {
struct kvm_vcpu {
struct kvm *kvm;
+ struct list_head list;
#ifdef CONFIG_PREEMPT_NOTIFIERS
struct preempt_notifier preempt_notifier;
#endif
@@ -251,12 +252,14 @@ struct kvm {
struct mm_struct *mm; /* userspace tied to this vm */
struct kvm_memslots *memslots;
struct srcu_struct srcu;
+ struct srcu_struct srcu_vcpus;
+
Another srcu. This alone is worth explaining in the changelog IMO.
Sorry, but why? I think it is just a srcu, and because it has
different aim and want a independent grace period, so not multiplex
kvm->srcu.
I cannot say what MUST be explained in the changelog.
But many well known maintainers are telling the importance of changelog.
It is up to you, and KVM maintainers.
Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On 12/28/2011 12:19 PM, Takuya Yoshikawa wrote: >> Oops, that's only needed when the unplug API is introduced. >> > > > I think it is OK to to add such an API later on, but I really want > the author to write the plan in the changelog. It was in fact in the beginning of the thread. > I am not objecting to this patch itself, but the way this kind of change > is being introduced seems not be in a good manner. It should be part of a patch series that adds the API, otherwise it will never be tested. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On 12/29/2011 04:03 PM, Liu ping fan wrote: > > Why do we want an independent grace period, is hotunplugging a vcpu that > > much different from hotunplugging memory? > > > I thought that if less readers on the same srcu lock, then > synchronize_srcu_expedited() may success to return more quickly. It would be good to measure it, otherwise it's premature optimization. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On Wed, Dec 28, 2011 at 5:53 PM, Avi Kivity wrote:
> On 12/28/2011 08:54 AM, Liu ping fan wrote:
>> >>
>> >> struct kvm_vcpu {
>> >> struct kvm *kvm;
>> >> + struct list_head list;
>> >> #ifdef CONFIG_PREEMPT_NOTIFIERS
>> >> struct preempt_notifier preempt_notifier;
>> >> #endif
>> >> @@ -251,12 +252,14 @@ struct kvm {
>> >> struct mm_struct *mm; /* userspace tied to this vm */
>> >> struct kvm_memslots *memslots;
>> >> struct srcu_struct srcu;
>> >> + struct srcu_struct srcu_vcpus;
>> >> +
>> >
>> > Another srcu. This alone is worth explaining in the changelog IMO.
>> >
>> Sorry, but why? I think it is just a srcu, and because it has
>> different aim and want a independent grace period, so not multiplex
>> kvm->srcu.
>
> There is Documentation/virtual/kvm/locking.txt for that.
>
> btw, why does it have to be srcu? Is rcu insufficient?
>
Just to survive from "if (yield_to(task, 1)) in kvm_vcpu_on_spin()",
> Why do we want an independent grace period, is hotunplugging a vcpu that
> much different from hotunplugging memory?
>
I thought that if less readers on the same srcu lock, then
synchronize_srcu_expedited() may success to return more quickly.
Thanks and regards,
ping fan
> --
> error compiling committee.c: too many arguments to function
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] kvm: make vcpu life cycle separated from kvm instance
On Thu, Dec 29, 2011 at 10:31 PM, Avi Kivity wrote: > On 12/29/2011 04:03 PM, Liu ping fan wrote: >> > Why do we want an independent grace period, is hotunplugging a vcpu that >> > much different from hotunplugging memory? >> > >> I thought that if less readers on the same srcu lock, then >> synchronize_srcu_expedited() may success to return more quickly. > > It would be good to measure it, otherwise it's premature optimization. > Yes, after using kprobetrace to measure it, I found it was premature optimization. So I will resort to the kvm->srcu, instead of creating a new one in next version. Thanks and regards ping fan > -- > error compiling committee.c: too many arguments to function > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
