Re: [RFC PATCH 3/8] migration/dirtyrate: Add dirtyrate statistics series functions

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Add dirtyrate statistics to record/update dirtyrate info.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 47 ++++++++++++++++++++++++++++++-----------------
>  migration/dirtyrate.h | 11 +++++++++++
>  2 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index fc652fb..6baf674 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -13,19 +13,41 @@
>  #include "dirtyrate.h"
>  
>  static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> -static uint64_t dirty_rate; /* MB/s */
> +static struct dirtyrate_statistics dirty_stat;
>  CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>  
> -static bool calculate_dirtyrate(struct dirtyrate_config config,
> -                        uint64_t *dirty_rate, int64_t time)
> +static void reset_dirtyrate_stat(void)
>  {
> -    /* todo */
> -    return true;
> +    dirty_stat.total_dirty_samples = 0;
> +    dirty_stat.total_sample_count = 0;
> +    dirty_stat.total_block_mem_MB = 0;
> +    dirty_stat.dirty_rate = 0;
> +}
> +
> +static void update_dirtyrate_stat(struct block_dirty_info *info)
> +{
> +    dirty_stat.total_dirty_samples += info->sample_dirty_count;
> +    dirty_stat.total_sample_count += info->sample_pages_count;
> +    dirty_stat.total_block_mem_MB += (info->block_pages << DIRTYRATE_PAGE_SIZE_SHIFT) >> PAGE_SIZE_SHIFT;
>  }
>  
> -static void set_dirty_rate(uint64_t drate)
> +static void update_dirtyrate(int64_t msec)
>  {
> -    dirty_rate = drate;
> +    uint64_t dirty_rate;
> +    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
> +    unsigned int total_sample_count = dirty_stat.total_sample_count;
> +    unsigned long total_block_mem_MB = dirty_stat.total_block_mem_MB;
> +
> +    dirty_rate = total_dirty_samples * total_block_mem_MB *
> +                 1000 / (total_sample_count * msec);
> +
> +    dirty_stat.dirty_rate = dirty_rate;
> +}
> +
> +
> +static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
> +{
> +    /* todo */
>  }
>  
>  /*
> @@ -42,21 +64,12 @@ static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
> -    uint64_t dirty_rate;
> -    uint64_t hash_dirty_rate;
> -    bool query_succ;
>      int64_t msec = 0;
>   
>      set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
>  
> -    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
> -    if (!query_succ) {
> -        dirty_rate = 0;
> -    } else {
> -        dirty_rate = hash_dirty_rate;
> -    }

All this was only just added; it might be easier to create the
update_dirtyrate function first.

> +    calculate_dirtyrate(config, msec);
>  
> -    set_dirty_rate(dirty_rate);
>      set_dirty_rate_stage(CAL_DIRTY_RATE_END);
>  
>      return NULL;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 342b89f..2994535 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,6 +15,9 @@
>  
>  /* take 256 pages per GB for cal dirty rate */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
> +#define DIRTYRATE_PAGE_SIZE_SHIFT       12
> +#define BLOCK_INFO_MAX_LEN              256
> +#define PAGE_SIZE_SHIFT                 20

I think you might also have used one of these #define's in a previous
patch; so make sure the patches each compile in order.
Also, can you please comment each one of these - I was confused by a lot
of the calculations above because I don't quite understand what each of
these is.
I don't thinl 'BLOCK_INFO_MAX_LEN' is needed - becuase it's just a
RAMBlock ID, and you can link to the RAMBlock.
I'm not sure what DIRTYRATE_PAGE_SIZE_SHIFT is, or why it's different
from TARGET_PAGE_BITS.

>  
>  struct dirtyrate_config {
>      uint64_t sample_pages_per_gigabytes;
> @@ -33,6 +36,14 @@ typedef enum {
>      CAL_DIRTY_RATE_END   = 2,
>  } CalculatingDirtyRateStage;
>  
> +struct dirtyrate_statistics {
> +    unsigned int total_dirty_samples;
> +    unsigned int total_sample_count;
> +    unsigned long total_block_mem_MB;

'long' is normally a bad idea - we use it in a few places and it
was generally a bad idea; size_t for a size is much better.

> +    int64_t dirty_rate;

Is this blocks/sec, MB/s what - please comment it.

Dave

> +};
> +
> +
>  /* 
>   * Store dirtypage info for each block.
>   */
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK