Re: [Qemu-devel] [PATCH 3/8] rdma: new QEMUFileOps hooks

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

All good catches, acknowledged.

On 04/12/2013 07:02 AM, Paolo Bonzini wrote:
> Il 12/04/2013 07:52, mrhines@linux.vnet.ibm.com ha scritto:
>> From: "Michael R. Hines" <mrhines@us.ibm.com>
>>
>>   These are just the prototypes for optional new hooks
>>   that RDMA takes advantage of to perform dynamic page
>>   registration.
>>
>> An optional hook is also introduced for a custom function
>> to be able to override the default save_page function.
>>
>> Also included are just the protytpes of the accessor methods used by
>> arch_init.c which invoke functions inside savevm.c to call
>> out to the hooks that may (or may not) have been overridden
>> inside of QEMUFileOps.
>>
>> The actual definitions for all of these come later in the patch series.
>>
>> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
>> ---
>>   include/migration/migration.h |   22 ++++++++++++++++++++++
>>   include/migration/qemu-file.h |   30 ++++++++++++++++++++++++++++++
>>   2 files changed, 52 insertions(+)
>>
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index e2acec6..5ab1d73 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -21,6 +21,7 @@
>>   #include "qapi/error.h"
>>   #include "migration/vmstate.h"
>>   #include "qapi-types.h"
>> +#include "exec/cpu-common.h"
>>   
>>   struct MigrationParams {
>>       bool blk;
>> @@ -75,6 +76,10 @@ void fd_start_incoming_migration(const char *path, Error **errp);
>>   
>>   void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
>>   
>> +void rdma_start_outgoing_migration(void *opaque, const char *host_port, Error **errp);
>> +
>> +void rdma_start_incoming_migration(const char *host_port, Error **errp);
>> +
> These two declarations should have been in patch 2.
>
>>   void migrate_fd_error(MigrationState *s);
>>   
>>   void migrate_fd_connect(MigrationState *s);
>> @@ -127,4 +132,21 @@ int migrate_use_xbzrle(void);
>>   int64_t migrate_xbzrle_cache_size(void);
>>   
>>   int64_t xbzrle_cache_resize(int64_t new_size);
>> +
>> +bool migrate_chunk_register_destination(void);
> This declaration must be in patch 5.
>
>> +void ram_control_before_iterate(QEMUFile *f, uint32_t flags);
>> +void ram_control_after_iterate(QEMUFile *f, uint32_t flags);
>> +void ram_control_load_hook(QEMUFile *f, uint32_t flags);
>> +
>> +/* Whenever this is found in the data stream, the flags
>> + * will be passed to ram_control_load_hook in the incoming-migration
>> + * side. This lets before_ram_iterate/after_ram_iterate add
>> + * transport-specific sections to the RAM migration data.
>> + */
>> +#define RAM_SAVE_FLAG_HOOK     0x80
>> +
>> +size_t ram_control_save_page(QEMUFile *f,
>> +                             ram_addr_t block_offset, ram_addr_t offset,
>> +                             size_t size, uint8_t *va);
>> +
>>   #endif
>> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
>> index 623c434..9d23ff3 100644
>> --- a/include/migration/qemu-file.h
>> +++ b/include/migration/qemu-file.h
>> @@ -23,6 +23,7 @@
>>    */
>>   #ifndef QEMU_FILE_H
>>   #define QEMU_FILE_H 1
>> +#include "exec/cpu-common.h"
>>   
>>   /* This function writes a chunk of data to a file at the given position.
>>    * The pos argument can be ignored if the file is only being used for
>> @@ -57,12 +58,39 @@ typedef int (QEMUFileGetFD)(void *opaque);
>>   typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
>>                                              int iovcnt);
>>   
>> +/*
>> + * This function provides hooks around different
>> + * stages of RAM migration.
>> + */
>> +typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint32_t flags);
>> +
>> +/*
>> + * Constants used by ram_control_* hooks
>> + */
>> +#define RAM_CONTROL_SETUP    0
>> +#define RAM_CONTROL_ROUND    1
>> +#define RAM_CONTROL_HOOK     2
>> +#define RAM_CONTROL_FINISH   3
>> +
>> +/*
>> + * This function allows override of where the RAM page
>> + * is saved (such as RDMA, for example.)
>> + */
>> +typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
>> +                               ram_addr_t block_offset,
>> +                               ram_addr_t offset,
>> +                               size_t size, uint8_t *va);
>> +
>>   typedef struct QEMUFileOps {
>>       QEMUFilePutBufferFunc *put_buffer;
>>       QEMUFileGetBufferFunc *get_buffer;
>>       QEMUFileCloseFunc *close;
>>       QEMUFileGetFD *get_fd;
>>       QEMUFileWritevBufferFunc *writev_buffer;
>> +    QEMURamHookFunc *before_ram_iterate;
>> +    QEMURamHookFunc *after_ram_iterate;
>> +    QEMURamHookFunc *hook_ram_load;
>> +    QEMURamSaveFunc *save_page;
> Without these, patch 2 cannot compile with --enable-rdma.
>
>
>>   } QEMUFileOps;
>>   
>>   QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
>> @@ -81,6 +109,8 @@ void qemu_put_byte(QEMUFile *f, int v);
>>    */
>>   void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size);
>>   
>> +bool qemu_file_mode_is_not_valid(const char *mode);
> This declaration must be in patch 4.
>
> Please reorder patches 2-3-4-5 as follows:
>
> - rdma: new QEMUFileOps hooks && rdma: implement new QEMUFileOps hooks
>    (squashed)
> - rdma: introduce capability for chunk registration (with fixed
>    declaration)
> - rdma: core rdma logic (with fixed declaration).
>
> Since you are at it, please add "x-" to both the capability and the
> transport.
>
> I'm a fan of releasing early, so this is my last request now.
>
> Paolo
>
>>   static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>>   {
>>       qemu_put_byte(f, (int)v);
>>
>