Chuang Xu <xuchuangxc...@bytedance.com> wrote: > Hi,Juan, > > On 2023/1/30 下午4:09, Juan Quintela wrote: >> Current code asumes that all pages are whole. That is not true for >> example for compression already. Fix it for creating a new field >> ->sent_bytes that includes it. >> >> All ram_counters are used only from the migration thread, so we have >> two options: >> - put a mutex and fill everything when we sent it (not only >> ram_counters, also qemu_file->xfer_bytes). >> - Create a local variable that implements how much has been sent >> through each channel. And when we push another packet, we "add" the >> previous stats. >> >> I choose two due to less changes overall. On the previous code we >> increase transferred and then we sent. Current code goes the other >> way around. It sents the data, and after the fact, it updates the >> counters. Notice that each channel can have a maximum of half a >> megabyte of data without counting, so it is not very important. >> >> Signed-off-by: Juan Quintela <quint...@redhat.com> >> --- >> migration/multifd.h | 2 ++ >> migration/multifd.c | 6 ++++-- >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/migration/multifd.h b/migration/multifd.h >> index e2802a9ce2..36f899c56f 100644 >> --- a/migration/multifd.h >> +++ b/migration/multifd.h >> @@ -102,6 +102,8 @@ typedef struct { >> uint32_t flags; >> /* global number of generated multifd packets */ >> uint64_t packet_num; >> + /* How many bytes have we sent on the last packet */ >> + uint64_t sent_bytes; >> /* thread has work to do */ >> int pending_job; >> /* array of pages to sent. >> diff --git a/migration/multifd.c b/migration/multifd.c >> index 61cafe4c76..cd26b2fda9 100644 >> --- a/migration/multifd.c >> +++ b/migration/multifd.c >> @@ -394,7 +394,6 @@ static int multifd_send_pages(QEMUFile *f) >> static int next_channel; >> MultiFDSendParams *p = NULL; /* make happy gcc */ >> MultiFDPages_t *pages = multifd_send_state->pages; >> - uint64_t transferred; >> >> if (qatomic_read(&multifd_send_state->exiting)) { >> return -1; >> @@ -429,7 +428,8 @@ static int multifd_send_pages(QEMUFile *f) >> p->packet_num = multifd_send_state->packet_num++; >> multifd_send_state->pages = p->pages; >> p->pages = pages; >> - transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len; >> + uint64_t transferred = p->sent_bytes; >> + p->sent_bytes = 0; >> qemu_file_acct_rate_limit(f, transferred); >> qemu_mutex_unlock(&p->mutex); >> stat64_add(&ram_atomic_counters.multifd_bytes, transferred); >> @@ -719,6 +719,8 @@ static void *multifd_send_thread(void *opaque) >> } >> >> qemu_mutex_lock(&p->mutex); >> + p->sent_bytes += p->packet_len; >> + p->sent_bytes += p->next_packet_size; > > Consider a scenario where some normal pages are transmitted in the first > round, > followed by several consecutive rounds of zero pages. When zero pages > are transmitted, > next_packet_size of first round is still incorrectly added to > sent_bytes. If we set a rate > limiting for dirty page transmission, the transmission performance of > multi zero check > will degrade. > > Maybe we should set next_packet_size to 0 in multifd_send_pages()?
See my series of migration atomic counters O:-) You are right with your comments, that is the reason why it took me so many patches to fix it properly. After the last serie on the list that set_bytes variable don't exist anymore and I just do (with atomic operations): multifd_bytes += size_of_write_just_done; And no more sheanigans. Thanks, Juan.