Re: [Qemu-devel] [PATCH 07/13 v7] dump: add members to DumpState and init some of them

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 01/17/14 08:46, qiaonuohan wrote:
> add some members to DumpState that will be used in writing vmcore in
> kdump-compressed format. some of them, like page_size, will be initialized
> in the patch.
> 
> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> ---
>  dump.c                |   30 ++++++++++++++++++++++++++++++
>  include/sysemu/dump.h |    7 +++++++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 2b940bd..bf7d31d 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -79,6 +79,16 @@ typedef struct DumpState {
>  
>      uint8_t *note_buf;          /* buffer for notes */
>      size_t note_buf_offset;     /* the writing place in note_buf */
> +    uint32_t nr_cpus;           /* number of guest's cpu */
> +    size_t page_size;           /* guest's page size */
> +    uint32_t page_shift;        /* guest's page shift */
> +    uint64_t max_mapnr;         /* the biggest guest's phys-mem's number */
> +    size_t len_dump_bitmap;     /* the size of the place used to store
> +                                   dump_bitmap in vmcore */
> +    off_t offset_dump_bitmap;   /* offset of dump_bitmap part in vmcore */
> +    off_t offset_page;          /* offset of page part in vmcore */
> +    size_t num_dumpable;        /* number of page that can be dumped */
> +    uint32_t flag_compress;     /* indicate the compression format */
>  } DumpState;

v6 06/11 addded these, but we have the following changes here:
- flag_flatten is gone, OK,
- bunch of comments, good,
- page_shift and num_dumpable are now added at once (originally in v6
07/11).

>  
>  static int dump_cleanup(DumpState *s)
> @@ -796,6 +806,16 @@ static ram_addr_t get_start_block(DumpState *s)
>      return -1;
>  }
>  
> +static void get_max_mapnr(DumpState *s)
> +{
> +    MemoryMapping *memory_mapping;
> +
> +    QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
> +        s->max_mapnr = paddr_to_pfn(memory_mapping->phys_addr +
> +                        memory_mapping->length, s->page_shift);
> +    }
> +}
> +
>  static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>                       int64_t begin, int64_t length, Error **errp)
>  {

This is from v6 10/11, OK.

> @@ -864,6 +884,16 @@ static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>          qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
>      }
>  
> +    s->nr_cpus = nr_cpus;
> +    s->page_size = TARGET_PAGE_SIZE;
> +    s->page_shift = ffs(s->page_size) - 1;
> +
> +    get_max_mapnr(s);

Again from v6 10/11, good. The flag_flatten assignment has been dropped.
Initialization seems to happen in a good spot this time too.

> +
> +    uint64_t tmp;
> +    tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), s->page_size);
> +    s->len_dump_bitmap = tmp * s->page_size;
> +
>      if (s->has_filter) {
>          memory_mapping_filter(&s->list, s->begin, s->length);
>      }

Again from v6 10/11.

These assignments now all occur without depending on a user request for
a compressed dump (kept this way in v7 12/13 too), but they are not
costly. The loop in get_max_mapnr() iterates over less than 10 mappings
in the non-paging dump case, and in the paging dump case it also
shouldn't be more than a hundred or so (as I recall from earlier
testing). This might be worth some regression-testing (perf-wise), but
it looks OK to me.

> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b32b390..995bf47 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -20,6 +20,13 @@
>  #define VERSION_FLAT_HEADER         (1)    /* version of flattened format */
>  #define END_FLAG_FLAT_HEADER        (-1)
>  
> +#define ARCH_PFN_OFFSET             (0)
> +
> +#define paddr_to_pfn(X, page_shift) \
> +    (((unsigned long long)(X) >> (page_shift)) - ARCH_PFN_OFFSET)

>From v6 07/11, needed by get_max_mapnr().

> +#define pfn_to_paddr(X, page_shift) \
> +    (((unsigned long long)(X) + ARCH_PFN_OFFSET) << (page_shift))
> +
>  typedef struct ArchDumpInfo {
>      int d_machine;  /* Architecture */
>      int d_endian;   /* ELFDATA2LSB or ELFDATA2MSB */
> 

>From v6 09/11. Not strictly needed right now, but it does make sense for
consistency.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>