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 | 90 ++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 37 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 4d8d89e5e5..345cfdb50c 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,39 +76,47 @@ 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;
+    /* multifd flags for sending ram */
+    int write_flags;
+
+    /* 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 sending ram */
-    int write_flags;
-    /* 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 */
@@ -120,7 +130,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 */
@@ -129,31 +141,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.3

* 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 | 90 ++++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 4d8d89e5e5..345cfdb50c 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 */

Other than the typo's that Chen spotted;


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +    /* No lock required for them, they are read only */
> +
>      /* channel number */
>      uint8_t id;
>      /* channel thread name */
> @@ -74,39 +76,47 @@ 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;
> +    /* multifd flags for sending ram */
> +    int write_flags;
> +
> +    /* 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 sending ram */
> -    int write_flags;
> -    /* 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 */
> @@ -120,7 +130,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 */
> @@ -129,31 +141,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.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Juan Quintela
> Sent: Tuesday, May 31, 2022 6:43 PM
> To: qemu-devel@nongnu.org
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>; Philippe Mathieu-
> Daudé <f4bug@amsat.org>; Yanan Wang <wangyanan55@huawei.com>; Dr.
> David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela
> <quintela@redhat.com>; Eduardo Habkost <eduardo@habkost.net>; Peter
> Xu <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: [PATCH v7 01/13] multifd: Document the locking of
> MultiFD{Send/Recv}Params
> 
> 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 | 90 ++++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/migration/multifd.h b/migration/multifd.h index
> 4d8d89e5e5..345cfdb50c 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 */

S/Fiields/Fields

Thanks
Chen

> +    /* No lock required for them, they are read only */
> +
>      /* channel number */
>      uint8_t id;
>      /* channel thread name */
> @@ -74,39 +76,47 @@ 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;
> +    /* multifd flags for sending ram */
> +    int write_flags;
> +
> +    /* 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 sending ram */
> -    int write_flags;
> -    /* 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 */
> @@ -120,7 +130,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 */
> @@ -129,31 +141,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.3
> 

* Zhang, Chen (chen.zhang@intel.com) wrote:
> 
> 
> > -----Original Message-----
> > From: Qemu-devel <qemu-devel-
> > bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Juan Quintela
> > Sent: Tuesday, May 31, 2022 6:43 PM
> > To: qemu-devel@nongnu.org
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>; Philippe Mathieu-
> > Daudé <f4bug@amsat.org>; Yanan Wang <wangyanan55@huawei.com>; Dr.
> > David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela
> > <quintela@redhat.com>; Eduardo Habkost <eduardo@habkost.net>; Peter
> > Xu <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> > Subject: [PATCH v7 01/13] multifd: Document the locking of
> > MultiFD{Send/Recv}Params
> > 
> > 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 | 90 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 53 insertions(+), 37 deletions(-)
> > 
> > diff --git a/migration/multifd.h b/migration/multifd.h index
> > 4d8d89e5e5..345cfdb50c 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 */
> 
> S/Fiields/Fields

Thanks, I've fixed that up.

> Thanks
> Chen
> 
> > +    /* No lock required for them, they are read only */
> > +
> >      /* channel number */
> >      uint8_t id;
> >      /* channel thread name */
> > @@ -74,39 +76,47 @@ 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;
> > +    /* multifd flags for sending ram */
> > +    int write_flags;
> > +
> > +    /* 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 sending ram */
> > -    int write_flags;
> > -    /* 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 */
> > @@ -120,7 +130,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 */
> > @@ -129,31 +141,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.3
> > 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK