From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCBgT-0000nw-7t for qemu-devel@nongnu.org; Tue, 29 Jul 2014 13:59:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCBgK-00067i-5P for qemu-devel@nongnu.org; Tue, 29 Jul 2014 13:59:33 -0400 Received: from mail-pd0-x22d.google.com ([2607:f8b0:400e:c02::22d]:45756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCBgJ-00067B-Qr for qemu-devel@nongnu.org; Tue, 29 Jul 2014 13:59:24 -0400 Received: by mail-pd0-f173.google.com with SMTP id w10so18006pde.18 for ; Tue, 29 Jul 2014 10:59:22 -0700 (PDT) Message-ID: <53D7E0F0.8050908@gmail.com> Date: Tue, 29 Jul 2014 23:29:12 +0530 From: Sanidhya Kashyap MIME-Version: 1.0 References: <1406302776-2306-1-git-send-email-sanidhya.iiith@gmail.com> <1406302776-2306-6-git-send-email-sanidhya.iiith@gmail.com> <87d2cothad.fsf@troll.troll> In-Reply-To: <87d2cothad.fsf@troll.troll> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com Cc: qemu list , "Dr. David Alan Gilbert" >> +{ '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? > Okay. Will remove it. > >> +#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? > > yup, tracepoints do make sense. Will incorporate that. > We need a better preffix that test_vmstates_ > But I can't think of one right now. Will think later about it. > > I am really bad with naming conventions :-/. Whatever seems fit to you. I'll use that. >> +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? > My mistake. Will correct it. >> +{ >> + 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? > Actually, I have used this variable which will help us in deciding whether the user has provided a sane list of vmstates name's or not. If the names do not match, then we do not proceed. That is why I have used the device_present variable. I'll change the variable name. >> + 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. > Well, they are not. Currently, when qemu_system_reset() is called the mc->reset is NULL. So, the old way of resetting the device takes place which has some different devices or might be bus registered. Therefore, I have tried to provide whatever is there on the table i.e related to the resetting. But, that is not required, I'll completely remove it. >> + 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? > > If I perform the cleanup at the end, then I will be eating the memory. When I close the buffer, at least that memory is freed. The other issue will be taking the read pointer back to the write pointer, of which I don't know whether there is a support or not. >> + >> + 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", ...) > > Yup, I have missed qemu_fclose(f) before f = qemu_bufopen("r", ..); I'll correct it. Now, regarding the qemu_buf_set_ro and qemu_buf_set_rw, I guess, I'll have to rewind the pointer, for which I have to get some idea before doing it, or extend the QEMUFile code for memory buffer. -- ----- Sanidhya Kashyap