[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH RDMA support v6: 4/7] Introduce two new capa

From: Michael R. Hines
Subject: Re: [Qemu-devel] [RFC PATCH RDMA support v6: 4/7] Introduce two new capabilities
Date: Wed, 10 Apr 2013 08:34:42 -0400
User-agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130106 Thunderbird/17.0.2

On 04/10/2013 03:50 AM, Paolo Bonzini wrote:
Il 10/04/2013 06:29, address@hidden ha scritto:
From: "Michael R. Hines" <address@hidden>

RDMA performs very slowly with zero-page checking.
Without the ability to disable it, RDMA throughput and
latency promises and high performance links cannot be
fully realized.

On the other hand, dynamic page registration support is also
included in the RDMA protocol. This second capability also
cannot be fully realized without the ability to enable zero
page scanning.

So, we have two new capabilities which work together:

1. migrate_set_capability check_for_zero on|off (default on)
2. migrate_set_capability chunk_register_destination on|off (default off)

Signed-off-by: Michael R. Hines <address@hidden>
Michael, please listen to _all_ review comments.

1) I asked you to place check_for_zero in a separate patch.

2) Again, patch 3 cannot compile without this one.  The code should
compile after each patch, with and without --enable-rdma.

My apologies - I misunderstood the request. I am indeed addressing every comment.

When you said separate, I thought you meant a different patch series altpgether.

This is my first time, so the meaning of "separate" was not clear =)

And yes, patch 3 does in fact compile both with and without --enable-rdma.
  include/migration/qemu-file.h |   15 +++++++++++++++
  migration.c                   |   33 +++++++++++++++++++++++++++++++--
  qapi-schema.json              |    2 +-
  3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 623c434..b6f3256 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -57,12 +57,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
  typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
                                             int iovcnt);
+typedef struct QEMURamControlOps QEMURamControlOps;
  typedef struct QEMUFileOps {
      QEMUFilePutBufferFunc *put_buffer;
      QEMUFileGetBufferFunc *get_buffer;
      QEMUFileCloseFunc *close;
      QEMUFileGetFD *get_fd;
      QEMUFileWritevBufferFunc *writev_buffer;
+    const QEMURamControlOps *ram_control;
Why a separate member?  You can put them directly here.


  } QEMUFileOps;
QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
@@ -80,6 +83,18 @@ void qemu_put_byte(QEMUFile *f, int v);
   * The buffer should be available till it is sent asynchronously.
  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size);
+void qemu_file_set_error(QEMUFile *f, int ret);
+void qemu_rdma_cleanup(void *opaque);
+int qemu_rdma_close(void *opaque);
+int qemu_rdma_get_fd(void *opaque);
+int qemu_rdma_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size);
+int qemu_rdma_put_buffer(void *opaque, const uint8_t *buf,
+                            int64_t pos, int size);
+bool qemu_file_mode_is_not_valid(const char * mode);
+extern const QEMUFileOps rdma_read_ops;
+extern const QEMUFileOps rdma_write_ops;
Please put these in migration-rdma.c.  You do not need them here.


  static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
diff --git a/migration.c b/migration.c
index 3b4b467..875cee3 100644
--- a/migration.c
+++ b/migration.c
@@ -66,6 +66,7 @@ MigrationState *migrate_get_current(void)
          .state = MIG_STATE_SETUP,
          .bandwidth_limit = MAX_THROTTLE,
          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
+        .enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true,
return &current_migration;
@@ -77,6 +78,10 @@ void qemu_start_incoming_migration(const char *uri, Error 
if (strstart(uri, "tcp:", &p))
          tcp_start_incoming_migration(p, errp);
+    else if (strstart(uri, "rdma:", &p))
+        rdma_start_incoming_migration(p, errp);
  #if !defined(WIN32)
      else if (strstart(uri, "exec:", &p))
          exec_start_incoming_migration(p, errp);
@@ -120,8 +125,10 @@ void process_incoming_migration(QEMUFile *f)
      Coroutine *co = qemu_coroutine_create(process_incoming_migration_co);
      int fd = qemu_get_fd(f);
- assert(fd != -1);
-    qemu_set_nonblock(fd);
+    if(fd != -2) { /* rdma returns -2 */
Please do not invent a special return value, this will not be maintainable.

Just sacrifice the assert for now, as we had agreed in the past.

You flip-flopped on me =)
You said conditionalize it, then make a separate patch, then delete it altogether =)

But I'm fine with delete =)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]