Andrei Gudkov <gudkov.and...@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.and...@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:
