Re: [PATCH v6 01/13] multifd: Document the locking of MultiFD{Send/Recv}Params

[Juan Quintela]

"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
>>