| Message ID | 20220721132256.2171-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
Hi On Thu, Jul 21, 2022 at 5:23 PM Janosch Frank <frankja@linux.ibm.com> wrote: > > The iteration over the memblocks is hard to understand so it's about > time to clean it up. Instead of manually grabbing the next memblock we > can use QTAILQ_FOREACH to iterate over all memblocks. > > Additionally we move the calculation of the offset and length out by > using the dump_get_memblock_*() functions. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 91 +++++++++++-------------------------------- > include/sysemu/dump.h | 9 ++--- > 2 files changed, 27 insertions(+), 73 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 0ed7cf9c7b..5c9ed25c5a 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -591,56 +591,27 @@ static void dump_begin(DumpState *s, Error **errp) > write_elf_notes(s, errp); > } > > -static int get_next_block(DumpState *s, GuestPhysBlock *block) > -{ > - while (1) { > - block = QTAILQ_NEXT(block, next); > - if (!block) { > - /* no more block */ > - return 1; > - } > - > - s->start = 0; > - s->next_block = block; > - if (s->has_filter) { > - if (block->target_start >= s->begin + s->length || > - block->target_end <= s->begin) { > - /* This block is out of the range */ > - continue; > - } > - > - if (s->begin > block->target_start) { > - s->start = s->begin - block->target_start; > - } > - } > - > - return 0; > - } > -} > - > /* write all memory to vmcore */ > static void dump_iterate(DumpState *s, Error **errp) > { > ERRP_GUARD(); > GuestPhysBlock *block; > - int64_t size; > + int64_t memblock_size, memblock_start; > > - do { > - block = s->next_block; > - > - size = block->target_end - block->target_start; > - if (s->has_filter) { > - size -= s->start; > - if (s->begin + s->length < block->target_end) { > - size -= block->target_end - (s->begin + s->length); > - } > + QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { > + memblock_start = dump_get_memblock_start(block, s->begin, s->length); > + if (memblock_start == -1) { > + continue; > } > - write_memory(s, block, s->start, size, errp); > + > + memblock_size = dump_get_memblock_size(block, s->begin, s->length); > + > + /* Write the memory to file */ > + write_memory(s, block, memblock_start, memblock_size, errp); > if (*errp) { > return; > } > - > - } while (!get_next_block(s, block)); > + } > } > > static void create_vmcore(DumpState *s, Error **errp) > @@ -1490,30 +1461,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp) > } > } > > -static ram_addr_t get_start_block(DumpState *s) > +static int validate_start_block(DumpState *s) > { > GuestPhysBlock *block; > > if (!s->has_filter) { > - s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head); > return 0; > } > > QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { > + /* This block is out of the range */ > if (block->target_start >= s->begin + s->length || > block->target_end <= s->begin) { > - /* This block is out of the range */ > continue; > } > - > - s->next_block = block; > - if (s->begin > block->target_start) { > - s->start = s->begin - block->target_start; > - } else { > - s->start = 0; > - } > - return s->start; > - } > + return 0; > + } > > return -1; > } > @@ -1540,25 +1503,17 @@ bool qemu_system_dump_in_progress(void) > return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE); > } > > -/* calculate total size of memory to be dumped (taking filter into > - * acoount.) */ > +/* > + * calculate total size of memory to be dumped (taking filter into > + * account.) > + */ > static int64_t dump_calculate_size(DumpState *s) > { > GuestPhysBlock *block; > - int64_t size = 0, total = 0, left = 0, right = 0; > + int64_t total = 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; > + total += dump_get_memblock_size(block, s->begin, s->length); > } > > return total; > @@ -1660,8 +1615,8 @@ static void dump_init(DumpState *s, int fd, bool has_format, > goto cleanup; > } > > - s->start = get_start_block(s); > - if (s->start == -1) { > + /* Is the filter filtering everything? */ > + if (validate_start_block(s) == -1) { > error_setg(errp, QERR_INVALID_PARAMETER, "begin"); > goto cleanup; > } > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 2b39abeeae..7025e50682 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -166,11 +166,10 @@ typedef struct DumpState { > hwaddr memory_offset; > int fd; > > - GuestPhysBlock *next_block; > - ram_addr_t start; > - bool has_filter; > - int64_t begin; > - int64_t length; > + /* Guest memory related data */ > + bool has_filter; /* Are we dumping parts of the memory? */ > + int64_t begin; /* Start address of the chunk we want to dump */ > + int64_t length; /* Length of the dump we want to dump */ > > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */ > -- > 2.34.1 > My suggestion in v2 review was to introduce each function & refactoring independently, if possible. And it looks like the validate_start_block() change could be a 3rd patch too.
On 7/21/22 16:36, Marc-André Lureau wrote: [..] >> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h >> index 2b39abeeae..7025e50682 100644 >> --- a/include/sysemu/dump.h >> +++ b/include/sysemu/dump.h >> @@ -166,11 +166,10 @@ typedef struct DumpState { >> hwaddr memory_offset; >> int fd; >> >> - GuestPhysBlock *next_block; >> - ram_addr_t start; >> - bool has_filter; >> - int64_t begin; >> - int64_t length; >> + /* Guest memory related data */ >> + bool has_filter; /* Are we dumping parts of the memory? */ >> + int64_t begin; /* Start address of the chunk we want to dump */ >> + int64_t length; /* Length of the dump we want to dump */ >> >> uint8_t *note_buf; /* buffer for notes */ >> size_t note_buf_offset; /* the writing place in note_buf */ >> -- >> 2.34.1 >> > > My suggestion in v2 review was to introduce each function & > refactoring independently, if possible. And it looks like the > validate_start_block() change could be a 3rd patch too. > > Alright, I just squashed and split this into 5 patches: * Introducing the 2 new functions * Converting dump_iterate and removing get_next_block * get_start_block -> validate_start_block * Removal of next_block and start from DumpState, last user was get_start_block * Re-work of dump_calculate_size I don't think we can easily adapt to dump_get_memblock_size() and dump_get_memblock_start() independently. I'll also move the DumpState comment hunk to the removal of start and next_block tomorrow.
diff --git a/dump/dump.c b/dump/dump.c index 0ed7cf9c7b..5c9ed25c5a 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -591,56 +591,27 @@ static void dump_begin(DumpState *s, Error **errp) write_elf_notes(s, errp); } -static int get_next_block(DumpState *s, GuestPhysBlock *block) -{ - while (1) { - block = QTAILQ_NEXT(block, next); - if (!block) { - /* no more block */ - return 1; - } - - s->start = 0; - s->next_block = block; - if (s->has_filter) { - if (block->target_start >= s->begin + s->length || - block->target_end <= s->begin) { - /* This block is out of the range */ - continue; - } - - if (s->begin > block->target_start) { - s->start = s->begin - block->target_start; - } - } - - return 0; - } -} - /* write all memory to vmcore */ static void dump_iterate(DumpState *s, Error **errp) { ERRP_GUARD(); GuestPhysBlock *block; - int64_t size; + int64_t memblock_size, memblock_start; - do { - block = s->next_block; - - size = block->target_end - block->target_start; - if (s->has_filter) { - size -= s->start; - if (s->begin + s->length < block->target_end) { - size -= block->target_end - (s->begin + s->length); - } + QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { + memblock_start = dump_get_memblock_start(block, s->begin, s->length); + if (memblock_start == -1) { + continue; } - write_memory(s, block, s->start, size, errp); + + memblock_size = dump_get_memblock_size(block, s->begin, s->length); + + /* Write the memory to file */ + write_memory(s, block, memblock_start, memblock_size, errp); if (*errp) { return; } - - } while (!get_next_block(s, block)); + } } static void create_vmcore(DumpState *s, Error **errp) @@ -1490,30 +1461,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp) } } -static ram_addr_t get_start_block(DumpState *s) +static int validate_start_block(DumpState *s) { GuestPhysBlock *block; if (!s->has_filter) { - s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head); return 0; } QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { + /* This block is out of the range */ if (block->target_start >= s->begin + s->length || block->target_end <= s->begin) { - /* This block is out of the range */ continue; } - - s->next_block = block; - if (s->begin > block->target_start) { - s->start = s->begin - block->target_start; - } else { - s->start = 0; - } - return s->start; - } + return 0; + } return -1; } @@ -1540,25 +1503,17 @@ bool qemu_system_dump_in_progress(void) return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE); } -/* calculate total size of memory to be dumped (taking filter into - * acoount.) */ +/* + * calculate total size of memory to be dumped (taking filter into + * account.) + */ static int64_t dump_calculate_size(DumpState *s) { GuestPhysBlock *block; - int64_t size = 0, total = 0, left = 0, right = 0; + int64_t total = 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; + total += dump_get_memblock_size(block, s->begin, s->length); } return total; @@ -1660,8 +1615,8 @@ static void dump_init(DumpState *s, int fd, bool has_format, goto cleanup; } - s->start = get_start_block(s); - if (s->start == -1) { + /* Is the filter filtering everything? */ + if (validate_start_block(s) == -1) { error_setg(errp, QERR_INVALID_PARAMETER, "begin"); goto cleanup; } diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index 2b39abeeae..7025e50682 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -166,11 +166,10 @@ typedef struct DumpState { hwaddr memory_offset; int fd; - GuestPhysBlock *next_block; - ram_addr_t start; - bool has_filter; - int64_t begin; - int64_t length; + /* Guest memory related data */ + bool has_filter; /* Are we dumping parts of the memory? */ + int64_t begin; /* Start address of the chunk we want to dump */ + int64_t length; /* Length of the dump we want to dump */ uint8_t *note_buf; /* buffer for notes */ size_t note_buf_offset; /* the writing place in note_buf */
The iteration over the memblocks is hard to understand so it's about time to clean it up. Instead of manually grabbing the next memblock we can use QTAILQ_FOREACH to iterate over all memblocks. Additionally we move the calculation of the offset and length out by using the dump_get_memblock_*() functions. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 91 +++++++++++-------------------------------- include/sysemu/dump.h | 9 ++--- 2 files changed, 27 insertions(+), 73 deletions(-)