From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W0YTv-0005YP-I9 for qemu-devel@nongnu.org; Tue, 07 Jan 2014 10:22:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W0YTq-0003Rx-OS for qemu-devel@nongnu.org; Tue, 07 Jan 2014 10:22:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W0YTq-0003Rr-GQ for qemu-devel@nongnu.org; Tue, 07 Jan 2014 10:22:10 -0500 Message-ID: <52CC1B98.801@redhat.com> Date: Tue, 07 Jan 2014 16:22:00 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1388906864-1083-1-git-send-email-qiaonuohan@cn.fujitsu.com> <1388906864-1083-9-git-send-email-qiaonuohan@cn.fujitsu.com> In-Reply-To: <1388906864-1083-9-git-send-email-qiaonuohan@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 08/11] dump: Add APIs to operate DataCache List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Qiao Nuohan Cc: stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, kumagai-atsushi@mxc.nes.nec.co.jp, anderson@redhat.com, akong@redhat.com, afaerber@suse.de comments below On 01/05/14 08:27, Qiao Nuohan wrote: > DataCache is used to store data temporarily, then the data will be written to > vmcore. These functions will be called later when writing data of page to > vmcore. > > Signed-off-by: Qiao Nuohan > --- > dump.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 9 ++++++++ > 2 files changed, 61 insertions(+), 0 deletions(-) > > diff --git a/dump.c b/dump.c > index 1fae152..b4c40f2 100644 > --- a/dump.c > +++ b/dump.c > @@ -1088,6 +1088,58 @@ out: > return ret; > } > > +static void prepare_data_cache(DataCache *data_cache, DumpState *s) > +{ > + data_cache->fd = s->fd; > + data_cache->data_size = 0; > + data_cache->buf_size = BUFSIZE_DATA_CACHE; > + data_cache->buf = g_malloc0(BUFSIZE_DATA_CACHE); > +} > + > +static int write_cache(DataCache *dc, bool flag_flatten, void *buf, size_t size) > +{ > + /* > + * check if the space is enough to cache data, if not, write the cached > + * data to dc->fd and reset the buf > + */ > + if (dc->data_size + size > dc->buf_size) { > + if (write_buffer(dc->fd, flag_flatten, dc->offset, dc->buf, > + dc->data_size) < 0) { > + return -1; > + } > + > + dc->offset += dc->data_size; > + dc->data_size = 0; > + } > + > + memcpy(dc->buf + dc->data_size, buf, size); > + dc->data_size += size; > + > + return 0; > +} I think we should at least add a check ("if" or assert()) because the passed-in "size" could be greater than all of the room we have in the buffer (ie. size > dc->buf_size), and in that case we'd overflow the buffer. Then, > + > +/* write the remaining data in dc->buf to dc->fd */ > +static int sync_data_cache(DataCache *dc, bool flag_flatten) > +{ > + if (dc->data_size == 0) { > + return 0; > + } > + > + if (write_buffer(dc->fd, flag_flatten, dc->offset, dc->buf, > + dc->data_size) < 0) { > + return -1; > + } > + > + dc->offset += dc->data_size; > + > + return 0; > +} Incrementing the offset here, but not resetting dc->data_size, seems inconsistent. Do both or neither. Ideally, do both, and rebase write_cache() on top of this (ie. when syncing is necessary, call sync_data_cache() from write_cache()). It doesn't cause problems as-is in the current patchset though. > + > +static void free_data_cache(DataCache *data_cache) > +{ > + g_free(data_cache->buf); > +} > + > static ram_addr_t get_start_block(DumpState *s) > { > GuestPhysBlock *block; > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index b5eaf8d..ab44af8 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -36,6 +36,7 @@ > #define BUFSIZE_BITMAP (PAGE_SIZE) > #define PFN_BUFBITMAP (CHAR_BIT * BUFSIZE_BITMAP) > #define ARCH_PFN_OFFSET (0) > +#define BUFSIZE_DATA_CACHE (PAGE_SIZE * 4) > > #define paddr_to_pfn(X, page_shift) \ > (((unsigned long long)(X) >> (page_shift)) - ARCH_PFN_OFFSET) > @@ -140,6 +141,14 @@ typedef struct QEMU_PACKED KdumpSubHeader64 { > uint64_t max_mapnr_64; /* header_version 6 and later */ > } KdumpSubHeader64; > > +typedef struct DataCache { > + int fd; /* fd of the file where to write the cached data */ > + char *buf; /* buffer for cached data */ > + size_t buf_size; /* size of the buf */ > + size_t data_size; /* size of cached data in buf */ > + off_t offset; /* offset of the file */ > +} DataCache; > + > struct GuestPhysBlockList; /* memory_mapping.h */ > int cpu_get_dump_info(ArchDumpInfo *info, > const struct GuestPhysBlockList *guest_phys_blocks); > I feel that stuff that depends on page size should be centralized somehow. I can't describe it very well now, but I feel that having a bunch of macros that open-code the page size as 4096, and using struct members elsewhere (with dynamically set values) for the same purpose, is a mess. However that could be refactored in a separate series, *if* you think it would be worthwhile. Reviewed-by: Laszlo Ersek