From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35819) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W61Ei-0007HT-NX for qemu-devel@nongnu.org; Wed, 22 Jan 2014 12:05:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W61Ee-0004KS-3F for qemu-devel@nongnu.org; Wed, 22 Jan 2014 12:05:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W61Ed-0004Hk-Qh for qemu-devel@nongnu.org; Wed, 22 Jan 2014 12:05:04 -0500 Message-ID: <52DFFA35.6030002@redhat.com> Date: Wed, 22 Jan 2014 18:04:53 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1389944779-31899-1-git-send-email-qiaonuohan@cn.fujitsu.com> <1389944779-31899-8-git-send-email-qiaonuohan@cn.fujitsu.com> In-Reply-To: <1389944779-31899-8-git-send-email-qiaonuohan@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/13 v7] dump: add members to DumpState and init some of them List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qiaonuohan Cc: stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, anderson@redhat.com, kumagai-atsushi@mxc.nes.nec.co.jp, afaerber@suse.de 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 > --- > 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