Reorder the structures so we can know if the fields are:
- Read only
- Their own locking (i.e. sems)
- Protected by 'mutex'
- Only for the multifd channel
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 86 +++++++++++++++++++++++++++------------------
1 file changed, 51 insertions(+), 35 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index 7d0effcb03..f1f88c6737 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -65,7 +65,9 @@ typedef struct {
} MultiFDPages_t;
typedef struct {
- /* this fields are not changed once the thread is created */
+ /* Fiields are only written at creating/deletion time */
+ /* No lock required for them, they are read only */
+
/* channel number */
uint8_t id;
/* channel thread name */
@@ -74,37 +76,45 @@ typedef struct {
QemuThread thread;
/* communication channel */
QIOChannel *c;
- /* sem where to wait for more work */
- QemuSemaphore sem;
- /* this mutex protects the following parameters */
- QemuMutex mutex;
- /* is this channel thread running */
- bool running;
- /* should this thread finish */
- bool quit;
/* is the yank function registered */
bool registered_yank;
+ /* packet allocated len */
+ uint32_t packet_len;
+
+ /* sem where to wait for more work */
+ QemuSemaphore sem;
+ /* syncs main thread and channels */
+ QemuSemaphore sem_sync;
+
+ /* this mutex protects the following parameters */
+ QemuMutex mutex;
+ /* is this channel thread running */
+ bool running;
+ /* should this thread finish */
+ bool quit;
+ /* multifd flags for each packet */
+ uint32_t flags;
+ /* global number of generated multifd packets */
+ uint64_t packet_num;
/* thread has work to do */
int pending_job;
- /* array of pages to sent */
+ /* array of pages to sent.
+ * The owner of 'pages' depends of 'pending_job' value:
+ * pending_job == 0 -> migration_thread can use it.
+ * pending_job != 0 -> multifd_channel can use it.
+ */
MultiFDPages_t *pages;
- /* packet allocated len */
- uint32_t packet_len;
+
+ /* thread local variables. No locking required */
+
/* pointer to the packet */
MultiFDPacket_t *packet;
- /* multifd flags for each packet */
- uint32_t flags;
/* size of the next packet that contains pages */
uint32_t next_packet_size;
- /* global number of generated multifd packets */
- uint64_t packet_num;
- /* thread local variables */
/* packets sent through this channel */
uint64_t num_packets;
/* non zero pages sent through this channel */
uint64_t total_normal_pages;
- /* syncs main thread and channels */
- QemuSemaphore sem_sync;
/* buffers to send */
struct iovec *iov;
/* number of iovs used */
@@ -118,7 +128,9 @@ typedef struct {
} MultiFDSendParams;
typedef struct {
- /* this fields are not changed once the thread is created */
+ /* Fiields are only written at creating/deletion time */
+ /* No lock required for them, they are read only */
+
/* channel number */
uint8_t id;
/* channel thread name */
@@ -127,31 +139,35 @@ typedef struct {
QemuThread thread;
/* communication channel */
QIOChannel *c;
+ /* packet allocated len */
+ uint32_t packet_len;
+
+ /* syncs main thread and channels */
+ QemuSemaphore sem_sync;
+
/* this mutex protects the following parameters */
QemuMutex mutex;
/* is this channel thread running */
bool running;
/* should this thread finish */
bool quit;
+ /* multifd flags for each packet */
+ uint32_t flags;
+ /* global number of generated multifd packets */
+ uint64_t packet_num;
+
+ /* thread local variables. No locking required */
+
+ /* pointer to the packet */
+ MultiFDPacket_t *packet;
+ /* size of the next packet that contains pages */
+ uint32_t next_packet_size;
+ /* packets sent through this channel */
+ uint64_t num_packets;
/* ramblock host address */
uint8_t *host;
- /* packet allocated len */
- uint32_t packet_len;
- /* pointer to the packet */
- MultiFDPacket_t *packet;
- /* multifd flags for each packet */
- uint32_t flags;
- /* global number of generated multifd packets */
- uint64_t packet_num;
- /* thread local variables */
- /* size of the next packet that contains pages */
- uint32_t next_packet_size;
- /* packets sent through this channel */
- uint64_t num_packets;
/* non zero pages recv through this channel */
uint64_t total_normal_pages;
- /* syncs main thread and channels */
- QemuSemaphore sem_sync;
/* buffers to recv */
struct iovec *iov;
/* Pages that are not zero */
--
2.35.1* Juan Quintela (quintela@redhat.com) wrote:
> Reorder the structures so we can know if the fields are:
> - Read only
> - Their own locking (i.e. sems)
> - Protected by 'mutex'
> - Only for the multifd channel
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd.h | 86 +++++++++++++++++++++++++++------------------
> 1 file changed, 51 insertions(+), 35 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 7d0effcb03..f1f88c6737 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -65,7 +65,9 @@ typedef struct {
> } MultiFDPages_t;
>
> typedef struct {
> - /* this fields are not changed once the thread is created */
> + /* Fiields are only written at creating/deletion time */
> + /* No lock required for them, they are read only */
> +
> /* channel number */
> uint8_t id;
> /* channel thread name */
> @@ -74,37 +76,45 @@ typedef struct {
> QemuThread thread;
> /* communication channel */
> QIOChannel *c;
> - /* sem where to wait for more work */
> - QemuSemaphore sem;
> - /* this mutex protects the following parameters */
> - QemuMutex mutex;
> - /* is this channel thread running */
> - bool running;
> - /* should this thread finish */
> - bool quit;
> /* is the yank function registered */
> bool registered_yank;
> + /* packet allocated len */
> + uint32_t packet_len;
> +
> + /* sem where to wait for more work */
> + QemuSemaphore sem;
> + /* syncs main thread and channels */
> + QemuSemaphore sem_sync;
> +
> + /* this mutex protects the following parameters */
> + QemuMutex mutex;
> + /* is this channel thread running */
> + bool running;
> + /* should this thread finish */
> + bool quit;
> + /* multifd flags for each packet */
> + uint32_t flags;
> + /* global number of generated multifd packets */
> + uint64_t packet_num;
Is there a way to explain why packet_num, being global, is inside
SendParams? I understand why num_packets is - because
that's per channel; so why is a global isnide the params
(and having two things with almost the same name is very confusing!)
Dave
> /* thread has work to do */
> int pending_job;
> - /* array of pages to sent */
> + /* array of pages to sent.
> + * The owner of 'pages' depends of 'pending_job' value:
> + * pending_job == 0 -> migration_thread can use it.
> + * pending_job != 0 -> multifd_channel can use it.
> + */
> MultiFDPages_t *pages;
> - /* packet allocated len */
> - uint32_t packet_len;
> +
> + /* thread local variables. No locking required */
> +
> /* pointer to the packet */
> MultiFDPacket_t *packet;
> - /* multifd flags for each packet */
> - uint32_t flags;
> /* size of the next packet that contains pages */
> uint32_t next_packet_size;
> - /* global number of generated multifd packets */
> - uint64_t packet_num;
> - /* thread local variables */
> /* packets sent through this channel */
> uint64_t num_packets;
> /* non zero pages sent through this channel */
> uint64_t total_normal_pages;
> - /* syncs main thread and channels */
> - QemuSemaphore sem_sync;
> /* buffers to send */
> struct iovec *iov;
> /* number of iovs used */
> @@ -118,7 +128,9 @@ typedef struct {
> } MultiFDSendParams;
>
> typedef struct {
> - /* this fields are not changed once the thread is created */
> + /* Fiields are only written at creating/deletion time */
> + /* No lock required for them, they are read only */
> +
> /* channel number */
> uint8_t id;
> /* channel thread name */
> @@ -127,31 +139,35 @@ typedef struct {
> QemuThread thread;
> /* communication channel */
> QIOChannel *c;
> + /* packet allocated len */
> + uint32_t packet_len;
> +
> + /* syncs main thread and channels */
> + QemuSemaphore sem_sync;
> +
> /* this mutex protects the following parameters */
> QemuMutex mutex;
> /* is this channel thread running */
> bool running;
> /* should this thread finish */
> bool quit;
> + /* multifd flags for each packet */
> + uint32_t flags;
> + /* global number of generated multifd packets */
> + uint64_t packet_num;
> +
> + /* thread local variables. No locking required */
> +
> + /* pointer to the packet */
> + MultiFDPacket_t *packet;
> + /* size of the next packet that contains pages */
> + uint32_t next_packet_size;
> + /* packets sent through this channel */
> + uint64_t num_packets;
> /* ramblock host address */
> uint8_t *host;
> - /* packet allocated len */
> - uint32_t packet_len;
> - /* pointer to the packet */
> - MultiFDPacket_t *packet;
> - /* multifd flags for each packet */
> - uint32_t flags;
> - /* global number of generated multifd packets */
> - uint64_t packet_num;
> - /* thread local variables */
> - /* size of the next packet that contains pages */
> - uint32_t next_packet_size;
> - /* packets sent through this channel */
> - uint64_t num_packets;
> /* non zero pages recv through this channel */
> uint64_t total_normal_pages;
> - /* syncs main thread and channels */
> - QemuSemaphore sem_sync;
> /* buffers to recv */
> struct iovec *iov;
> /* Pages that are not zero */
> --
> 2.35.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Reorder the structures so we can know if the fields are:
>> - Read only
>> - Their own locking (i.e. sems)
>> - Protected by 'mutex'
>> - Only for the multifd channel
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/multifd.h | 86 +++++++++++++++++++++++++++------------------
>> 1 file changed, 51 insertions(+), 35 deletions(-)
>>
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 7d0effcb03..f1f88c6737 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -65,7 +65,9 @@ typedef struct {
>> } MultiFDPages_t;
>>
>> typedef struct {
>> - /* this fields are not changed once the thread is created */
>> + /* Fiields are only written at creating/deletion time */
>> + /* No lock required for them, they are read only */
>> +
>> /* channel number */
>> uint8_t id;
>> /* channel thread name */
>> @@ -74,37 +76,45 @@ typedef struct {
>> QemuThread thread;
>> /* communication channel */
>> QIOChannel *c;
>> - /* sem where to wait for more work */
>> - QemuSemaphore sem;
>> - /* this mutex protects the following parameters */
>> - QemuMutex mutex;
>> - /* is this channel thread running */
>> - bool running;
>> - /* should this thread finish */
>> - bool quit;
>> /* is the yank function registered */
>> bool registered_yank;
>> + /* packet allocated len */
>> + uint32_t packet_len;
>> +
>> + /* sem where to wait for more work */
>> + QemuSemaphore sem;
>> + /* syncs main thread and channels */
>> + QemuSemaphore sem_sync;
>> +
>> + /* this mutex protects the following parameters */
>> + QemuMutex mutex;
>> + /* is this channel thread running */
>> + bool running;
>> + /* should this thread finish */
>> + bool quit;
>> + /* multifd flags for each packet */
>> + uint32_t flags;
>> + /* global number of generated multifd packets */
>> + uint64_t packet_num;
>
> Is there a way to explain why packet_num, being global, is inside
> SendParams? I understand why num_packets is - because
> that's per channel; so why is a global isnide the params
> (and having two things with almost the same name is very confusing!)
Ok, I will try to improve the documentation (it was already there).
Each packet that we sent (independently of what channel we sent it
through) has a packet number, that is unique for all channels (i.e. not
only for a single channel). The number is assigned in
multifd_send_pages(), and the "global" value is stored in
multifd_send_state.
This field is _where_ we "transport" it to the real packet.
We have that field in:
- multifd_send_state, where we copy the current value to
- Multifd_send_params, where we copy that value to
- Multifd_packet.
Notice that the only place where we change the value is
multifd_send_state, once that we put a value on the multifd_send_params,
it is a constant until the next packet.
So how about:
/* assigned global packet number for this packet */
??
I am open to better names.
Later, Juan.
> Dave
>
>> /* thread has work to do */
>> int pending_job;
>> - /* array of pages to sent */
>> + /* array of pages to sent.
>> + * The owner of 'pages' depends of 'pending_job' value:
>> + * pending_job == 0 -> migration_thread can use it.
>> + * pending_job != 0 -> multifd_channel can use it.
>> + */
>> MultiFDPages_t *pages;
>> - /* packet allocated len */
>> - uint32_t packet_len;
>> +
>> + /* thread local variables. No locking required */
>> +
>> /* pointer to the packet */
>> MultiFDPacket_t *packet;
>> - /* multifd flags for each packet */
>> - uint32_t flags;
>> /* size of the next packet that contains pages */
>> uint32_t next_packet_size;
>> - /* global number of generated multifd packets */
>> - uint64_t packet_num;
>> - /* thread local variables */
>> /* packets sent through this channel */
>> uint64_t num_packets;
>> /* non zero pages sent through this channel */
>> uint64_t total_normal_pages;
>> - /* syncs main thread and channels */
>> - QemuSemaphore sem_sync;
>> /* buffers to send */
>> struct iovec *iov;
>> /* number of iovs used */
>> @@ -118,7 +128,9 @@ typedef struct {
>> } MultiFDSendParams;
>>
>> typedef struct {
>> - /* this fields are not changed once the thread is created */
>> + /* Fiields are only written at creating/deletion time */
>> + /* No lock required for them, they are read only */
>> +
>> /* channel number */
>> uint8_t id;
>> /* channel thread name */
>> @@ -127,31 +139,35 @@ typedef struct {
>> QemuThread thread;
>> /* communication channel */
>> QIOChannel *c;
>> + /* packet allocated len */
>> + uint32_t packet_len;
>> +
>> + /* syncs main thread and channels */
>> + QemuSemaphore sem_sync;
>> +
>> /* this mutex protects the following parameters */
>> QemuMutex mutex;
>> /* is this channel thread running */
>> bool running;
>> /* should this thread finish */
>> bool quit;
>> + /* multifd flags for each packet */
>> + uint32_t flags;
>> + /* global number of generated multifd packets */
>> + uint64_t packet_num;
>> +
>> + /* thread local variables. No locking required */
>> +
>> + /* pointer to the packet */
>> + MultiFDPacket_t *packet;
>> + /* size of the next packet that contains pages */
>> + uint32_t next_packet_size;
>> + /* packets sent through this channel */
>> + uint64_t num_packets;
>> /* ramblock host address */
>> uint8_t *host;
>> - /* packet allocated len */
>> - uint32_t packet_len;
>> - /* pointer to the packet */
>> - MultiFDPacket_t *packet;
>> - /* multifd flags for each packet */
>> - uint32_t flags;
>> - /* global number of generated multifd packets */
>> - uint64_t packet_num;
>> - /* thread local variables */
>> - /* size of the next packet that contains pages */
>> - uint32_t next_packet_size;
>> - /* packets sent through this channel */
>> - uint64_t num_packets;
>> /* non zero pages recv through this channel */
>> uint64_t total_normal_pages;
>> - /* syncs main thread and channels */
>> - QemuSemaphore sem_sync;
>> /* buffers to recv */
>> struct iovec *iov;
>> /* Pages that are not zero */
>> --
>> 2.35.1
>>
© 2016 - 2026 Red Hat, Inc.