Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Juan Quintela <quintela@redhat.com>]

Sanidhya Kashyap <sanidhya.iiith@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.iiith@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 &current_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.