| Message ID | 20180819001746.21586-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] kvm: leverage change to adjust slots->used_slots in update_memslots() | expand |
On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: > update_memslots() is only called by __kvm_set_memory_region(), in which > change is calculated to indicate the following behavior. With this What is the 'following behavior' you mention? > information, it is not necessary to do the calculation again in > update_memslots(). > > By encoding the number of slot adjustment in the lowest byte of change, > the slots->used_slots adjustment is accomplished by one addition. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > include/linux/kvm_host.h | 11 +++++++---- > virt/kvm/kvm_main.c | 14 ++++---------- > 2 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 4ee7bc548a83..e0ae6cf82c0d 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -610,12 +610,15 @@ id_to_memslot(struct kvm_memslots *slots, int id) > * > * Since flags can be changed by some of these operations, the following > * differentiation is the best we can do for __kvm_set_memory_region(): > + * > + * Encode the number of slot adjustment in the lowest byte, which will > + * be used in update_memslots(). > */ > enum kvm_mr_change { > - KVM_MR_CREATE, > - KVM_MR_DELETE, > - KVM_MR_MOVE, > - KVM_MR_FLAGS_ONLY, > + KVM_MR_CREATE = 0x0001, > + KVM_MR_DELETE = 0x00ff, > + KVM_MR_MOVE = 0x0000, > + KVM_MR_FLAGS_ONLY = 0x0100, > }; > > int kvm_set_memory_region(struct kvm *kvm, > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3d233ebfbee9..5c1911790f25 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -801,21 +801,15 @@ static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) > * sorted array and known changed memslot position. > */ > static void update_memslots(struct kvm_memslots *slots, > - struct kvm_memory_slot *new) > + struct kvm_memory_slot *new, > + enum kvm_mr_change change) > { > int id = new->id; > int i = slots->id_to_index[id]; > struct kvm_memory_slot *mslots = slots->memslots; > > WARN_ON(mslots[i].id != id); > - if (!new->npages) { > - WARN_ON(!mslots[i].npages); > - if (mslots[i].npages) > - slots->used_slots--; > - } else { > - if (!mslots[i].npages) > - slots->used_slots++; > - } > + slots->used_slots += (char)change; > > while (i < KVM_MEM_SLOTS_NUM - 1 && > new->base_gfn <= mslots[i + 1].base_gfn) { > @@ -1050,7 +1044,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > memset(&new.arch, 0, sizeof(new.arch)); > } > > - update_memslots(slots, &new); > + update_memslots(slots, &new, change); > old_memslots = install_new_memslots(kvm, as_id, slots); > > kvm_arch_commit_memory_region(kvm, mem, &old, &new, change); > -- > 2.15.1 >
On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >> update_memslots() is only called by __kvm_set_memory_region(), in which >> change is calculated to indicate the following behavior. With this > >What is the 'following behavior' you mention? Ah, 'following behavior' means what need to do in update_memslots() to used_slots. For example, if change equals KVM_MR_CREATE, it means update_memslots() will add a new slot and used_slots need to increase by one. With this information, we don't need to do the calculation in update_memslots() again, but just do an addition. > >> information, it is not necessary to do the calculation again in >> update_memslots(). >> >> By encoding the number of slot adjustment in the lowest byte of change, >> the slots->used_slots adjustment is accomplished by one addition. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> --- >> include/linux/kvm_host.h | 11 +++++++---- >> virt/kvm/kvm_main.c | 14 ++++---------- >> 2 files changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 4ee7bc548a83..e0ae6cf82c0d 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -610,12 +610,15 @@ id_to_memslot(struct kvm_memslots *slots, int id) >> * >> * Since flags can be changed by some of these operations, the following >> * differentiation is the best we can do for __kvm_set_memory_region(): >> + * >> + * Encode the number of slot adjustment in the lowest byte, which will >> + * be used in update_memslots(). >> */ >> enum kvm_mr_change { >> - KVM_MR_CREATE, >> - KVM_MR_DELETE, >> - KVM_MR_MOVE, >> - KVM_MR_FLAGS_ONLY, >> + KVM_MR_CREATE = 0x0001, >> + KVM_MR_DELETE = 0x00ff, >> + KVM_MR_MOVE = 0x0000, >> + KVM_MR_FLAGS_ONLY = 0x0100, >> }; >> >> int kvm_set_memory_region(struct kvm *kvm, >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 3d233ebfbee9..5c1911790f25 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -801,21 +801,15 @@ static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) >> * sorted array and known changed memslot position. >> */ >> static void update_memslots(struct kvm_memslots *slots, >> - struct kvm_memory_slot *new) >> + struct kvm_memory_slot *new, >> + enum kvm_mr_change change) >> { >> int id = new->id; >> int i = slots->id_to_index[id]; >> struct kvm_memory_slot *mslots = slots->memslots; >> >> WARN_ON(mslots[i].id != id); >> - if (!new->npages) { >> - WARN_ON(!mslots[i].npages); >> - if (mslots[i].npages) >> - slots->used_slots--; >> - } else { >> - if (!mslots[i].npages) >> - slots->used_slots++; >> - } >> + slots->used_slots += (char)change; >> >> while (i < KVM_MEM_SLOTS_NUM - 1 && >> new->base_gfn <= mslots[i + 1].base_gfn) { >> @@ -1050,7 +1044,7 @@ int __kvm_set_memory_region(struct kvm *kvm, >> memset(&new.arch, 0, sizeof(new.arch)); >> } >> >> - update_memslots(slots, &new); >> + update_memslots(slots, &new, change); >> old_memslots = install_new_memslots(kvm, as_id, slots); >> >> kvm_arch_commit_memory_region(kvm, mem, &old, &new, change); >> -- >> 2.15.1 >>
On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: > On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: > >On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: > >> update_memslots() is only called by __kvm_set_memory_region(), in which > >> change is calculated to indicate the following behavior. With this > > > >What is the 'following behavior' you mention? > > Ah, 'following behavior' means what need to do in update_memslots() to > used_slots. > > For example, if change equals KVM_MR_CREATE, it means update_memslots() > will add a new slot and used_slots need to increase by one. > > With this information, we don't need to do the calculation in > update_memslots() again, but just do an addition. Could you update the commit ot have this description please? > > > > >> information, it is not necessary to do the calculation again in > >> update_memslots(). > >> > >> By encoding the number of slot adjustment in the lowest byte of change, > >> the slots->used_slots adjustment is accomplished by one addition. > >> > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >> --- > >> include/linux/kvm_host.h | 11 +++++++---- > >> virt/kvm/kvm_main.c | 14 ++++---------- > >> 2 files changed, 11 insertions(+), 14 deletions(-) > >> > >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > >> index 4ee7bc548a83..e0ae6cf82c0d 100644 > >> --- a/include/linux/kvm_host.h > >> +++ b/include/linux/kvm_host.h > >> @@ -610,12 +610,15 @@ id_to_memslot(struct kvm_memslots *slots, int id) > >> * > >> * Since flags can be changed by some of these operations, the following > >> * differentiation is the best we can do for __kvm_set_memory_region(): > >> + * > >> + * Encode the number of slot adjustment in the lowest byte, which will > >> + * be used in update_memslots(). > >> */ > >> enum kvm_mr_change { > >> - KVM_MR_CREATE, > >> - KVM_MR_DELETE, > >> - KVM_MR_MOVE, > >> - KVM_MR_FLAGS_ONLY, > >> + KVM_MR_CREATE = 0x0001, > >> + KVM_MR_DELETE = 0x00ff, > >> + KVM_MR_MOVE = 0x0000, > >> + KVM_MR_FLAGS_ONLY = 0x0100, > >> }; > >> > >> int kvm_set_memory_region(struct kvm *kvm, > >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > >> index 3d233ebfbee9..5c1911790f25 100644 > >> --- a/virt/kvm/kvm_main.c > >> +++ b/virt/kvm/kvm_main.c > >> @@ -801,21 +801,15 @@ static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) > >> * sorted array and known changed memslot position. > >> */ > >> static void update_memslots(struct kvm_memslots *slots, > >> - struct kvm_memory_slot *new) > >> + struct kvm_memory_slot *new, > >> + enum kvm_mr_change change) > >> { > >> int id = new->id; > >> int i = slots->id_to_index[id]; > >> struct kvm_memory_slot *mslots = slots->memslots; > >> > >> WARN_ON(mslots[i].id != id); > >> - if (!new->npages) { > >> - WARN_ON(!mslots[i].npages); > >> - if (mslots[i].npages) > >> - slots->used_slots--; > >> - } else { > >> - if (!mslots[i].npages) > >> - slots->used_slots++; > >> - } > >> + slots->used_slots += (char)change; > >> > >> while (i < KVM_MEM_SLOTS_NUM - 1 && > >> new->base_gfn <= mslots[i + 1].base_gfn) { > >> @@ -1050,7 +1044,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > >> memset(&new.arch, 0, sizeof(new.arch)); > >> } > >> > >> - update_memslots(slots, &new); > >> + update_memslots(slots, &new, change); > >> old_memslots = install_new_memslots(kvm, as_id, slots); > >> > >> kvm_arch_commit_memory_region(kvm, mem, &old, &new, change); > >> -- > >> 2.15.1 > >> > > -- > Wei Yang > Help you, Help me
On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote: >On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: >> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >> >On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >> >> update_memslots() is only called by __kvm_set_memory_region(), in which >> >> change is calculated to indicate the following behavior. With this >> > >> >What is the 'following behavior' you mention? >> >> Ah, 'following behavior' means what need to do in update_memslots() to >> used_slots. >> >> For example, if change equals KVM_MR_CREATE, it means update_memslots() >> will add a new slot and used_slots need to increase by one. >> >> With this information, we don't need to do the calculation in >> update_memslots() again, but just do an addition. > >Could you update the commit ot have this description please? >> Sure. Per comments from Paolo in patch 2, only this one is necessary in next spin, right?
On 21/08/2018 00:53, Wei Yang wrote: > On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote: >> On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: >>> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >>>> On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >>>>> update_memslots() is only called by __kvm_set_memory_region(), in which >>>>> change is calculated to indicate the following behavior. With this >>>> >>>> What is the 'following behavior' you mention? >>> >>> Ah, 'following behavior' means what need to do in update_memslots() to >>> used_slots. >>> >>> For example, if change equals KVM_MR_CREATE, it means update_memslots() >>> will add a new slot and used_slots need to increase by one. >>> >>> With this information, we don't need to do the calculation in >>> update_memslots() again, but just do an addition. >> >> Could you update the commit ot have this description please? >>> > > Sure. > > Per comments from Paolo in patch 2, only this one is necessary in next > spin, right? I don't know... Encoding the delta in bits 0-7 is not something I would really do unless it's super important for performance. Paolo
On Tue, Aug 21, 2018 at 12:31:31PM +0200, Paolo Bonzini wrote: >On 21/08/2018 00:53, Wei Yang wrote: >> On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote: >>> On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: >>>> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >>>>> On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >>>>>> update_memslots() is only called by __kvm_set_memory_region(), in which >>>>>> change is calculated to indicate the following behavior. With this >>>>> >>>>> What is the 'following behavior' you mention? >>>> >>>> Ah, 'following behavior' means what need to do in update_memslots() to >>>> used_slots. >>>> >>>> For example, if change equals KVM_MR_CREATE, it means update_memslots() >>>> will add a new slot and used_slots need to increase by one. >>>> >>>> With this information, we don't need to do the calculation in >>>> update_memslots() again, but just do an addition. >>> >>> Could you update the commit ot have this description please? >>>> >> >> Sure. >> >> Per comments from Paolo in patch 2, only this one is necessary in next >> spin, right? > >I don't know... Encoding the delta in bits 0-7 is not something I would >really do unless it's super important for performance. The original idea is just pass the change to update_memslots() and adjust used_slots based on change instead of re-calculate it. While I did a step further to encode the adjustment in change. How about something like this: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3d233ebfbee9..efecb0489dbc 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -808,13 +808,15 @@ static void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *mslots = slots->memslots; WARN_ON(mslots[i].id != id); - if (!new->npages) { - WARN_ON(!mslots[i].npages); - if (mslots[i].npages) - slots->used_slots--; - } else { - if (!mslots[i].npages) - slots->used_slots++; + switch (change) { + case KVM_MR_CREATE: + slots->used_slots++; + break; + case KVM_MR_DELETE: + slots->used_slots--; + break; + default: + break; } > >Paolo
On 21/08/2018 14:57, Wei Yang wrote: > On Tue, Aug 21, 2018 at 12:31:31PM +0200, Paolo Bonzini wrote: >> On 21/08/2018 00:53, Wei Yang wrote: >>> On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote: >>>> On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: >>>>> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >>>>>> On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >>>>>>> update_memslots() is only called by __kvm_set_memory_region(), in which >>>>>>> change is calculated to indicate the following behavior. With this >>>>>> >>>>>> What is the 'following behavior' you mention? >>>>> >>>>> Ah, 'following behavior' means what need to do in update_memslots() to >>>>> used_slots. >>>>> >>>>> For example, if change equals KVM_MR_CREATE, it means update_memslots() >>>>> will add a new slot and used_slots need to increase by one. >>>>> >>>>> With this information, we don't need to do the calculation in >>>>> update_memslots() again, but just do an addition. >>>> >>>> Could you update the commit ot have this description please? >>>>> >>> >>> Sure. >>> >>> Per comments from Paolo in patch 2, only this one is necessary in next >>> spin, right? >> >> I don't know... Encoding the delta in bits 0-7 is not something I would >> really do unless it's super important for performance. > > The original idea is just pass the change to update_memslots() and > adjust used_slots based on change instead of re-calculate it. Looks good, and perhaps: > How about something like this: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3d233ebfbee9..efecb0489dbc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -808,13 +808,15 @@ static void update_memslots(struct kvm_memslots *slots, > struct kvm_memory_slot *mslots = slots->memslots; > > WARN_ON(mslots[i].id != id); > - if (!new->npages) { > - WARN_ON(!mslots[i].npages); > - if (mslots[i].npages) > - slots->used_slots--; > - } else { > - if (!mslots[i].npages) > - slots->used_slots++; > + switch (change) { > + case KVM_MR_CREATE: > + slots->used_slots++; WARN_ON(mslots[i].npages || !new->npages); > + break; > + case KVM_MR_DELETE: > + slots->used_slots--; WARN_ON(new->npages || !mslots[i].npages); > + break; > + default: > + break; > } Ensuring the invariants is really more important here than being fast. Paolo
On Tue, Aug 21, 2018 at 03:52:40PM +0200, Paolo Bonzini wrote: >On 21/08/2018 14:57, Wei Yang wrote: >> On Tue, Aug 21, 2018 at 12:31:31PM +0200, Paolo Bonzini wrote: >>> On 21/08/2018 00:53, Wei Yang wrote: >>>> On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote: >>>>> On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote: >>>>>> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote: >>>>>>> On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote: >>>>>>>> update_memslots() is only called by __kvm_set_memory_region(), in which >>>>>>>> change is calculated to indicate the following behavior. With this >>>>>>> >>>>>>> What is the 'following behavior' you mention? >>>>>> >>>>>> Ah, 'following behavior' means what need to do in update_memslots() to >>>>>> used_slots. >>>>>> >>>>>> For example, if change equals KVM_MR_CREATE, it means update_memslots() >>>>>> will add a new slot and used_slots need to increase by one. >>>>>> >>>>>> With this information, we don't need to do the calculation in >>>>>> update_memslots() again, but just do an addition. >>>>> >>>>> Could you update the commit ot have this description please? >>>>>> >>>> >>>> Sure. >>>> >>>> Per comments from Paolo in patch 2, only this one is necessary in next >>>> spin, right? >>> >>> I don't know... Encoding the delta in bits 0-7 is not something I would >>> really do unless it's super important for performance. >> >> The original idea is just pass the change to update_memslots() and >> adjust used_slots based on change instead of re-calculate it. > >Looks good, and perhaps: > >> How about something like this: >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 3d233ebfbee9..efecb0489dbc 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -808,13 +808,15 @@ static void update_memslots(struct kvm_memslots *slots, >> struct kvm_memory_slot *mslots = slots->memslots; >> >> WARN_ON(mslots[i].id != id); >> - if (!new->npages) { >> - WARN_ON(!mslots[i].npages); >> - if (mslots[i].npages) >> - slots->used_slots--; >> - } else { >> - if (!mslots[i].npages) >> - slots->used_slots++; >> + switch (change) { >> + case KVM_MR_CREATE: >> + slots->used_slots++; > >WARN_ON(mslots[i].npages || !new->npages); > >> + break; >> + case KVM_MR_DELETE: >> + slots->used_slots--; > >WARN_ON(new->npages || !mslots[i].npages); > >> + break; >> + default: >> + break; >> } > >Ensuring the invariants is really more important here than being fast. Ok, get your point. I will spin a patch based on this format. BTW, I am willing to measure the impact of those change. Also I have another idea to improve the code. Do you have some case or method to measure this? > >Paolo
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 4ee7bc548a83..e0ae6cf82c0d 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -610,12 +610,15 @@ id_to_memslot(struct kvm_memslots *slots, int id) * * Since flags can be changed by some of these operations, the following * differentiation is the best we can do for __kvm_set_memory_region(): + * + * Encode the number of slot adjustment in the lowest byte, which will + * be used in update_memslots(). */ enum kvm_mr_change { - KVM_MR_CREATE, - KVM_MR_DELETE, - KVM_MR_MOVE, - KVM_MR_FLAGS_ONLY, + KVM_MR_CREATE = 0x0001, + KVM_MR_DELETE = 0x00ff, + KVM_MR_MOVE = 0x0000, + KVM_MR_FLAGS_ONLY = 0x0100, }; int kvm_set_memory_region(struct kvm *kvm, diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3d233ebfbee9..5c1911790f25 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -801,21 +801,15 @@ static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) * sorted array and known changed memslot position. */ static void update_memslots(struct kvm_memslots *slots, - struct kvm_memory_slot *new) + struct kvm_memory_slot *new, + enum kvm_mr_change change) { int id = new->id; int i = slots->id_to_index[id]; struct kvm_memory_slot *mslots = slots->memslots; WARN_ON(mslots[i].id != id); - if (!new->npages) { - WARN_ON(!mslots[i].npages); - if (mslots[i].npages) - slots->used_slots--; - } else { - if (!mslots[i].npages) - slots->used_slots++; - } + slots->used_slots += (char)change; while (i < KVM_MEM_SLOTS_NUM - 1 && new->base_gfn <= mslots[i + 1].base_gfn) { @@ -1050,7 +1044,7 @@ int __kvm_set_memory_region(struct kvm *kvm, memset(&new.arch, 0, sizeof(new.arch)); } - update_memslots(slots, &new); + update_memslots(slots, &new, change); old_memslots = install_new_memslots(kvm, as_id, slots); kvm_arch_commit_memory_region(kvm, mem, &old, &new, change);
update_memslots() is only called by __kvm_set_memory_region(), in which change is calculated to indicate the following behavior. With this information, it is not necessary to do the calculation again in update_memslots(). By encoding the number of slot adjustment in the lowest byte of change, the slots->used_slots adjustment is accomplished by one addition. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- include/linux/kvm_host.h | 11 +++++++---- virt/kvm/kvm_main.c | 14 ++++---------- 2 files changed, 11 insertions(+), 14 deletions(-)