Message ID | 20220510224220.5912-2-quintela@redhat.com |
---|---|
State | New |
Headers | show |
Series | Migration: Transmit and detect zero pages in the multifd threads | expand |
* 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> 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 >>
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 */
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(-)