| Message ID | 20201012125323.17509-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-mem: Big Block Mode (BBM) | expand |
On Mon, Oct 12, 2020 at 02:52:56PM +0200, David Hildenbrand wrote: >We actually need one byte less (next_mb_id is exclusive, first_mb_id is >inclusive). Simplify. > >Cc: "Michael S. Tsirkin" <mst@redhat.com> >Cc: Jason Wang <jasowang@redhat.com> >Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >Signed-off-by: David Hildenbrand <david@redhat.com> >--- > drivers/virtio/virtio_mem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >index a1f5bf7a571a..670b3faf412d 100644 >--- a/drivers/virtio/virtio_mem.c >+++ b/drivers/virtio/virtio_mem.c >@@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, > */ > static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) > { >- unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; >- unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; >+ unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; >+ unsigned long new_bytes = old_bytes + 1; This is correct. So this looks more like a fix? > int old_pages = PFN_UP(old_bytes); > int new_pages = PFN_UP(new_bytes); > uint8_t *new_mb_state; >-- >2.26.2
On 15.10.20 06:02, Wei Yang wrote: > On Mon, Oct 12, 2020 at 02:52:56PM +0200, David Hildenbrand wrote: >> We actually need one byte less (next_mb_id is exclusive, first_mb_id is >> inclusive). Simplify. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> drivers/virtio/virtio_mem.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >> index a1f5bf7a571a..670b3faf412d 100644 >> --- a/drivers/virtio/virtio_mem.c >> +++ b/drivers/virtio/virtio_mem.c >> @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, >> */ >> static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) >> { >> - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; >> - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; >> + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; >> + unsigned long new_bytes = old_bytes + 1; > > This is correct. > > So this looks more like a fix? We allocate an additional new page "one memory block too early". So we would allocate the first page for blocks 0..510, and already allocate the second page with block 511, although we could have fit it into the first page. Block 512 will then find that the second page is already there and simply use the second page. So as we do it consistently, nothing will go wrong - that's why I avoided using the "fix" terminology. Thanks!
On Thu, Oct 15, 2020 at 10:00:15AM +0200, David Hildenbrand wrote: >On 15.10.20 06:02, Wei Yang wrote: >> On Mon, Oct 12, 2020 at 02:52:56PM +0200, David Hildenbrand wrote: >>> We actually need one byte less (next_mb_id is exclusive, first_mb_id is >>> inclusive). Simplify. >>> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Jason Wang <jasowang@redhat.com> >>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> drivers/virtio/virtio_mem.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >>> index a1f5bf7a571a..670b3faf412d 100644 >>> --- a/drivers/virtio/virtio_mem.c >>> +++ b/drivers/virtio/virtio_mem.c >>> @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, >>> */ >>> static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) >>> { >>> - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; >>> - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; >>> + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; >>> + unsigned long new_bytes = old_bytes + 1; >> >> This is correct. >> >> So this looks more like a fix? > >We allocate an additional new page "one memory block too early". > >So we would allocate the first page for blocks 0..510, and already >allocate the second page with block 511, although we could have fit it >into the first page. Block 512 will then find that the second page is >already there and simply use the second page. > >So as we do it consistently, nothing will go wrong - that's why I >avoided using the "fix" terminology. > Yes, my feeling is this is not a simplification. Instead this is a more precise calculation. How about use this subject? virtio-mem: more precise calculation in virtio_mem_mb_state_prepare_next_mb() >Thanks! > >-- >Thanks, > >David / dhildenb
On 15.10.20 12:00, Wei Yang wrote: > On Thu, Oct 15, 2020 at 10:00:15AM +0200, David Hildenbrand wrote: >> On 15.10.20 06:02, Wei Yang wrote: >>> On Mon, Oct 12, 2020 at 02:52:56PM +0200, David Hildenbrand wrote: >>>> We actually need one byte less (next_mb_id is exclusive, first_mb_id is >>>> inclusive). Simplify. >>>> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>> Cc: Jason Wang <jasowang@redhat.com> >>>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> drivers/virtio/virtio_mem.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >>>> index a1f5bf7a571a..670b3faf412d 100644 >>>> --- a/drivers/virtio/virtio_mem.c >>>> +++ b/drivers/virtio/virtio_mem.c >>>> @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, >>>> */ >>>> static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) >>>> { >>>> - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; >>>> - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; >>>> + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; >>>> + unsigned long new_bytes = old_bytes + 1; >>> >>> This is correct. >>> >>> So this looks more like a fix? >> >> We allocate an additional new page "one memory block too early". >> >> So we would allocate the first page for blocks 0..510, and already >> allocate the second page with block 511, although we could have fit it >> into the first page. Block 512 will then find that the second page is >> already there and simply use the second page. >> >> So as we do it consistently, nothing will go wrong - that's why I >> avoided using the "fix" terminology. >> > > Yes, my feeling is this is not a simplification. Instead this is a more > precise calculation. > > How about use this subject? > > virtio-mem: more precise calculation in virtio_mem_mb_state_prepare_next_mb() Agreed, thanks!
> We actually need one byte less (next_mb_id is exclusive, first_mb_id is > inclusive). Simplify. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > drivers/virtio/virtio_mem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c > index a1f5bf7a571a..670b3faf412d 100644 > --- a/drivers/virtio/virtio_mem.c > +++ b/drivers/virtio/virtio_mem.c > @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, > */ > static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) > { > - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; > - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; > + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; > + unsigned long new_bytes = old_bytes + 1; Maybe we can avoid new_bytes & old_bytes variables, instead use single variable. Can later be used with PFN_UP/PFN_DOWN. > int old_pages = PFN_UP(old_bytes); > int new_pages = PFN_UP(new_bytes); > uint8_t *new_mb_state;
On 15.10.20 22:24, Pankaj Gupta wrote: >> We actually need one byte less (next_mb_id is exclusive, first_mb_id is >> inclusive). Simplify. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> drivers/virtio/virtio_mem.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >> index a1f5bf7a571a..670b3faf412d 100644 >> --- a/drivers/virtio/virtio_mem.c >> +++ b/drivers/virtio/virtio_mem.c >> @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, >> */ >> static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) >> { >> - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; >> - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; >> + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; >> + unsigned long new_bytes = old_bytes + 1; > > Maybe we can avoid new_bytes & old_bytes variables, instead use single > variable. Can later be used with PFN_UP/PFN_DOWN. I'll see if it fits into a single line now - if it does, I'll move it there. Thanks!
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c index a1f5bf7a571a..670b3faf412d 100644 --- a/drivers/virtio/virtio_mem.c +++ b/drivers/virtio/virtio_mem.c @@ -257,8 +257,8 @@ static enum virtio_mem_mb_state virtio_mem_mb_get_state(struct virtio_mem *vm, */ static int virtio_mem_mb_state_prepare_next_mb(struct virtio_mem *vm) { - unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id + 1; - unsigned long new_bytes = vm->next_mb_id - vm->first_mb_id + 2; + unsigned long old_bytes = vm->next_mb_id - vm->first_mb_id; + unsigned long new_bytes = old_bytes + 1; int old_pages = PFN_UP(old_bytes); int new_pages = PFN_UP(new_bytes); uint8_t *new_mb_state;
We actually need one byte less (next_mb_id is exclusive, first_mb_id is inclusive). Simplify. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- drivers/virtio/virtio_mem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)