Re: [PATCH v2 2/4] migration/calc-dirty-rate: detailed stats in sampling mode

[Juan Quintela <quintela@redhat.com>]

<gudkov.andrei@huawei.com> wrote:
> On Wed, May 10, 2023 at 07:36:33PM +0200, Juan Quintela wrote:
>> Andrei Gudkov <gudkov.andrei@huawei.com> wrote:
>> > Collect number of dirty pages for progresseively increasing time
>> > periods starting with 125ms up to number of seconds specified with
>> > calc-dirty-rate. Report through qmp and hmp: 1) vector of dirty page
>> > measurements, 2) page size, 3) total number of VM pages, 4) number
>> > of sampled pages.
>> >
>> > Signed-off-by: Andrei Gudkov <gudkov.andrei@huawei.com>
>> > ---
>> >  migration/dirtyrate.c | 165 +++++++++++++++++++++++++++++-------------
>> >  migration/dirtyrate.h |  25 ++++++-
>> >  qapi/migration.json   |  24 +++++-
>> 
>> You need the equivalent of this in your .git/config file.
>> 
>> [diff]
>> 	orderFile = scripts/git.orderfile
>> 
>> In particular:
>> *json files cames first
>> *.h second
>> *.c third
>> 
>> >  3 files changed, 160 insertions(+), 54 deletions(-)
>> >
>> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> > index acba3213a3..4491bbe91a 100644
>> > --- a/migration/dirtyrate.c
>> > +++ b/migration/dirtyrate.c
>> > @@ -224,6 +224,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>> >      info->calc_time = DirtyStat.calc_time;
>> >      info->sample_pages = DirtyStat.sample_pages;
>> >      info->mode = dirtyrate_mode;
>> > +    info->page_size = TARGET_PAGE_SIZE;
>> 
>> I thought we exported this trough ""info migrate"
>> but we do it only if we are in the middle of a migration.  Perhaps we
>> should print it always.
>
> So, which one do you prefer? To keep it here or to make "info migrate" print it always (or both)?

info migrate to print it always.  Thanks.

>> > @@ -1814,7 +1830,13 @@
>> >             'calc-time': 'int64',
>> >             'sample-pages': 'uint64',
>> >             'mode': 'DirtyRateMeasureMode',
>> > -           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ] } }
>> > +           '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ],
>> > +           'page-size': 'int64',
>> 
>> 2 things:
>> a- this is exported in info migrate, so you can get it from there.
>> b- even if we export it here, it is as size or uint64, negative page
>>    size make no sense (not that I am expecting to have page that don't
>>    fit in a int O:-)
>
> But can you be sure that in the future you are not going to return
> sentinel value like "-1"? :)

For page-size?  I don't expect it.  And if I want a sentinel values,
UINT64_MAX, UINT64_MAX-1 and friends looks good enough for me.

>> Same for the rest of the counters.
>
> Ok, but I still insist on using 64 bit types for the page number counters.
> It looks to me that 16TiB VM is a matter of near future.

size_t is 64 bits on any host that is able to handle 16TiB guests O:-)
But I am indifferent to uint64_t or size_t (I am only on 64 bit machines,
so it is the same for me).  That would only help in 32bits hosts, but I
am pretty sure that nobody is using them seriously (for migration)
because all the 32bit bugs that I get on migration is from the build
bots, not real users.

Later, Juan.