| Message ID | 20230515195709.63843-17-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote: > In the past, we had to put the in the main thread all the operations > related with sizes due to qemu_file not beeing thread safe. As now > all counters are atomic, we can update the counters just after the > do the write. As an aditional bonus, we are able to use the right > value for the compression methods. Right now we were assuming that > there were no compression at all. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/multifd.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index aabf9b6d98..0bf5958a9c 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops) > static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > { > MultiFDInit_t msg = {}; > + size_t size = sizeof(msg); > int ret; > > msg.magic = cpu_to_be32(MULTIFD_MAGIC); > @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > msg.id = p->id; > memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid)); > > - ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp); > + ret = qio_channel_write_all(p->c, (char *)&msg, size, errp); > if (ret != 0) { > return -1; > } > + stat64_add(&mig_stats.multifd_bytes, size); > + stat64_add(&mig_stats.transferred, size); > return 0; > } Humm, those are atomic ops, right? You think we could have 'multifd_bytes' and 'transferred' in the same cacheline, to avoid 2 cacheline bounces? Well, it's unrelated to this patchset, so: Reviewed-by: Leonardo Bras <leobras@redhat.com> > > @@ -395,7 +398,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; > @@ -430,10 +432,7 @@ 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; > qemu_mutex_unlock(&p->mutex); > - stat64_add(&mig_stats.transferred, transferred); > - stat64_add(&mig_stats.multifd_bytes, transferred); > qemu_sem_post(&p->sem); > > return 1; > @@ -715,6 +714,8 @@ static void *multifd_send_thread(void *opaque) > if (ret != 0) { > break; > } > + stat64_add(&mig_stats.multifd_bytes, p->packet_len); > + stat64_add(&mig_stats.transferred, p->packet_len); > } else { > /* Send header using the same writev call */ > p->iov[0].iov_len = p->packet_len; > @@ -727,6 +728,8 @@ static void *multifd_send_thread(void *opaque) > break; > } > > + stat64_add(&mig_stats.multifd_bytes, p->next_packet_size); > + stat64_add(&mig_stats.transferred, p->next_packet_size); > qemu_mutex_lock(&p->mutex); > p->pending_job--; > qemu_mutex_unlock(&p->mutex);
Leonardo Brás <leobras@redhat.com> wrote: > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote: >> In the past, we had to put the in the main thread all the operations >> related with sizes due to qemu_file not beeing thread safe. As now >> all counters are atomic, we can update the counters just after the >> do the write. As an aditional bonus, we are able to use the right >> value for the compression methods. Right now we were assuming that >> there were no compression at all. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/multifd.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/migration/multifd.c b/migration/multifd.c >> index aabf9b6d98..0bf5958a9c 100644 >> --- a/migration/multifd.c >> +++ b/migration/multifd.c >> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops) >> static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) >> { >> MultiFDInit_t msg = {}; >> + size_t size = sizeof(msg); >> int ret; >> >> msg.magic = cpu_to_be32(MULTIFD_MAGIC); >> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) >> msg.id = p->id; >> memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid)); >> >> - ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp); >> + ret = qio_channel_write_all(p->c, (char *)&msg, size, errp); >> if (ret != 0) { >> return -1; >> } >> + stat64_add(&mig_stats.multifd_bytes, size); >> + stat64_add(&mig_stats.transferred, size); >> return 0; >> } > > Humm, those are atomic ops, right? > > You think we could have 'multifd_bytes' and 'transferred' in the same cacheline, > to avoid 2 cacheline bounces? Don't matter on next series. mig_stats.transferred is dropped. And transferred becomes: qemu_file_transferred + multifd_bytes + rdma_bytes. So everytime that we do a write, we only update one counter. > Well, it's unrelated to this patchset, so: > > Reviewed-by: Leonardo Bras <leobras@redhat.com>
On Fri, May 26, 2023 at 5:24 AM Juan Quintela <quintela@redhat.com> wrote: > > Leonardo Brás <leobras@redhat.com> wrote: > > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote: > >> In the past, we had to put the in the main thread all the operations > >> related with sizes due to qemu_file not beeing thread safe. As now > >> all counters are atomic, we can update the counters just after the > >> do the write. As an aditional bonus, we are able to use the right > >> value for the compression methods. Right now we were assuming that > >> there were no compression at all. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/multifd.c | 13 ++++++++----- > >> 1 file changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/migration/multifd.c b/migration/multifd.c > >> index aabf9b6d98..0bf5958a9c 100644 > >> --- a/migration/multifd.c > >> +++ b/migration/multifd.c > >> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops) > >> static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > >> { > >> MultiFDInit_t msg = {}; > >> + size_t size = sizeof(msg); > >> int ret; > >> > >> msg.magic = cpu_to_be32(MULTIFD_MAGIC); > >> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > >> msg.id = p->id; > >> memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid)); > >> > >> - ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp); > >> + ret = qio_channel_write_all(p->c, (char *)&msg, size, errp); > >> if (ret != 0) { > >> return -1; > >> } > >> + stat64_add(&mig_stats.multifd_bytes, size); > >> + stat64_add(&mig_stats.transferred, size); > >> return 0; > >> } > > > > Humm, those are atomic ops, right? > > > > You think we could have 'multifd_bytes' and 'transferred' in the same cacheline, > > to avoid 2 cacheline bounces? > > Don't matter on next series. > > mig_stats.transferred is dropped. > > And transferred becomes: > > qemu_file_transferred + multifd_bytes + rdma_bytes. > > So everytime that we do a write, we only update one counter. That's even better :) Thanks! > > > Well, it's unrelated to this patchset, so: > > > > Reviewed-by: Leonardo Bras <leobras@redhat.com> >
diff --git a/migration/multifd.c b/migration/multifd.c index aabf9b6d98..0bf5958a9c 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops) static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) { MultiFDInit_t msg = {}; + size_t size = sizeof(msg); int ret; msg.magic = cpu_to_be32(MULTIFD_MAGIC); @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) msg.id = p->id; memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid)); - ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp); + ret = qio_channel_write_all(p->c, (char *)&msg, size, errp); if (ret != 0) { return -1; } + stat64_add(&mig_stats.multifd_bytes, size); + stat64_add(&mig_stats.transferred, size); return 0; } @@ -395,7 +398,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; @@ -430,10 +432,7 @@ 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; qemu_mutex_unlock(&p->mutex); - stat64_add(&mig_stats.transferred, transferred); - stat64_add(&mig_stats.multifd_bytes, transferred); qemu_sem_post(&p->sem); return 1; @@ -715,6 +714,8 @@ static void *multifd_send_thread(void *opaque) if (ret != 0) { break; } + stat64_add(&mig_stats.multifd_bytes, p->packet_len); + stat64_add(&mig_stats.transferred, p->packet_len); } else { /* Send header using the same writev call */ p->iov[0].iov_len = p->packet_len; @@ -727,6 +728,8 @@ static void *multifd_send_thread(void *opaque) break; } + stat64_add(&mig_stats.multifd_bytes, p->next_packet_size); + stat64_add(&mig_stats.transferred, p->next_packet_size); qemu_mutex_lock(&p->mutex); p->pending_job--; qemu_mutex_unlock(&p->mutex);
In the past, we had to put the in the main thread all the operations related with sizes due to qemu_file not beeing thread safe. As now all counters are atomic, we can update the counters just after the do the write. As an aditional bonus, we are able to use the right value for the compression methods. Right now we were assuming that there were no compression at all. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/multifd.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)