Re: [PATCH v6 18/19] migration/multifd: Stop changing the packet on recv side

[Peter Xu]

On Tue, Aug 27, 2024 at 04:27:42PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Aug 27, 2024 at 03:45:11PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Tue, Aug 27, 2024 at 02:46:05PM -0300, Fabiano Rosas wrote:
> >> >> @@ -254,12 +250,10 @@ int multifd_ram_unfill_packet(MultiFDRecvParams 
> >> >> *p, Error **errp)
> >> >>          return 0;
> >> >>      }
> >> >>  
> >> >> -    /* make sure that ramblock is 0 terminated */
> >> >> -    packet->ramblock[255] = 0;
> >> >> -    p->block = qemu_ram_block_by_name(packet->ramblock);
> >> >> +    ramblock_name = g_strndup(packet->ramblock, 255);
> >> >
> >> > I understand we want to move to a const*, however this introduces a 256B
> >> > allocation per multifd packet, which we definitely want to avoid.. I 
> >> > wonder
> >> > whether that's worthwhile just to make it const. :-(
> >> >
> >> > I don't worry too much on the const* and vars pointed being abused /
> >> > updated when without it - the packet struct is pretty much limited only 
> >> > to
> >> > be referenced in this unfill function, and then we will do the load based
> >> > on MultiFDRecvParams* later anyway.  So personally I'd rather lose the
> >> > const* v.s. one allocation.
> >> >
> >> > Or we could also sanity check byte 255 to be '\0' (which, AFAIU, should
> >> > always be the case..), then we can get both benefits.
> >> 
> >> We can't because it breaks compat. Previous QEMUs didn't zero the
> >> packet.
> >
> > Ouch!
> >
> > Then.. shall we still try to avoid the allocation?
> 
> Can I strcpy it to the stack?
> 
> char idstr[256];
> 
> strncpy(&idstr, packet->ramblock, 256);
> idstr[255] = 0;

Should be much better than an allocation, yes.  However personally I'd
still try to avoid that.

Multifd is a performance feature, after all, so we care about perf here
more than elsewhere.  Meanwhile this is exactly the hot path on recv
side.. so it might still be wise we leave all non-trivial cosmetic changes
for later when it's against it.

-- 
Peter Xu