Sanidhya Kashyap <sanidhya.ii...@gmail.com> 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 <sanidhya.ii...@gmail.com>
> +##
> +# @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.
