| Message ID | 20220119080929.39485-11-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
* Peter Xu (peterx@redhat.com) wrote: > Static variable is very unfriendly to threading of ram_block_from_stream(). > Move it into MigrationIncomingState. > > Make the incoming state pointer to be passed over to ram_block_from_stream() on > both caller sites. > > Signed-off-by: Peter Xu <peterx@redhat.com> OK, but I'm not sure if I noticed where you changed this to be per channel later? Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Dave > --- > migration/migration.h | 3 ++- > migration/ram.c | 13 +++++++++---- > 2 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/migration/migration.h b/migration/migration.h > index 35e7f7babe..34b79cb961 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -66,7 +66,8 @@ typedef struct { > /* State for the incoming migration */ > struct MigrationIncomingState { > QEMUFile *from_src_file; > - > + /* Previously received RAM's RAMBlock pointer */ > + RAMBlock *last_recv_block; > /* A hook to allow cleanup at the end of incoming migration */ > void *transport_data; > void (*transport_cleanup)(void *data); > diff --git a/migration/ram.c b/migration/ram.c > index 3f823ffffc..3a7d943f9c 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3183,12 +3183,14 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > * > * Returns a pointer from within the RCU-protected ram_list. > * > + * @mis: the migration incoming state pointer > * @f: QEMUFile where to read the data from > * @flags: Page flags (mostly to see if it's a continuation of previous block) > */ > -static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) > +static inline RAMBlock *ram_block_from_stream(MigrationIncomingState *mis, > + QEMUFile *f, int flags) > { > - static RAMBlock *block; > + RAMBlock *block = mis->last_recv_block; > char id[256]; > uint8_t len; > > @@ -3215,6 +3217,8 @@ static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) > return NULL; > } > > + mis->last_recv_block = block; > + > return block; > } > > @@ -3667,7 +3671,7 @@ static int ram_load_postcopy(QEMUFile *f) > trace_ram_load_postcopy_loop((uint64_t)addr, flags); > if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | > RAM_SAVE_FLAG_COMPRESS_PAGE)) { > - block = ram_block_from_stream(f, flags); > + block = ram_block_from_stream(mis, f, flags); > if (!block) { > ret = -EINVAL; > break; > @@ -3881,6 +3885,7 @@ void colo_flush_ram_cache(void) > */ > static int ram_load_precopy(QEMUFile *f) > { > + MigrationIncomingState *mis = migration_incoming_get_current(); > int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0; > /* ADVISE is earlier, it shows the source has the postcopy capability on */ > bool postcopy_advised = postcopy_is_advised(); > @@ -3919,7 +3924,7 @@ static int ram_load_precopy(QEMUFile *f) > > if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | > RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) { > - RAMBlock *block = ram_block_from_stream(f, flags); > + RAMBlock *block = ram_block_from_stream(mis, f, flags); > > host = host_from_ram_block_offset(block, addr); > /* > -- > 2.32.0 >
On Thu, Feb 03, 2022 at 05:48:31PM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > Static variable is very unfriendly to threading of ram_block_from_stream(). > > Move it into MigrationIncomingState. > > > > Make the incoming state pointer to be passed over to ram_block_from_stream() on > > both caller sites. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > OK, but I'm not sure if I noticed where you changed this to be per > channel later? It's done in the last patch where it'll start to pass over "channel" index into ram_block_from_stream(): static inline RAMBlock *ram_block_from_stream(MigrationIncomingState *mis, QEMUFile *f, int flags, int channel) { RAMBlock *block = mis->last_recv_block[channel]; ... } I could have moved it into the new PostcopyTmpPage structure, but it'll be a bit weird because precopy also uses this to cache the block info, hence I made it an array. > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Thanks,
diff --git a/migration/migration.h b/migration/migration.h index 35e7f7babe..34b79cb961 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -66,7 +66,8 @@ typedef struct { /* State for the incoming migration */ struct MigrationIncomingState { QEMUFile *from_src_file; - + /* Previously received RAM's RAMBlock pointer */ + RAMBlock *last_recv_block; /* A hook to allow cleanup at the end of incoming migration */ void *transport_data; void (*transport_cleanup)(void *data); diff --git a/migration/ram.c b/migration/ram.c index 3f823ffffc..3a7d943f9c 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3183,12 +3183,14 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) * * Returns a pointer from within the RCU-protected ram_list. * + * @mis: the migration incoming state pointer * @f: QEMUFile where to read the data from * @flags: Page flags (mostly to see if it's a continuation of previous block) */ -static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) +static inline RAMBlock *ram_block_from_stream(MigrationIncomingState *mis, + QEMUFile *f, int flags) { - static RAMBlock *block; + RAMBlock *block = mis->last_recv_block; char id[256]; uint8_t len; @@ -3215,6 +3217,8 @@ static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags) return NULL; } + mis->last_recv_block = block; + return block; } @@ -3667,7 +3671,7 @@ static int ram_load_postcopy(QEMUFile *f) trace_ram_load_postcopy_loop((uint64_t)addr, flags); if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE)) { - block = ram_block_from_stream(f, flags); + block = ram_block_from_stream(mis, f, flags); if (!block) { ret = -EINVAL; break; @@ -3881,6 +3885,7 @@ void colo_flush_ram_cache(void) */ static int ram_load_precopy(QEMUFile *f) { + MigrationIncomingState *mis = migration_incoming_get_current(); int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0; /* ADVISE is earlier, it shows the source has the postcopy capability on */ bool postcopy_advised = postcopy_is_advised(); @@ -3919,7 +3924,7 @@ static int ram_load_precopy(QEMUFile *f) if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) { - RAMBlock *block = ram_block_from_stream(f, flags); + RAMBlock *block = ram_block_from_stream(mis, f, flags); host = host_from_ram_block_offset(block, addr); /*
Static variable is very unfriendly to threading of ram_block_from_stream(). Move it into MigrationIncomingState. Make the incoming state pointer to be passed over to ram_block_from_stream() on both caller sites. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.h | 3 ++- migration/ram.c | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-)