Re: [Qemu-devel] [PATCH v4 3/8] BitmapLog: bitmap dump code via QAPI framework with runstates

[Dr. David Alan Gilbert]

* Sanidhya Kashyap (address@hidden) wrote:
> Reformatted the code and added the functionality of dumping the blocks' info
> which can be read by the user when required. I have also made the block name
> length global.
> Some minor modification to the structure which is now storing all the
> information.

One thought, I wonder how much of the savevm.c changes could move into
a separate file rather than making savevm even bigger?

> 
> Signed-off-by: Sanidhya Kashyap <address@hidden>
> ---
>  include/exec/cpu-all.h |   4 +-
>  migration.c            |   7 ++
>  qapi-schema.json       |  18 +++
>  qmp-commands.hx        |  30 +++++
>  savevm.c               | 325 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 383 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index f91581f..b459301 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -297,13 +297,15 @@ CPUArchState *cpu_copy(CPUArchState *env);
>  
>  /* memory API */
>  
> +#define RAMBLOCK_NAME_LENGTH (1<<8)

Be careful; making this bigger would break migration formats,
making it smaller would probably break migration loading.

>  typedef struct RAMBlock {
>      struct MemoryRegion *mr;
>      uint8_t *host;
>      ram_addr_t offset;
>      ram_addr_t length;
>      uint32_t flags;
> -    char idstr[256];
> +    char idstr[RAMBLOCK_NAME_LENGTH];
>      /* Reads can take either the iothread or the ramlist lock.
>       * Writes must take both locks.
>       */
> diff --git a/migration.c b/migration.c
> index 8d675b3..e2e313c 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -436,6 +436,13 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> +    if (runstate_check(RUN_STATE_DUMP_BITMAP)) {
> +        error_setg(errp, "bitmap dump in progress");
> +        return;
> +    }
> +
> +    runstate_set(RUN_STATE_MIGRATE);
> +
>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 501b8d0..924d6bc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3485,3 +3485,21 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +##
> +# @log-dirty-bitmap
> +#
> +# dumps the dirty bitmap to a file by logging the
> +# memory for a specified number of times with a
> +# a defined time differnce
> +#
> +# @filename: name of the file in which the bitmap will be saved.
> +# @epochs: number of times the memory will be logged (optional).
> +# @frequency: time difference in milliseconds between each epoch (optional).
> +#
> +# Since 2.2
> +##
> +{ 'command' : 'log-dirty-bitmap',
> +  'data'    : { 'filename'      : 'str',
> +                '*epochs'       : 'int',
> +                '*frequency'    : 'int' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4be4765..200f57e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3753,5 +3753,35 @@ Example:
>  
>  -> { "execute": "rtc-reset-reinjection" }
>  <- { "return": {} }
> +EQMP
> +
> +    {
> +        .name       = "log-dirty-bitmap",
> +        .args_type  = "filename:s,epochs:i?,frequency:i?,readable:-r?",
> +        .mhandler.cmd_new = qmp_marshal_input_log_dirty_bitmap,
> +    },
> +
> +SQMP
> +log-dirty-bitmap
> +--------------------
> +
> +start logging the memory of the VM for writable working set
> +
> +Arguments:
> +
> +- "filename": name of the file, in which the bitmap will be saved
> +- "epochs": number of times, the memory will be logged
> +- "frequency": time difference in milliseconds between each epoch
> +
> +Examples:
> +-> { "execute" : "log-dirty-bitmap",
> +     "arguments" : {
> +         "filename" : "/tmp/fileXXX",
> +         "epochs" : 3,
> +         "frequency" : 10 } }
> +
> +<- { "return": {} }
>  
> +Note: The epochs, frequency and readable are optional. epochs default
> +value is 3 while that of frequency is 10.
>  EQMP
> diff --git a/savevm.c b/savevm.c
> index e19ae0a..ecb334e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -42,6 +42,9 @@
>  #include "qemu/iov.h"
>  #include "block/snapshot.h"
>  #include "block/qapi.h"
> +#include "exec/address-spaces.h"
> +#include "exec/ram_addr.h"
> +#include "qemu/bitmap.h"
>  
>  
>  #ifndef ETH_P_RARP
> @@ -1137,6 +1140,328 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +/*
> + * Adding the functionality of continuous logging of the
> + * dirty bitmap which is almost similar to the migration
> + * thread
> + */
> +
> +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).

> +
> +typedef struct BitmapLogState BitmapLogState;
> +static unsigned long *logging_bitmap;
> +static int64_t MIN_EPOCH_VALUE = 3;
> +static int64_t MIN_FREQUENCY_VALUE = 10;
> +static int64_t MAX_EPOCH_VALUE = 100000;
> +static int64_t MAX_FREQUENCY_VALUE = 100000;
> +
> +struct BitmapLogState {
> +    int state;
> +    int fd;
> +    int64_t current_frequency;
> +    int64_t current_epoch;
> +    int64_t total_epochs;
> +    QemuThread thread;
> +};
> +
> +/*
> + * helper functions
> + */
> +
> +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.

> +
> +static inline void logging_unlock(void)
> +{
> +    qemu_mutex_unlock_ramlist();
> +    qemu_mutex_unlock_iothread();
> +}
> +
> +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).

> +    set_bit(nr, logging_bitmap);
> +}
> +
> +static bool logging_state_set_status(BitmapLogState *b,
> +                                     int old_state,
> +                                     int new_state)
> +{
> +    return atomic_cmpxchg(&b->state, old_state, new_state);
> +}
> +
> +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?

> +
> +/*
> + * inspired from migration mechanism
> + */
> +
> +static BitmapLogState *logging_current_state(void)
> +{
> +    static BitmapLogState current_bitmaplogstate = {
> +        .state = LOG_BITMAP_STATE_NONE,
> +    };
> +
> +    return &current_bitmaplogstate;
> +}
> +
> +/*
> + * syncing the logging_bitmap with the ram_list dirty bitmap
> + */
> +
> +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.

> +
> +static inline void logging_bitmap_close(BitmapLogState *b)
> +{
> +    logging_lock();
> +    memory_global_dirty_log_stop();
> +    logging_unlock();
> +
> +    g_free(logging_bitmap);
> +    logging_bitmap = NULL;
> +    qemu_close(b->fd);
> +    b->fd = -1;
> +}
> +
> +static bool ram_block_info_dump(int fd)
> +{
> +    int64_t ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    int block_count = 0;
> +    RAMBlock *block;
> +    int ret;
> +
> +    if (qemu_write_full(fd, &ram_bitmap_pages, sizeof(int64_t)) < 0) {
> +        return true;
> +    }
> +
> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> +        block_count++;
> +    }
> +
> +    ret = qemu_write_full(fd, &block_count, sizeof(int));
> +    if (ret < sizeof(int)) {
> +        return true;
> +    }
> +
> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> +        ret = qemu_write_full(fd, &(block->idstr), sizeof(char) *
> +                                                   RAMBLOCK_NAME_LENGTH);
> +        if (ret < sizeof(char) * RAMBLOCK_NAME_LENGTH) {
> +            return true;
> +        }
> +        ret = qemu_write_full(fd, &(block->offset), sizeof(ram_addr_t));
> +        if (ret < sizeof(ram_addr_t)) {
> +            return true;
> +        }
> +        ret = qemu_write_full(fd, &(block->length), sizeof(ram_addr_t));
> +        if (ret < sizeof(ram_addr_t)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +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)

Dave

> +static void *bitmap_logging_thread(void *opaque)
> +{
> +    /*
> +     * setup basic structures
> +     */
> +
> +    BitmapLogState *b = opaque;
> +    int fd = b->fd;
> +    b->current_epoch = 1;
> +    int64_t ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    size_t bitmap_size = BITS_TO_LONGS(ram_bitmap_pages) *
> +                         sizeof(unsigned long);
> +    int ret;
> +    char marker = 'M';
> +
> +    logging_state_set_status(b, LOG_BITMAP_STATE_NONE,
> +                                LOG_BITMAP_STATE_SETUP);
> +
> +    logging_bitmap = bitmap_new(ram_bitmap_pages);
> +
> +    if (logging_bitmap == NULL) {
> +        b->state = LOG_BITMAP_STATE_ERROR;
> +        goto log_thread_end;
> +    }
> +
> +    logging_state_set_status(b, LOG_BITMAP_STATE_SETUP,
> +                                LOG_BITMAP_STATE_ACTIVE);
> +    /*
> +     *  start the logging period
> +     */
> +    logging_lock();
> +    memory_global_dirty_log_start();
> +    dirty_bitmap_sync();
> +    bitmap_zero(logging_bitmap, ram_bitmap_pages);
> +    logging_unlock();
> +
> +    if (ram_block_info_dump(fd)) {
> +        b->state = LOG_BITMAP_STATE_ERROR;
> +        goto log_thread_end;
> +    }
> +
> +    /*
> +     * sync the dirty bitmap along with saving it
> +     * using the FILE pointer f.
> +     */
> +    while (b->current_epoch <= b->total_epochs) {
> +        if (!runstate_check(RUN_STATE_DUMP_BITMAP) ||
> +            b->state != LOG_BITMAP_STATE_ACTIVE) {
> +            goto log_thread_end;
> +        }
> +        bitmap_zero(logging_bitmap, ram_bitmap_pages);
> +        logging_lock();
> +        dirty_bitmap_sync();
> +        logging_unlock();
> +
> +        ret = qemu_write_full(fd, logging_bitmap, bitmap_size);
> +        if (ret < bitmap_size) {
> +            b->state = LOG_BITMAP_STATE_ERROR;
> +            goto log_thread_end;
> +        }
> +
> +        ret = qemu_write_full(fd, &marker, sizeof(char));
> +        if (ret < sizeof(char)) {
> +            b->state = LOG_BITMAP_STATE_ERROR;
> +            goto log_thread_end;
> +        }
> +        g_usleep(b->current_frequency * 1000);
> +        b->current_epoch++;
> +    }
> +
> +    /*
> +     * stop the logging period.
> +     */
> + log_thread_end:
> +    logging_bitmap_close(b);
> +    logging_state_update_status(b);
> +    runstate_set(RUN_STATE_RUNNING);
> +    return NULL;
> +}
> +
> +void qmp_log_dirty_bitmap(const char *filename, bool has_epochs,
> +                          int64_t epochs, bool has_frequency,
> +                          int64_t frequency, Error **errp)
> +{
> +    int fd = -1;
> +    BitmapLogState *b = logging_current_state();
> +    Error *local_err = NULL;
> +    if (runstate_check(RUN_STATE_DUMP_BITMAP) ||
> +            b->state == LOG_BITMAP_STATE_ACTIVE ||
> +            b->state == LOG_BITMAP_STATE_SETUP ||
> +            b->state == LOG_BITMAP_STATE_CANCELING) {
> +        error_setg(errp, "dirty bitmap dump in progress");
> +        return;
> +    }
> +
> +    if (!runstate_is_running()) {
> +        error_setg(errp, "Guest is not in a running state");
> +        return;
> +    }
> +
> +    runstate_set(RUN_STATE_DUMP_BITMAP);
> +    b->state = LOG_BITMAP_STATE_NONE;
> +
> +    /*
> +     * checking the epoch range
> +     */
> +    if (!has_epochs) {
> +        b->total_epochs = MIN_EPOCH_VALUE;
> +    } else if (!value_in_range(epochs, MIN_EPOCH_VALUE,
> +                               MAX_EPOCH_VALUE, "epoch", &local_err)) {
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +        }
> +        runstate_set(RUN_STATE_RUNNING);
> +        return;
> +    } else {
> +        b->total_epochs = epochs;
> +    }
> +
> +    /*
> +     * checking the frequency range
> +     */
> +    if (!has_frequency) {
> +        b->current_frequency = MIN_FREQUENCY_VALUE;
> +    } else if (!value_in_range(frequency, MIN_FREQUENCY_VALUE,
> +                               MAX_FREQUENCY_VALUE, "frequency", 
> &local_err)) {
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +        }
> +        runstate_set(RUN_STATE_RUNNING);
> +        return;
> +    }  else {
> +        b->current_frequency = frequency;
> +    }
> +
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 
> S_IRUSR);
> +    if (fd < 0) {
> +        error_setg_file_open(errp, errno, filename);
> +        runstate_set(RUN_STATE_RUNNING);
> +        return;
> +    }
> +
> +    b->fd = fd;
> +    qemu_thread_create(&b->thread, "dirty-bitmap-dump",
> +                       bitmap_logging_thread, b,
> +                       QEMU_THREAD_JOINABLE);
> +
> +    return;
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>  {
>      QEMUFile *f;
> -- 
> 1.9.3
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK