Patchwork [07/10] ram_save_remaining() returns an uint64_t

login
register
mail settings
Submitter Juan Quintela
Date Nov. 23, 2010, 11:03 p.m.
Message ID <24346c6699fced39ed3725938091984ed23f48e5.1290552026.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/72770/
State New
Headers show

Comments

Juan Quintela - Nov. 23, 2010, 11:03 p.m.
From: Juan Quintela <quintela@trasno.org>

It returns a counter of things, not a ram address.

Signed-off-by: Juan Quintela <quintela@trasno.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Anthony Liguori - Nov. 30, 2010, 2:06 a.m.
On 11/23/2010 05:03 PM, Juan Quintela wrote:
> From: Juan Quintela<quintela@trasno.org>
>
> It returns a counter of things, not a ram address.
>
> Signed-off-by: Juan Quintela<quintela@trasno.org>
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   arch_init.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index df3d91f..9e941a0 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -173,10 +173,10 @@ static int ram_save_block(QEMUFile *f)
>
>   static uint64_t bytes_transferred;
>
> -static ram_addr_t ram_save_remaining(void)
> +static uint64_t ram_save_remaining(void)
>   {
>       RAMBlock *block;
> -    ram_addr_t count = 0;
> +    uint64_t count = 0;
>
>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>           ram_addr_t addr;
>    

No, it returns a count of bytes of ram which is a subset of ram_addr_t's 
space.  The unit is definitely right here.

Regards,

Anthony Liguori
Paolo Bonzini - Nov. 30, 2010, 7:21 a.m.
On 11/30/2010 03:06 AM, Anthony Liguori wrote:
>>
>> -static ram_addr_t ram_save_remaining(void)
>> +static uint64_t ram_save_remaining(void)
>>   {
>>       RAMBlock *block;
>> -    ram_addr_t count = 0;
>> +    uint64_t count = 0;
>>
>>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>>           ram_addr_t addr;
>
> No, it returns a count of bytes of ram which is a subset of ram_addr_t's
> space.  The unit is definitely right here.

It returns a count of pages, actually, which is a different unit.  For 
example, in practice it can fit in 32 bits even (though I'm not saying 
we should make it uint32_t; we'd be dangerously close to the limit for 
the guest sizes that Juan is handling).

Paolo
Anthony Liguori - Nov. 30, 2010, 1:44 p.m.
On 11/30/2010 01:21 AM, Paolo Bonzini wrote:
> On 11/30/2010 03:06 AM, Anthony Liguori wrote:
>>>
>>> -static ram_addr_t ram_save_remaining(void)
>>> +static uint64_t ram_save_remaining(void)
>>>   {
>>>       RAMBlock *block;
>>> -    ram_addr_t count = 0;
>>> +    uint64_t count = 0;
>>>
>>>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>>>           ram_addr_t addr;
>>
>> No, it returns a count of bytes of ram which is a subset of ram_addr_t's
>> space.  The unit is definitely right here.
>
> It returns a count of pages, actually, which is a different unit.

Page size is just a fixed number in the ram_addr_t space.

Regards,

Anthony Liguori

>   For example, in practice it can fit in 32 bits even (though I'm not 
> saying we should make it uint32_t; we'd be dangerously close to the 
> limit for the guest sizes that Juan is handling).
>
> Paolo
Juan Quintela - Nov. 30, 2010, 2:38 p.m.
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 11/23/2010 05:03 PM, Juan Quintela wrote:
>> From: Juan Quintela<quintela@trasno.org>
>>
>> It returns a counter of things, not a ram address.
>>
>> Signed-off-by: Juan Quintela<quintela@trasno.org>
>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>> ---
>>   arch_init.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index df3d91f..9e941a0 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -173,10 +173,10 @@ static int ram_save_block(QEMUFile *f)
>>
>>   static uint64_t bytes_transferred;
>>
>> -static ram_addr_t ram_save_remaining(void)
>> +static uint64_t ram_save_remaining(void)
>>   {
>>       RAMBlock *block;
>> -    ram_addr_t count = 0;
>> +    uint64_t count = 0;
>>
>>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>>           ram_addr_t addr;
>>    
>
> No, it returns a count of bytes of ram which is a subset of
> ram_addr_t's space.  The unit is definitely right here.

I thought this would be un-controversial.  But the important part from
your sentence is "count".  ram_addr_t is an addr in my mind.

  /* address in the RAM (different from a physical address) */
  typedef unsigned long ram_addr_t;

in cpu-common.h comment, it is also an address.  But doing more grepping
I see that it is "conveniently" used as a size in other places.

Later, Juan.

> Regards,
>
> Anthony Liguori

Patch

diff --git a/arch_init.c b/arch_init.c
index df3d91f..9e941a0 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -173,10 +173,10 @@  static int ram_save_block(QEMUFile *f)

 static uint64_t bytes_transferred;

-static ram_addr_t ram_save_remaining(void)
+static uint64_t ram_save_remaining(void)
 {
     RAMBlock *block;
-    ram_addr_t count = 0;
+    uint64_t count = 0;

     QLIST_FOREACH(block, &ram_list.blocks, next) {
         ram_addr_t addr;