From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XC7dh-0005Sm-Mi for qemu-devel@nongnu.org; Tue, 29 Jul 2014 09:40:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XC7da-0003ki-67 for qemu-devel@nongnu.org; Tue, 29 Jul 2014 09:40:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XC7dZ-0003kV-Tr for qemu-devel@nongnu.org; Tue, 29 Jul 2014 09:40:18 -0400 From: Juan Quintela In-Reply-To: <1406302776-2306-6-git-send-email-sanidhya.iiith@gmail.com> (Sanidhya Kashyap's message of "Fri, 25 Jul 2014 21:09:29 +0530") References: <1406302776-2306-1-git-send-email-sanidhya.iiith@gmail.com> <1406302776-2306-6-git-send-email-sanidhya.iiith@gmail.com> Date: Tue, 29 Jul 2014 15:40:10 +0200 Message-ID: <87d2cothad.fsf@troll.troll> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sanidhya Kashyap Cc: qemu list , "Dr. David Alan Gilbert" Sanidhya Kashyap wrote: > In this patch, I have made the following changes: > > * changed the DPRINT statement. > * renamed the variables. > * added noqdev variable which decides which option to use for resetting. > * added devices option which can help in resetting one or many devices > (only qdevified ones). > * updated the documentation. > > Signed-off-by: Sanidhya Kashyap > +## > +# @test-vmstates > +# > +# tests the vmstates' value by dumping and loading in memory > +# > +# @iterations: (optional) The total iterations for vmstate testing. > +# The min and max defind value is 10 and 100 respectively. > +# > +# @period: (optional) sleep interval between iteration (in milliseconds). > +# The default interval is 100 milliseconds with min and max being > +# 1 and 10000 respectively. > +# > +# @noqdev: boolean variable which decides whether to use qdevified devices > +# or not. Will be removed when all the devices have been qdevified. > +# > +# @devices: (optional) helps in resetting particular qdevified decices > +# that have been registered with SaveStateEntry > +# > +# Since 2.2 > +## > +{ 'command': 'test-vmstates', > + 'data': {'*iterations': 'int', > + '*period': 'int', > + 'noqdev': 'bool', Do we really care about "noqdev", or should we just "decree" that it is "false" always? > +#define DEBUG_TEST_VMSTATES 1 > + > +#ifndef DEBUG_TEST_VMSTATES > +#define DEBUG_TEST_VMSTATES 0 > +#endif If you have the previe line, this ones are not needed. > + > +#define DPRINTF(fmt, ...) \ > + do { \ > + if (DEBUG_TEST_VMSTATES) { \ > + printf("vmstate_test: " fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) DPRINTF is *so* last year O:-) It is considedered better to just add tracepoints to the code. I think that all the DPRINTF's make sense to be a tracepoint, no? > +struct VMStateLogState { > + int64_t current_iteration; > + int64_t iterations; > + int64_t period; > + bool active_state; > + bool noqdev; > + VMStatesQdevDevices *qdevices; > + QEMUTimer *timer; > + > + QTAILQ_HEAD(qdev_list, VMStatesQdevResetEntry) qdev_list; > +}; > + > +static VMStateLogState *vmstate_current_state(void) > +{ > + static VMStateLogState current_state = { > + .active_state = false, > + }; > + > + return ¤t_state; > +} > + > +static inline void test_vmstates_clear_qdev_entries(VMStateLogState *v) We need a better preffix that test_vmstates_ But I can't think of one right now. Will think later about it. > +static inline bool check_device_name(VMStateLogState *v, > + VMStatesQdevDevices *qdevices, > + Error **errp) Is "inline" needed? I would expect the compiler to do a reasonable decision with an static function called only once? > +{ > + VMStatesQdevResetEntry *qre; > + strList *devices_name = qdevices->device; > + QTAILQ_INIT(&v->qdev_list); > + bool device_present; > + > + /* now, checking against each one */ > + for (; devices_name; devices_name = devices_name->next) { > + device_present = false; > + VMStatesQdevResetEntry *new_qre; > + QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) { > + if (!strcmp(qre->device_name, devices_name->value)) { > + > + device_present = true; > + > + new_qre = g_malloc0(sizeof(VMStatesQdevResetEntry)); > + new_qre->dev = qre->dev; > + strcpy(new_qre->device_name, qre->device_name); > + QTAILQ_INSERT_TAIL(&v->qdev_list, new_qre, entry); > + > + break; return; And remove the whole "device_present" variable and assignation? > + } > + } > + if (!device_present) { > + test_vmstates_clear_qdev_entries(v); > + error_setg(errp, "Incorrect device name - %s\n", > + devices_name->value); > + return false; > + } > + } > + return true; > +} > + > +static inline void test_vmstates_reset_devices(VMStateLogState *v) > +{ > + VMStatesQdevResetEntry *qre; > + > + if (v->noqdev) { > + DPRINTF("resetting all devices\n"); > + qemu_system_reset(VMRESET_SILENT); What is diffe9rent between calling with "noqdev" and with an empty device list? I would expect them to be the same list of devices. > + } else if (!v->qdevices) { > + QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) { > + DPRINTF("resetting device: %s\n", qre->device_name); > + device_reset(qre->dev); > + } > + } else { > + QTAILQ_FOREACH(qre, &v->qdev_list, entry) { > + DPRINTF("resetting device: %s\n", qre->device_name); > + device_reset(qre->dev); > + } > + } > +} > + > +static void vmstate_test_cb(void *opaque) > +{ > + VMStateLogState *v = opaque; > + int saved_vm_running = runstate_is_running(); > + const QEMUSizedBuffer *qsb; > + QEMUFile *f; > + int ret; > + int64_t save_vmstates_duration, load_vmstates_duration; > + int64_t start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > + > + /* executing the steps for a single time with the help of timer */ > + if (++(v->current_iteration) <= v->iterations) { > + saved_vm_running = runstate_is_running(); > + > + /* stopping the VM before dumping the vmstates */ > + vm_stop(RUN_STATE_SAVE_VM); > + > + f = qemu_bufopen("w", NULL); > + if (!f) { > + goto testing_end; > + } I think we can call qemu_bufopen() out of the timer, and then doing the free on the cleanup? > + > + cpu_synchronize_all_states(); > + > + /* saving the vmsates to memory buffer */ > + ret = qemu_save_device_state(f); > + if (ret < 0) { > + goto testing_end; > + } > + save_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - > + start_time; > + DPRINTF("iteration: %ld, save time (ms): %ld\n", > + v->current_iteration, save_vmstates_duration); > + > + /* clearing the states of the guest */ > + test_vmstates_reset_devices(v); > + > + start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > + qsb = qemu_buf_get(f); > + f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb); We are only using this function once, can't we convince it to just be called "const"? ok what are we doing here: for(i=0; i< times; i++) { ..... f = qemu_bufopen("r", ..); ..... f = qemu_buf_get(f); f = qemu_bufopen("w", ..) ... qemu_fclose(f); } What I propose is switching to something like: f = qemu_bufopen("r", ..); for(i=0; i< times; i++) { .... qemu_buf_set_ro(f); ..... qemu_buf_set_rw(f) ... } qemu_fclose(f); This makes qemu_bufopen() way simpler. Once there, my understanding is that current code is leaking a QEMUBuffer each time that we call qemu_bufopen("w", ...) > + if (!has_period) { > + v->period = TEST_VMSTATE_DEFAULT_INTERVAL_MS; > + } else if (period >= TEST_VMSTATE_MIN_INTERVAL_MS && > + period <= TEST_VMSTATE_MAX_INTERVAL_MS) { > + v->period = period; > + } else { > + error_setg(errp, "sleep interval (period) value must be " > + "in the defined range [%d, %d](ms)\n", > + TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS); > + v->active_state = false; > + return; > + } I think this sholud be a macro and not being repeated by each numeric parameter, but that is a QMP API, not related to this patch. Thanks, Juan.