| Message ID | 1448976530-15984-10-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 12/01 21:28, Peter Xu wrote: > Here, total_size is the size in bytes to be dumped (raw data, which > means before compression), while written_size are bytes handled (raw > size too). > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > dump.c | 32 ++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 9 +++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/dump.c b/dump.c > index 5b040b7..daa1f2c 100644 > --- a/dump.c > +++ b/dump.c > @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp) > if (ret < 0) { > error_setg(errp, "dump: failed to save memory"); > } > + > + s->written_size += length; If (ret < 0), the incremental is inaccurate, do we want an accurate written_size in that case? I.e. put this in "else" branch? > } > > /* write the memory to vmcore. 1 page per I/O. */ > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > goto out; > } > } > + s->written_size += TARGET_PAGE_SIZE; The same question applies here. > } > > ret = write_cache(&page_desc, NULL, 0, true); > @@ -1433,6 +1436,30 @@ bool dump_in_progress(void) > return (state->status == DUMP_STATUS_ACTIVE); > } > > +/* calculate total size of memory to be dumped (taking filter into > + * acoount.) */ > +static size_t dump_calculate_size(DumpState *s) Is size_t big enough for 64 bit guest on 32 bit host (with 4 bytes size_t)? > +{ > + GuestPhysBlock *block; > + int64_t size = 0, total = 0, left = 0, right = 0; > + > + QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { > + if (s->has_filter) { > + /* calculate the overlapped region. */ > + left = MAX(s->begin, block->target_start); > + right = MIN(s->begin + s->length, block->target_end); > + size = right - left; > + size = size > 0 ? size : 0; > + } else { > + /* count the whole region in */ > + size = (block->target_end - block->target_start); > + } > + total += size; > + } > + > + return total; > +} > + > static void dump_init(DumpState *s, int fd, bool has_format, > DumpGuestMemoryFormat format, bool paging, bool has_filter, > int64_t begin, int64_t length, Error **errp) > @@ -1444,6 +1471,7 @@ static void dump_init(DumpState *s, int fd, bool has_format, > > s->has_format = has_format; > s->format = format; > + s->written_size = 0; > > /* kdump-compressed is conflict with paging and filter */ > if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) { > @@ -1475,6 +1503,10 @@ static void dump_init(DumpState *s, int fd, bool has_format, > > guest_phys_blocks_init(&s->guest_phys_blocks); > guest_phys_blocks_append(&s->guest_phys_blocks); > + s->total_size = dump_calculate_size(s); > +#ifdef DEBUG_DUMP_GUEST_MEMORY > + fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size); > +#endif > > s->start = get_start_block(s); > if (s->start == -1) { > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 31930c6..9c5a46b 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -189,6 +189,15 @@ typedef struct DumpState { > bool has_format; /* whether format is provided */ > DumpGuestMemoryFormat format; /* valid only if has_format == true */ > QemuThread dump_thread; /* thread for detached dump */ > + > + size_t total_size; /* total memory size (in bytes) to > + * be dumped. When filter is > + * enabled, this will only count > + * those to be written. */ > + size_t written_size; /* written memory size (in bytes), > + * this could be used to calculate > + * how many work we have s/many/much/ > + * finished. */ > } DumpState; > > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > -- > 2.4.3 >
On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote: > On Tue, 12/01 21:28, Peter Xu wrote: > > @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp) > > if (ret < 0) { > > error_setg(errp, "dump: failed to save memory"); > > } > > + > > + s->written_size += length; > > If (ret < 0), the incremental is inaccurate, do we want an accurate > written_size in that case? I.e. put this in "else" branch? Yes, I think I'd better put it into an else block... Thanks. Actually even that could not be accurate, since the write() could be partly done and failed. But I hope that's good enough for dump status queries. :) > > > } > > > > /* write the memory to vmcore. 1 page per I/O. */ > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > goto out; > > } > > } > > + s->written_size += TARGET_PAGE_SIZE; > > The same question applies here. For kdump case, it is using "goto out" when error happens. So it seems to have no problem here? > > > } > > > > ret = write_cache(&page_desc, NULL, 0, true); > > @@ -1433,6 +1436,30 @@ bool dump_in_progress(void) > > return (state->status == DUMP_STATUS_ACTIVE); > > } > > > > +/* calculate total size of memory to be dumped (taking filter into > > + * acoount.) */ > > +static size_t dump_calculate_size(DumpState *s) > > Is size_t big enough for 64 bit guest on 32 bit host (with 4 bytes size_t)? Thanks to point out. Will use int64_t here (and also written_size and total_size). > > + size_t written_size; /* written memory size (in bytes), > > + * this could be used to calculate > > + * how many work we have > > s/many/much/ Will fix it. Thanks! Peter
On Wed, 12/02 16:49, Peter Xu wrote: > On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote: > > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > > goto out; > > > } > > > } > > > + s->written_size += TARGET_PAGE_SIZE; > > > > The same question applies here. > > For kdump case, it is using "goto out" when error happens. So it > seems to have no problem here? write_cache can fail after you increment it here, no? Fam
On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote: > On Wed, 12/02 16:49, Peter Xu wrote: > > On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote: > > > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > > > goto out; > > > > } > > > > } > > > > + s->written_size += TARGET_PAGE_SIZE; > > > > > > The same question applies here. > > > > For kdump case, it is using "goto out" when error happens. So it > > seems to have no problem here? > > write_cache can fail after you increment it here, no? I am adding it at the end of loop. It looks like: while (get_next_page(&block_iter, &pfn_iter, &buf, s)) { if (is_zero_page(buf, TARGET_PAGE_SIZE)) { ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor), false); if (ret < 0) { error_setg(errp, "XXXXXXXX"); goto out; } } else { ... ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false); if (ret < 0) { error_setg(errp, "XXXXXXXX"); goto out; } ... } s->written_size += TARGET_PAGE_SIZE; } Label "out" is out of the loop. So, when error happens, it sets the errp and directly jump out of the loop. Did I miss anything? Thanks! Peter > > Fam
On Wed, 12/02 18:41, Peter Xu wrote: > On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote: > > On Wed, 12/02 16:49, Peter Xu wrote: > > > On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote: > > > > > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > > > > goto out; > > > > > } > > > > > } > > > > > + s->written_size += TARGET_PAGE_SIZE; > > > > > > > > The same question applies here. > > > > > > For kdump case, it is using "goto out" when error happens. So it > > > seems to have no problem here? > > > > write_cache can fail after you increment it here, no? > > I am adding it at the end of loop. It looks like: > > while (get_next_page(&block_iter, &pfn_iter, &buf, s)) { > if (is_zero_page(buf, TARGET_PAGE_SIZE)) { > ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor), > false); > if (ret < 0) { > error_setg(errp, "XXXXXXXX"); > goto out; > } > } else { > ... > ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false); > if (ret < 0) { > error_setg(errp, "XXXXXXXX"); > goto out; > } > ... > } > s->written_size += TARGET_PAGE_SIZE; > } > > Label "out" is out of the loop. So, when error happens, it sets the > errp and directly jump out of the loop. Did I miss anything? > You are right, I misread the context of this incremental. Thanks. Fam
On Wed, Dec 02, 2015 at 08:51:48PM +0800, Fam Zheng wrote: > On Wed, 12/02 18:41, Peter Xu wrote: > > On Wed, Dec 02, 2015 at 05:49:18PM +0800, Fam Zheng wrote: > > Label "out" is out of the loop. So, when error happens, it sets the > > errp and directly jump out of the loop. Did I miss anything? > > > > You are right, I misread the context of this incremental. Thanks. Thanks for review my first patch (even until v4 and more). :) Peter > > Fam
diff --git a/dump.c b/dump.c index 5b040b7..daa1f2c 100644 --- a/dump.c +++ b/dump.c @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp) if (ret < 0) { error_setg(errp, "dump: failed to save memory"); } + + s->written_size += length; } /* write the memory to vmcore. 1 page per I/O. */ @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp) goto out; } } + s->written_size += TARGET_PAGE_SIZE; } ret = write_cache(&page_desc, NULL, 0, true); @@ -1433,6 +1436,30 @@ bool dump_in_progress(void) return (state->status == DUMP_STATUS_ACTIVE); } +/* calculate total size of memory to be dumped (taking filter into + * acoount.) */ +static size_t dump_calculate_size(DumpState *s) +{ + GuestPhysBlock *block; + int64_t size = 0, total = 0, left = 0, right = 0; + + QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { + if (s->has_filter) { + /* calculate the overlapped region. */ + left = MAX(s->begin, block->target_start); + right = MIN(s->begin + s->length, block->target_end); + size = right - left; + size = size > 0 ? size : 0; + } else { + /* count the whole region in */ + size = (block->target_end - block->target_start); + } + total += size; + } + + return total; +} + static void dump_init(DumpState *s, int fd, bool has_format, DumpGuestMemoryFormat format, bool paging, bool has_filter, int64_t begin, int64_t length, Error **errp) @@ -1444,6 +1471,7 @@ static void dump_init(DumpState *s, int fd, bool has_format, s->has_format = has_format; s->format = format; + s->written_size = 0; /* kdump-compressed is conflict with paging and filter */ if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) { @@ -1475,6 +1503,10 @@ static void dump_init(DumpState *s, int fd, bool has_format, guest_phys_blocks_init(&s->guest_phys_blocks); guest_phys_blocks_append(&s->guest_phys_blocks); + s->total_size = dump_calculate_size(s); +#ifdef DEBUG_DUMP_GUEST_MEMORY + fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size); +#endif s->start = get_start_block(s); if (s->start == -1) { diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index 31930c6..9c5a46b 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -189,6 +189,15 @@ typedef struct DumpState { bool has_format; /* whether format is provided */ DumpGuestMemoryFormat format; /* valid only if has_format == true */ QemuThread dump_thread; /* thread for detached dump */ + + size_t total_size; /* total memory size (in bytes) to + * be dumped. When filter is + * enabled, this will only count + * those to be written. */ + size_t written_size; /* written memory size (in bytes), + * this could be used to calculate + * how many work we have + * finished. */ } DumpState; uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
Here, total_size is the size in bytes to be dumped (raw data, which means before compression), while written_size are bytes handled (raw size too). Signed-off-by: Peter Xu <peterx@redhat.com> --- dump.c | 32 ++++++++++++++++++++++++++++++++ include/sysemu/dump.h | 9 +++++++++ 2 files changed, 41 insertions(+)