From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8Cke-0003wi-D5 for qemu-devel@nongnu.org; Fri, 18 Jul 2014 14:19:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X8CkR-0004zv-Se for qemu-devel@nongnu.org; Fri, 18 Jul 2014 14:19:24 -0400 Received: from mail-pa0-x234.google.com ([2607:f8b0:400e:c03::234]:57748) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8CkR-0004zq-GR for qemu-devel@nongnu.org; Fri, 18 Jul 2014 14:19:11 -0400 Received: by mail-pa0-f52.google.com with SMTP id bj1so5910277pad.39 for ; Fri, 18 Jul 2014 11:19:10 -0700 (PDT) Message-ID: <53C96512.2040204@gmail.com> Date: Fri, 18 Jul 2014 23:48:58 +0530 From: Sanidhya Kashyap MIME-Version: 1.0 References: <1405596081-29701-1-git-send-email-sanidhya.iiith@gmail.com> <1405596081-29701-4-git-send-email-sanidhya.iiith@gmail.com> <20140718111228.GE2384@work-vm> In-Reply-To: <20140718111228.GE2384@work-vm> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/8] BitmapLog: bitmap dump code via QAPI framework with runstates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Amit Shah , qemu list , Juan Quintela >> +#define RAMBLOCK_NAME_LENGTH (1<<8) > > Be careful; making this bigger would break migration formats, > making it smaller would probably break migration loading. > its the same as previous, will write it explicitly. What I think is that we should make name_length to be global. I have seen at most of the places that we are using 256 as length, why can't we use a variable? >> + >> +enum { >> + LOG_BITMAP_STATE_ERROR = -1, >> + LOG_BITMAP_STATE_NONE, >> + LOG_BITMAP_STATE_SETUP, >> + LOG_BITMAP_STATE_ACTIVE, >> + LOG_BITMAP_STATE_CANCELING, >> + LOG_BITMAP_STATE_COMPLETED >> +}; > > I'd be tempted to give that enum a name and use it as the type > for functions that pass the state around (although I realise > you have to be careful with the variable having to be an int > for the atomic). > will see what can be done. >> +static inline void logging_lock(void) >> +{ >> + qemu_mutex_lock_iothread(); >> + qemu_mutex_lock_ramlist(); >> +} > > I wonder how often you can really not have the ramlist locked; if stuff > is added/removed the last_ram_offset would change, so to really be safe > you probably need to hold it for much longer than you currently do - > but that might not be practical. > I didn't realize that. I will try to make suitable changes in my code to cater to those added/removed last_ram_offset. >> +static inline void logging_bitmap_set_dirty(ram_addr_t addr) >> +{ >> + int nr = addr >> TARGET_PAGE_BITS; > > Be careful; int is too small; long is probably what's > needed (which I think is the type of the parameter to set_bit). > My mistake. I will rectify it! >> +static inline bool value_in_range(int64_t value, int64_t min_value, >> + int64_t max_value, const char *str, >> + Error **errp) >> +{ >> + if (value < min_value) { >> + error_setg(errp, "%s's value must be greater than %ld", >> + str, min_value); >> + return false; >> + } >> + if (value > max_value) { >> + error_setg(errp, "%s's value must be less than %ld", >> + str, max_value); >> + return false; >> + } >> + return true; >> +} > > This seems a pretty generic function; could it go somewhere > in util or the like? > okay. >> + >> +static void dirty_bitmap_sync(void) >> +{ >> + RAMBlock *block; >> + address_space_sync_dirty_bitmap(&address_space_memory); >> + QTAILQ_FOREACH(block, &ram_list.blocks, next) { >> + qemu_bitmap_sync_range(block->mr->ram_addr, block->length, >> + logging_bitmap, false); >> + } >> +} > > I think that should be logging_dirty_bitmap_sync since it's > specific to logging_bitmap. > okay. Will change it. >> + >> +static void logging_state_update_status(BitmapLogState *b) >> +{ >> + switch (b->state) { >> + case LOG_BITMAP_STATE_ACTIVE: >> + logging_state_set_status(b, LOG_BITMAP_STATE_ACTIVE, >> + LOG_BITMAP_STATE_COMPLETED); >> + return; >> + case LOG_BITMAP_STATE_CANCELING: >> + logging_state_set_status(b, LOG_BITMAP_STATE_CANCELING, >> + LOG_BITMAP_STATE_COMPLETED); >> + return; >> + case LOG_BITMAP_STATE_ERROR: >> + logging_state_set_status(b, LOG_BITMAP_STATE_ERROR, >> + LOG_BITMAP_STATE_COMPLETED); >> + } >> + return; >> +} > > I didn't really see the point of this at first, but I guess > it always moves to 'COMPLETED' unless you're already at completed > or in NONE; but then perhaps: > > int s = b->state; > switch (s) { > case LOG_BITMAP_STATE_ACTIVE: > case LOG_BITMAP_STATE_CANCELING: > case LOG_BITMAP_STATE_ERROR: > logging_state_set_status(b, s, > LOG_BITMAP_STATE_COMPLETED); > return; > } > return; > > would be more obvious (note I only read the state once) > Yup, will save some code and reduce confusion. -- ----- Sanidhya Kashyap