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.
--
Peter Xu
