Hi

While I am trying to put order in the atomic counters, I made in this series:

- convince and review code to see that everything is uint64_t.

- f->shutdown is not needed.  When we shutdown the file we put an
  error there if there is none.  So remove it.
- Make more clear how we use rate_limit.

Please review.

It is based on my previous series to the list:
Subject: [PATCH 0/2] More migration stats
Based-on: Message-Id: <20230504103357.22130-1-quintela@redhat.com>

Juan Quintela (9):
  migration: max_postcopy_bandwidth is a size parameter
  migration: qemu_file_total_transferred() function is monotonic
  qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t
  qemu-file: Make rate_limit_used an uint64_t
  qemu-file: No need to check for shutdown in qemu_file_rate_limit
  qemu-file: remove shutdown member
  qemu-file: Make total_transferred an uint64_t
  qemu-file: Make ram_control_save_page() use accessors for rate_limit
  qemu-file: Account for rate_limit usage on qemu_fflush()

 migration/block.c     | 13 +++----------
 migration/migration.c | 10 +++++-----
 migration/options.c   |  2 +-
 migration/options.h   |  2 +-
 migration/qemu-file.c | 42 +++++++++++++++++-------------------------
 migration/qemu-file.h | 10 +++++-----
 migration/savevm.c    |  6 ++----
 migration/vmstate.c   |  2 +-
 8 files changed, 35 insertions(+), 52 deletions(-)

-- 
2.40.0

On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
> - convince and review code to see that everything is uint64_t.

One general question to patches regarding this - what's the major benefit
of using uint64_t?

It doubles the possible numbers to hold, but it's already 64bits so I don't
think it matters a lot.  The thing is we're removing some code trying to
detect negative which seems to be still helpful to detect e.g. overflows
(even though I don't think it'll happen).  I just still think it's good to
know when overflow happens, and not sure what I missed on benefits of using
unsigned here.

I've reviewed all the rest patches and all look good here.

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
>> - convince and review code to see that everything is uint64_t.
>
> One general question to patches regarding this - what's the major benefit
> of using uint64_t?
>
> It doubles the possible numbers to hold, but it's already 64bits so I don't
> think it matters a lot.

We were checking for negatives even when that can't be.
And we are doing this dance of

int64_t x, y;
uint64_t a, b;

x = a;
b = y;

This is always confusing and not always right.

> The thing is we're removing some code trying to
> detect negative which seems to be still helpful to detect e.g. overflows
> (even though I don't think it'll happen).  I just still think it's good to
> know when overflow happens, and not sure what I missed on benefits of using
> unsigned here.

If you grep through the code, you see that half of the things are
int64_t and the other half is uint64_t.  I find it always confusing.


> I've reviewed all the rest patches and all look good here.

Thanks very much.

On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
> >> - convince and review code to see that everything is uint64_t.
> >
> > One general question to patches regarding this - what's the major benefit
> > of using uint64_t?
> >
> > It doubles the possible numbers to hold, but it's already 64bits so I don't
> > think it matters a lot.
> 
> We were checking for negatives even when that can't be.
> And we are doing this dance of
> 
> int64_t x, y;
> uint64_t a, b;
> 
> x = a;
> b = y;
> 
> This is always confusing and not always right.

Yeah this is confusing, but if anything can go wrong with this I assume we
could have some bigger problem anyway..

> 
> > The thing is we're removing some code trying to
> > detect negative which seems to be still helpful to detect e.g. overflows
> > (even though I don't think it'll happen).  I just still think it's good to
> > know when overflow happens, and not sure what I missed on benefits of using
> > unsigned here.
> 
> If you grep through the code, you see that half of the things are
> int64_t and the other half is uint64_t.  I find it always confusing.

Right, I'm personally curious whether we should just use int64_t always
unless necessary. :) Another good thing with int64_t is it's also suitable
for error report when used in retvals.

But no strong opinion here, I don't think that's a huge deal for now.
Having such an alignment on types makes sense to me.

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
>> >> - convince and review code to see that everything is uint64_t.
>> >
>> > One general question to patches regarding this - what's the major benefit
>> > of using uint64_t?
>> >
>> > It doubles the possible numbers to hold, but it's already 64bits so I don't
>> > think it matters a lot.
>> 
>> We were checking for negatives even when that can't be.
>> And we are doing this dance of
>> 
>> int64_t x, y;
>> uint64_t a, b;
>> 
>> x = a;
>> b = y;
>> 
>> This is always confusing and not always right.
>
> Yeah this is confusing, but if anything can go wrong with this I assume we
> could have some bigger problem anyway..
>
>> 
>> > The thing is we're removing some code trying to
>> > detect negative which seems to be still helpful to detect e.g. overflows
>> > (even though I don't think it'll happen).  I just still think it's good to
>> > know when overflow happens, and not sure what I missed on benefits of using
>> > unsigned here.
>> 
>> If you grep through the code, you see that half of the things are
>> int64_t and the other half is uint64_t.  I find it always confusing.
>
> Right, I'm personally curious whether we should just use int64_t always
> unless necessary. :) Another good thing with int64_t is it's also suitable
> for error report when used in retvals.

It is used by sizes.  We don't return errors right now.
And if it is negative, we need to check that if it is negative, even
when no code uses the negative functionality.

> But no strong opinion here, I don't think that's a huge deal for now.
> Having such an alignment on types makes sense to me.

Thanks, Juan.