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

[Juan Quintela <quintela@redhat.com>]

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.

>      if (qatomic_read(&CalculatingState) == DIRTY_RATE_STATUS_MEASURED) {
>          info->has_dirty_rate = true;
> @@ -245,6 +246,29 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>              info->vcpu_dirty_rate = head;
>          }
>  
> +        if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING)
> {

see my comment at the end about int64 vs uint64/size

> +        DirtyStat.page_sampling.n_total_pages = 0;
> +        DirtyStat.page_sampling.n_sampled_pages = 0;
> +        DirtyStat.page_sampling.n_readings = 0;
> +        DirtyStat.page_sampling.readings = g_try_malloc0_n(MAX_DIRTY_READINGS,
> +                                                          sizeof(DirtyReading));
>          break;

You do g_try_malloc0()

or you check for NULL return.

Even better, I think you can use here:

foo = g_new0(DirtyReading, MAX_DIRTY_READINGS);

MAX_DIRTY_READINGS is small enough that you can assume that there is
enough free memory.

> -    DirtyStat.dirty_rate = dirtyrate;
> +    if (DirtyStat.page_sampling.readings) {
> +        free(DirtyStat.page_sampling.readings);
> +        DirtyStat.page_sampling.readings = NULL;
> +    }

What glib gives, glib takes.

g_malloc() -> g_free()

g_free() knows how to handle the NULL case so:

        g_free(DirtyStat.page_sampling.readings);
        DirtyStat.page_sampling.readings = NULL;

Without if should be good enough.

> -static bool compare_page_hash_info(struct RamblockDirtyInfo *info,
> +static int64_t compare_page_hash_info(struct RamblockDirtyInfo *info,
>                                    int block_count)

                                     bad indentantion.

> +static int64_t increase_period(int64_t prev_period, int64_t max_period)
> +{
> +    int64_t delta;
> +    int64_t next_period;
> +
> +    if (prev_period < 500) {
> +        delta = 125;
> +    } else if (prev_period < 1000) {
> +        delta = 250;
> +    } else if (prev_period < 2000) {
> +        delta = 500;
> +    } else if (prev_period < 4000) {
> +        delta = 1000;
> +    } else if (prev_period < 10000) {
> +        delta = 2000;
> +    } else {
> +        delta = 5000;
> +    }
> +
> +    next_period = prev_period + delta;
> +    if (next_period + delta >= max_period) {
> +        next_period = max_period;
> +    }
> +    return next_period;
> +}

Is there any reason for this to be so complicated?


int64_t periods[] = { 125, 250, 375, 500, 750, 1000, 1500, 2000, 3000, 4000, 6000, 8000, 10000,
                      15000, 20000, 25000, 30000, 35000, 40000, 45000 };

And access it in the loop?  This way you get what the values are.

You can put a limit to config.sample_period_seconds, or you want it
unlimited?


> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2c35b7b9cf..f818f51e0e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1805,6 +1805,22 @@
>  # @vcpu-dirty-rate: dirtyrate for each vcpu if dirty-ring
>  #                   mode specified (Since 6.2)
>  #
> +# @page-size: page size in bytes (since 8.1)
> +#
> +# @n-total-pages: [page-sampling] total number of VM pages (since 8.1)
> +#
> +# @n-sampled-pages: [page-sampling] number of sampled VM pages (since 8.1)
> +#
> +# @periods: [page-sampling] array of time periods expressed in milliseconds
> +#           for which dirty-sample measurements were collected (since 8.1)
> +#
> +# @n-dirty-pages: [page-sampling] number of pages among all sampled pages
> +#                 that were observed as changed during respective time period.
> +#                 i-th element of this array corresponds to the i-th element
> +#                 of the @periods array, i.e. @n-dirty-pages[i] is the number
> +#                 of dirtied pages during period of @periods[i] milliseconds
> +#                 after the initiation of calc-dirty-rate (since 8.1)
> +#
>  # Since: 5.2
>  ##
>  { 'struct': 'DirtyRateInfo',
> @@ -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:-)

Same for the rest of the counters.

> +           '*n-total-pages': 'int64',
> +           '*n-sampled-pages': 'int64',
> +           '*periods': ['int64'],
> +           '*n-dirty-pages': ['int64'] } }
>  
>  ##
>  # @calc-dirty-rate: