| Message ID | 20240722175914.24022-9-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration/multifd: Remove multifd_send_state->pages | expand |
On Mon, Jul 22, 2024 at 02:59:13PM -0300, Fabiano Rosas wrote: > Skip saving and loading any ram data in the packet in the case of a > SYNC. This fixes a shortcoming of the current code which requires a > reset of the MultiFDPages_t fields right after the previous > pending_job finishes, otherwise the very next job might be a SYNC and > multifd_send_fill_packet() will put the stale values in the packet. > > By not calling multifd_ram_fill_packet(), we can stop resetting > MultiFDPages_t in the multifd core and leave that to the client code. > > Actually moving the reset function is not yet done because > pages->num==0 is used by the client code to determine whether the > MultiFDPages_t needs to be flushed. The subsequent patches will > replace that with a generic flag that is not dependent on > MultiFDPages_t. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/multifd.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index d25b8658b2..4394ca6ade 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -438,6 +438,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p) > { > MultiFDPacket_t *packet = p->packet; > uint64_t packet_num; > + bool sync_packet = p->flags & MULTIFD_FLAG_SYNC; > > memset(packet, 0, p->packet_len); > > @@ -452,7 +453,9 @@ void multifd_send_fill_packet(MultiFDSendParams *p) > > p->packets_sent++; > > - multifd_ram_fill_packet(p); > + if (!sync_packet) { > + multifd_ram_fill_packet(p); > + } > > trace_multifd_send(p->id, packet_num, > be32_to_cpu(packet->normal_pages), > @@ -563,7 +566,12 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) > p->packet_num = be64_to_cpu(packet->packet_num); > p->packets_recved++; > > - ret = multifd_ram_unfill_packet(p, errp); > + if (p->flags & MULTIFD_FLAG_SYNC) { > + p->normal_num = 0; > + p->zero_num = 0; Instead of this, I wonder whether we shouldn't touch those fields at all, but: diff --git a/migration/multifd.c b/migration/multifd.c index 0a85951d58..55abd9a1ef 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -1547,7 +1547,9 @@ static void *multifd_recv_thread(void *opaque) flags = p->flags; /* recv methods don't know how to handle the SYNC flag */ p->flags &= ~MULTIFD_FLAG_SYNC; - has_data = p->normal_num || p->zero_num; + + if (!(flags & MULTIFD_FLAG_SYNC)) + has_data = p->normal_num || p->zero_num; qemu_mutex_unlock(&p->mutex); } else { /* > + } else { > + ret = multifd_ram_unfill_packet(p, errp); > + } > > trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num, > p->flags, p->next_packet_size); > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Mon, Jul 22, 2024 at 02:59:13PM -0300, Fabiano Rosas wrote: >> Skip saving and loading any ram data in the packet in the case of a >> SYNC. This fixes a shortcoming of the current code which requires a >> reset of the MultiFDPages_t fields right after the previous >> pending_job finishes, otherwise the very next job might be a SYNC and >> multifd_send_fill_packet() will put the stale values in the packet. >> >> By not calling multifd_ram_fill_packet(), we can stop resetting >> MultiFDPages_t in the multifd core and leave that to the client code. >> >> Actually moving the reset function is not yet done because >> pages->num==0 is used by the client code to determine whether the >> MultiFDPages_t needs to be flushed. The subsequent patches will >> replace that with a generic flag that is not dependent on >> MultiFDPages_t. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/multifd.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/migration/multifd.c b/migration/multifd.c >> index d25b8658b2..4394ca6ade 100644 >> --- a/migration/multifd.c >> +++ b/migration/multifd.c >> @@ -438,6 +438,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p) >> { >> MultiFDPacket_t *packet = p->packet; >> uint64_t packet_num; >> + bool sync_packet = p->flags & MULTIFD_FLAG_SYNC; >> >> memset(packet, 0, p->packet_len); >> >> @@ -452,7 +453,9 @@ void multifd_send_fill_packet(MultiFDSendParams *p) >> >> p->packets_sent++; >> >> - multifd_ram_fill_packet(p); >> + if (!sync_packet) { >> + multifd_ram_fill_packet(p); >> + } >> >> trace_multifd_send(p->id, packet_num, >> be32_to_cpu(packet->normal_pages), >> @@ -563,7 +566,12 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) >> p->packet_num = be64_to_cpu(packet->packet_num); >> p->packets_recved++; >> >> - ret = multifd_ram_unfill_packet(p, errp); >> + if (p->flags & MULTIFD_FLAG_SYNC) { >> + p->normal_num = 0; >> + p->zero_num = 0; > > Instead of this, I wonder whether we shouldn't touch those fields at all, > but: > > diff --git a/migration/multifd.c b/migration/multifd.c > index 0a85951d58..55abd9a1ef 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -1547,7 +1547,9 @@ static void *multifd_recv_thread(void *opaque) > flags = p->flags; > /* recv methods don't know how to handle the SYNC flag */ > p->flags &= ~MULTIFD_FLAG_SYNC; > - has_data = p->normal_num || p->zero_num; > + > + if (!(flags & MULTIFD_FLAG_SYNC)) > + has_data = p->normal_num || p->zero_num; > qemu_mutex_unlock(&p->mutex); > } else { > /* Good idea. > >> + } else { >> + ret = multifd_ram_unfill_packet(p, errp); >> + } >> >> trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num, >> p->flags, p->next_packet_size); >> -- >> 2.35.3 >>
diff --git a/migration/multifd.c b/migration/multifd.c index d25b8658b2..4394ca6ade 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -438,6 +438,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p) { MultiFDPacket_t *packet = p->packet; uint64_t packet_num; + bool sync_packet = p->flags & MULTIFD_FLAG_SYNC; memset(packet, 0, p->packet_len); @@ -452,7 +453,9 @@ void multifd_send_fill_packet(MultiFDSendParams *p) p->packets_sent++; - multifd_ram_fill_packet(p); + if (!sync_packet) { + multifd_ram_fill_packet(p); + } trace_multifd_send(p->id, packet_num, be32_to_cpu(packet->normal_pages), @@ -563,7 +566,12 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) p->packet_num = be64_to_cpu(packet->packet_num); p->packets_recved++; - ret = multifd_ram_unfill_packet(p, errp); + if (p->flags & MULTIFD_FLAG_SYNC) { + p->normal_num = 0; + p->zero_num = 0; + } else { + ret = multifd_ram_unfill_packet(p, errp); + } trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num, p->flags, p->next_packet_size);
Skip saving and loading any ram data in the packet in the case of a SYNC. This fixes a shortcoming of the current code which requires a reset of the MultiFDPages_t fields right after the previous pending_job finishes, otherwise the very next job might be a SYNC and multifd_send_fill_packet() will put the stale values in the packet. By not calling multifd_ram_fill_packet(), we can stop resetting MultiFDPages_t in the multifd core and leave that to the client code. Actually moving the reset function is not yet done because pages->num==0 is used by the client code to determine whether the MultiFDPages_t needs to be flushed. The subsequent patches will replace that with a generic flag that is not dependent on MultiFDPages_t. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/multifd.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)