We actually need one byte less (next_mb_id is exclusive, first_mb_id is
inclusive). Simplify.
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Cc: Pankaj Gupta <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
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;
int old_pages = PFN_UP(old_bytes);
int new_pages = PFN_UP(new_bytes);
uint8_t *new_mb_state;
--
2.26.2
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" <[email protected]>
>Cc: Jason Wang <[email protected]>
>Cc: Pankaj Gupta <[email protected]>
>Signed-off-by: David Hildenbrand <[email protected]>
>---
> 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
--
Wei Yang
Help you, Help me
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" <[email protected]>
>> Cc: Jason Wang <[email protected]>
>> Cc: Pankaj Gupta <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>> ---
>> 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!
--
Thanks,
David / dhildenb
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" <[email protected]>
>>> Cc: Jason Wang <[email protected]>
>>> Cc: Pankaj Gupta <[email protected]>
>>> Signed-off-by: David Hildenbrand <[email protected]>
>>> ---
>>> 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
--
Wei Yang
Help you, Help me
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" <[email protected]>
>>>> Cc: Jason Wang <[email protected]>
>>>> Cc: Pankaj Gupta <[email protected]>
>>>> Signed-off-by: David Hildenbrand <[email protected]>
>>>> ---
>>>> 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!
--
Thanks,
David / dhildenb
> We actually need one byte less (next_mb_id is exclusive, first_mb_id is
> inclusive). Simplify.
>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Cc: Jason Wang <[email protected]>
> Cc: Pankaj Gupta <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> 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" <[email protected]>
>> Cc: Jason Wang <[email protected]>
>> Cc: Pankaj Gupta <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>> ---
>> 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!
--
Thanks,
David / dhildenb