On Wed, Feb 21, 2024 at 7:37 AM Elena Ufimtseva <ufimts...@gmail.com> wrote:
>
>
>
> On Fri, Feb 16, 2024 at 2:41 PM Hao Xiang <hao.xi...@bytedance.com> wrote:
>>
>> This change adds zero page counters and updates multifd send/receive
>> tracing format to track the newly added counters.
>>
>> Signed-off-by: Hao Xiang <hao.xi...@bytedance.com>
>> ---
>> migration/multifd.c | 43 ++++++++++++++++++++++++++++++++++--------
>> migration/multifd.h | 21 ++++++++++++++++++++-
>> migration/ram.c | 1 -
>> migration/trace-events | 8 ++++----
>> 4 files changed, 59 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index adfe8c9a0a..a33dba40d9 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -236,6 +236,8 @@ static void multifd_pages_reset(MultiFDPages_t *pages)
>> * overwritten later when reused.
>> */
>> pages->num = 0;
>> + pages->normal_num = 0;
>> + pages->zero_num = 0;
>> pages->block = NULL;
>> }
>>
>>
>> @@ -309,6 +311,8 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>>
>> pages->allocated = n;
>> pages->offset = g_new0(ram_addr_t, n);
>> + pages->normal = g_new0(ram_addr_t, n);
>> + pages->zero = g_new0(ram_addr_t, n);
>>
>>
>> return pages;
>> }
>> @@ -319,6 +323,10 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
>> pages->allocated = 0;
>> g_free(pages->offset);
>> pages->offset = NULL;
>> + g_free(pages->normal);
>> + pages->normal = NULL;
>> + g_free(pages->zero);
>> + pages->zero = NULL;
>> g_free(pages);
>> }
>>
>> @@ -332,6 +340,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
>> packet->flags = cpu_to_be32(p->flags);
>> packet->pages_alloc = cpu_to_be32(p->pages->allocated);
>> packet->normal_pages = cpu_to_be32(pages->num);
>> + packet->zero_pages = cpu_to_be32(pages->zero_num);
>> packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>>
>> packet_num = qatomic_fetch_inc(&multifd_send_state->packet_num);
>> @@ -350,9 +359,10 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
>>
>> p->packets_sent++;
>> p->total_normal_pages += pages->num;
>> + p->total_zero_pages += pages->zero_num;
>>
>> - trace_multifd_send(p->id, packet_num, pages->num, p->flags,
>> - p->next_packet_size);
>> + trace_multifd_send(p->id, packet_num, pages->num, pages->zero_num,
>> + p->flags, p->next_packet_size);
>> }
>>
>> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> @@ -393,20 +403,29 @@ static int
>> multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> p->normal_num = be32_to_cpu(packet->normal_pages);
>> if (p->normal_num > packet->pages_alloc) {
>> error_setg(errp, "multifd: received packet "
>> - "with %u pages and expected maximum pages are %u",
>> + "with %u normal pages and expected maximum pages are %u",
>> p->normal_num, packet->pages_alloc) ;
>> return -1;
>> }
>>
>> + p->zero_num = be32_to_cpu(packet->zero_pages);
>> + if (p->zero_num > packet->pages_alloc - p->normal_num) {
>> + error_setg(errp, "multifd: received packet "
>> + "with %u zero pages and expected maximum zero pages are
>> %u",
>> + p->zero_num, packet->pages_alloc - p->normal_num) ;
>> + return -1;
>> + }
>
>
> You could probably combine this check with normal_num against pages_alloc.
>>
>> +
>> p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>> p->packet_num = be64_to_cpu(packet->packet_num);
>> p->packets_recved++;
>> p->total_normal_pages += p->normal_num;
>> + p->total_zero_pages += p->zero_num;
>>
>> - trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
>> - p->next_packet_size);
>> + trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
>> + p->flags, p->next_packet_size);
>>
>> - if (p->normal_num == 0) {
>> + if (p->normal_num == 0 && p->zero_num == 0) {
>> return 0;
>> }
>>
>> @@ -823,6 +842,8 @@ static void *multifd_send_thread(void *opaque)
>>
>> stat64_add(&mig_stats.multifd_bytes,
>> p->next_packet_size + p->packet_len);
>> + stat64_add(&mig_stats.normal_pages, pages->num);
>
>
> That seems wrong. pages->num is the number of pages total in the packet.
> But next patch changes it, so I suggest or change it here and not in 3/7.
In this patch, multifd zero pages are not enabled yet. So pages->num
are the number of normal pages not pages total in the packet. The zero
pages were send in a different format in save_zero_page. Later on,
when multifd zero page is enabled, pages->normal_num counts the number
of normal pages and hence the accounting is changed.
>
>> + stat64_add(&mig_stats.zero_pages, pages->zero_num);
>>
>> multifd_pages_reset(p->pages);
>> p->next_packet_size = 0;
>> @@ -866,7 +887,8 @@ out:
>>
>> rcu_unregister_thread();
>> migration_threads_remove(thread);
>> - trace_multifd_send_thread_end(p->id, p->packets_sent,
>> p->total_normal_pages);
>> + trace_multifd_send_thread_end(p->id, p->packets_sent,
>> p->total_normal_pages,
>> + p->total_zero_pages);
>>
>> return NULL;
>> }
>> @@ -1132,6 +1154,8 @@ static void
>> multifd_recv_cleanup_channel(MultiFDRecvParams *p)
>> p->iov = NULL;
>> g_free(p->normal);
>> p->normal = NULL;
>> + g_free(p->zero);
>> + p->zero = NULL;
>> multifd_recv_state->ops->recv_cleanup(p);
>> }
>>
>> @@ -1251,7 +1275,9 @@ static void *multifd_recv_thread(void *opaque)
>> }
>>
>> rcu_unregister_thread();
>> - trace_multifd_recv_thread_end(p->id, p->packets_recved,
>> p->total_normal_pages);
>> + trace_multifd_recv_thread_end(p->id, p->packets_recved,
>> + p->total_normal_pages,
>> + p->total_zero_pages);
>>
>> return NULL;
>> }
>> @@ -1290,6 +1316,7 @@ int multifd_recv_setup(Error **errp)
>> p->name = g_strdup_printf("multifdrecv_%d", i);
>> p->iov = g_new0(struct iovec, page_count);
>> p->normal = g_new0(ram_addr_t, page_count);
>> + p->zero = g_new0(ram_addr_t, page_count);
>> p->page_count = page_count;
>> p->page_size = qemu_target_page_size();
>> }
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 8a1cad0996..9822ff298a 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -48,7 +48,10 @@ typedef struct {
>> /* size of the next packet that contains pages */
>> uint32_t next_packet_size;
>> uint64_t packet_num;
>> - uint64_t unused[4]; /* Reserved for future use */
>> + /* zero pages */
>> + uint32_t zero_pages;
>> + uint32_t unused32[1]; /* Reserved for future use */
>> + uint64_t unused64[3]; /* Reserved for future use */
>> char ramblock[256];
>> uint64_t offset[];
>> } __attribute__((packed)) MultiFDPacket_t;
>> @@ -56,10 +59,18 @@ typedef struct {
>> typedef struct {
>> /* number of used pages */
>> uint32_t num;
>> + /* number of normal pages */
>> + uint32_t normal_num;
>> + /* number of zero pages */
>> + uint32_t zero_num;
>> /* number of allocated pages */
>> uint32_t allocated;
>> /* offset of each page */
>> ram_addr_t *offset;
>> + /* offset of normal page */
>> + ram_addr_t *normal;
>> + /* offset of zero page */
>> + ram_addr_t *zero;
>> RAMBlock *block;
>> } MultiFDPages_t;
>>
>> @@ -124,6 +135,8 @@ typedef struct {
>> uint64_t packets_sent;
>> /* non zero pages sent through this channel */
>> uint64_t total_normal_pages;
>> + /* zero pages sent through this channel */
>> + uint64_t total_zero_pages;
>
>
> Can we initialize these to zero when threads are being set up?
> Also, I have a strong desire to rename these.. later.
When MultiFDSendParams are allocated in multifd_send_setup, g_new0
will initialize them to zero.
>
>>
>> /* buffers to send */
>> struct iovec *iov;
>> /* number of iovs used */
>> @@ -178,12 +191,18 @@ typedef struct {
>> uint8_t *host;
>> /* non zero pages recv through this channel */
>> uint64_t total_normal_pages;
>> + /* zero pages recv through this channel */
>> + uint64_t total_zero_pages;
>> /* buffers to recv */
>> struct iovec *iov;
>> /* Pages that are not zero */
>> ram_addr_t *normal;
>> /* num of non zero pages */
>> uint32_t normal_num;
>> + /* Pages that are zero */
>> + ram_addr_t *zero;
>> + /* num of zero pages */
>> + uint32_t zero_num;
>> /* used for de-compression methods */
>> void *data;
>> } MultiFDRecvParams;
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 556725c30f..5ece9f042e 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1259,7 +1259,6 @@ static int ram_save_multifd_page(RAMBlock *block,
>> ram_addr_t offset)
>> if (!multifd_queue_page(block, offset)) {
>> return -1;
>> }
>> - stat64_add(&mig_stats.normal_pages, 1);
>>
>> return 1;
>> }
>> diff --git a/migration/trace-events b/migration/trace-events
>> index 298ad2b0dd..9f1d7ae71a 100644
>> --- a/migration/trace-events
>> +++ b/migration/trace-events
>> @@ -128,21 +128,21 @@ postcopy_preempt_reset_channel(void) ""
>> # multifd.c
>> multifd_new_send_channel_async(uint8_t id) "channel %u"
>> multifd_new_send_channel_async_error(uint8_t id, void *err) "channel=%u
>> err=%p"
>> -multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t
>> flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " pages
>> %u flags 0x%x next packet size %u"
>> +multifd_recv(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t
>> zero, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %"
>> PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
>> multifd_recv_new_channel(uint8_t id) "channel %u"
>> multifd_recv_sync_main(long packet_num) "packet num %ld"
>> multifd_recv_sync_main_signal(uint8_t id) "channel %u"
>> multifd_recv_sync_main_wait(uint8_t id) "channel %u"
>> multifd_recv_terminate_threads(bool error) "error %d"
>> -multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages)
>> "channel %u packets %" PRIu64 " pages %" PRIu64
>> +multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t
>> normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal
>> pages %" PRIu64 " zero pages %" PRIu64
>> multifd_recv_thread_start(uint8_t id) "%u"
>> -multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t
>> flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal
>> pages %u flags 0x%x next packet size %u"
>> +multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal_pages,
>> uint32_t zero_pages, uint32_t flags, uint32_t next_packet_size) "channel %u
>> packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet
>> size %u"
>> multifd_send_error(uint8_t id) "channel %u"
>> multifd_send_sync_main(long packet_num) "packet num %ld"
>> multifd_send_sync_main_signal(uint8_t id) "channel %u"
>> multifd_send_sync_main_wait(uint8_t id) "channel %u"
>> multifd_send_terminate_threads(void) ""
>> -multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t
>> normal_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64
>> +multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t
>> normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal
>> pages %" PRIu64 " zero pages %" PRIu64
>> multifd_send_thread_start(uint8_t id) "%u"
>> multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char
>> *hostname) "ioc=%p tioc=%p hostname=%s"
>> multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p
>> err=%s"
>> --
>> 2.30.2
>>
>>
>
>
> --
> Elena
