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

[Qiao Nuohan <qiaonuohan@cn.fujitsu.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?

-- 
Regards
Qiao Nuohan