[07/13,v7] dump: add members to DumpState and init some of them

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(-)

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>

On 01/23/2014 01:04 AM, Laszlo Ersek wrote:
>> @@ -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.
>

I see, moving them into "if(format...) {...}" block would be better. But, I
have no idea of "regression-testing (perf-wise)", would you mind give some hint?

On 01/24/14 02:52, Qiao Nuohan wrote:
> On 01/23/2014 01:04 AM, Laszlo Ersek wrote:
>>> @@ -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.
>>
> 
> I see, moving them into "if(format...) {...}" block would be better. But, I
> have no idea of "regression-testing (perf-wise)", would you mind give
> some hint?

I meant comparing how long it would take to dump in paging mode before
this patchset, vs. after this patchset. In order to see the difference
that is introduced by get_max_mapnr() when paging is enabled.

However, please ignore this point. First, the loop is most probably
negligible even for a paging dump. Second, you could make it conditional
on compressed dumps (which force non-paging + non-filtering), where the
number of mappings is very low. And third, as I wrote in later, the loop
should be replaced anyway with an O(1) QTAILQ_LAST() access.

So please just ignore this "performance" remark.

Ultimately, what I suggest for get_max_mapnr() is:
- rebase it to guest_phys_blocks, just like the other two places (which
are now calling get_next_page()),
- use QTAILQ_LAST() in it,
- don't bother making it conditional (ie. its current call site is
fine), because:
  - It'll be fast with QTAILQ_LAST(),
  - guest_phys_blocks is available in any case, so you can access it
    always

Thanks
Laszlo