On 22.07.2024 23:06, Peter Xu wrote:
On Mon, Jul 22, 2024 at 05:34:44PM -0300, Fabiano Rosas wrote:Peter Xu <pet...@redhat.com> writes:On Mon, Jul 22, 2024 at 02:59:12PM -0300, Fabiano Rosas wrote:While we cannot yet disentangle the multifd packet from page data, we can make the code a bit cleaner by setting the page-related fields in a separate function. Signed-off-by: Fabiano Rosas <faro...@suse.de> --- migration/multifd.c | 97 ++++++++++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 36 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index fcdb12e04f..d25b8658b2 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -409,65 +409,62 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp) return msg.id; }-void multifd_send_fill_packet(MultiFDSendParams *p)+static void multifd_ram_fill_packet(MultiFDSendParams *p) { MultiFDPacket_t *packet = p->packet; MultiFDPages_t *pages = &p->data->u.ram; - uint64_t packet_num; uint32_t zero_num = pages->num - pages->normal_num; - int i;- packet->flags = cpu_to_be32(p->flags);packet->pages_alloc = cpu_to_be32(pages->allocated); packet->normal_pages = cpu_to_be32(pages->normal_num); packet->zero_pages = cpu_to_be32(zero_num); - packet->next_packet_size = cpu_to_be32(p->next_packet_size);Definitely good intention, but I had a feeling that this will need to be reorganized again when Maciej reworks on top, due to the fact that next_packet_size will be ram-private field, simply because it's defined after pages_alloc and normal_pages... E.g., see: https://lore.kernel.org/r/41dedaf2c9abebb5e45f88c052daa26320715a92.1718717584.git.maciej.szmigi...@oracle.com Where the new MultiFDPacketHdr_t cannot include next_packet_size (even if VFIO will need that too..).Isn't it just a matter of setting next_packet_size in the other path as well? @Maciej, can you comment?typedef struct { uint32_t magic; uint32_t version; uint32_t flags; } __attribute__((packed)) MultiFDPacketHdr_t; So _maybe_ it's easier we drop this patch and leave that part to Maciej to identify which is common and which is arm/vfio specific. No strong opinions here.I could drop it if that's preferrable. However, patch 8/9 is absolutely necessary so we can remove this madness of having to clear MultiFDPages_t fields at the multifd_send_thread() top level. It took me a whole day to figure that one out and that bug has been manifesting ever since I started working on multifd. I'm not sure how we'll do that without this patch. Maybe it's better I fix in this one whatever you guys think needs fixing.You can set next_packet_size only in ram path, or you can keep this patch as is and let Maciej rework it on top. I'm fine either way.
I need to split out the packet header parsing ("unfilling") anyway from
RAM / device state parsing (to detect the packet type before reading the
rest of it to the appropriate data structure) so either way is fine with
me too.
I think if it's easier for Fabiano to work on top of this patch then he
should just keep it.
Thanks,
Maciej
