| Message ID | 1739542467-226739-3-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Live update: vfio and iommufd | expand |
On Fri, Feb 14, 2025 at 06:13:44AM -0800, Steve Sistare wrote: > Add cpr_needed_for_reuse, cpr_resave_fd helpers, cpr_is_incoming, and > cpr_open_fd, for use when adding cpr support for vfio and iommufd. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/migration/cpr.h | 6 ++++++ > migration/cpr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/include/migration/cpr.h b/include/migration/cpr.h > index 3a6deb7..6ad04d4 100644 > --- a/include/migration/cpr.h > +++ b/include/migration/cpr.h > @@ -18,15 +18,21 @@ > void cpr_save_fd(const char *name, int id, int fd); > void cpr_delete_fd(const char *name, int id); > int cpr_find_fd(const char *name, int id); > +void cpr_resave_fd(const char *name, int id, int fd); > +int cpr_open_fd(const char *path, int flags, const char *name, int id, > + bool *reused, Error **errp); > > MigMode cpr_get_incoming_mode(void); > void cpr_set_incoming_mode(MigMode mode); > +bool cpr_is_incoming(void); > > int cpr_state_save(MigrationChannel *channel, Error **errp); > int cpr_state_load(MigrationChannel *channel, Error **errp); > void cpr_state_close(void); > struct QIOChannel *cpr_state_ioc(void); > > +bool cpr_needed_for_reuse(void *opaque); > + > QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp); > QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp); > > diff --git a/migration/cpr.c b/migration/cpr.c > index 584b0b9..12c489b 100644 > --- a/migration/cpr.c > +++ b/migration/cpr.c > @@ -95,6 +95,39 @@ int cpr_find_fd(const char *name, int id) > trace_cpr_find_fd(name, id, fd); > return fd; > } > + > +void cpr_resave_fd(const char *name, int id, int fd) > +{ > + CprFd *elem = find_fd(&cpr_state.fds, name, id); > + int old_fd = elem ? elem->fd : -1; > + > + if (old_fd < 0) { > + cpr_save_fd(name, id, fd); > + } else if (old_fd != fd) { > + error_setg(&error_fatal, > + "internal error: cpr fd '%s' id %d value %d " > + "already saved with a different value %d", > + name, id, fd, old_fd); How bad it is to trigger this? I wonder if cpr_save_fd() should have checked this already on duplicated entries; it looks risky there too if this happens to existing cpr_save_fd() callers. > + } > +} > + > +int cpr_open_fd(const char *path, int flags, const char *name, int id, > + bool *reused, Error **errp) > +{ > + int fd = cpr_find_fd(name, id); > + > + if (reused) { > + *reused = (fd >= 0); > + } > + if (fd < 0) { > + fd = qemu_open(path, flags, errp); > + if (fd >= 0) { > + cpr_save_fd(name, id, fd); > + } > + } > + return fd; > +} > + > /*************************************************************************/ > #define CPR_STATE "CprState" > > @@ -128,6 +161,11 @@ void cpr_set_incoming_mode(MigMode mode) > incoming_mode = mode; > } > > +bool cpr_is_incoming(void) > +{ > + return incoming_mode != MIG_MODE_NONE; > +} Maybe it'll be helpful to document either this function or incoming_mode; it's probably not yet obvious to most readers incoming_mode is only set to !NONE during a small window when VM loads. > + > int cpr_state_save(MigrationChannel *channel, Error **errp) > { > int ret; > @@ -222,3 +260,9 @@ void cpr_state_close(void) > cpr_state_file = NULL; > } > } > + > +bool cpr_needed_for_reuse(void *opaque) > +{ > + MigMode mode = migrate_mode(); Nit: can drop the var. > + return mode == MIG_MODE_CPR_TRANSFER; > +} > -- > 1.8.3.1 >
On 2/14/2025 11:37 AM, Peter Xu wrote: > On Fri, Feb 14, 2025 at 06:13:44AM -0800, Steve Sistare wrote: >> Add cpr_needed_for_reuse, cpr_resave_fd helpers, cpr_is_incoming, and >> cpr_open_fd, for use when adding cpr support for vfio and iommufd. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> include/migration/cpr.h | 6 ++++++ >> migration/cpr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/include/migration/cpr.h b/include/migration/cpr.h >> index 3a6deb7..6ad04d4 100644 >> --- a/include/migration/cpr.h >> +++ b/include/migration/cpr.h >> @@ -18,15 +18,21 @@ >> void cpr_save_fd(const char *name, int id, int fd); >> void cpr_delete_fd(const char *name, int id); >> int cpr_find_fd(const char *name, int id); >> +void cpr_resave_fd(const char *name, int id, int fd); >> +int cpr_open_fd(const char *path, int flags, const char *name, int id, >> + bool *reused, Error **errp); >> >> MigMode cpr_get_incoming_mode(void); >> void cpr_set_incoming_mode(MigMode mode); >> +bool cpr_is_incoming(void); >> >> int cpr_state_save(MigrationChannel *channel, Error **errp); >> int cpr_state_load(MigrationChannel *channel, Error **errp); >> void cpr_state_close(void); >> struct QIOChannel *cpr_state_ioc(void); >> >> +bool cpr_needed_for_reuse(void *opaque); >> + >> QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp); >> QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp); >> >> diff --git a/migration/cpr.c b/migration/cpr.c >> index 584b0b9..12c489b 100644 >> --- a/migration/cpr.c >> +++ b/migration/cpr.c >> @@ -95,6 +95,39 @@ int cpr_find_fd(const char *name, int id) >> trace_cpr_find_fd(name, id, fd); >> return fd; >> } >> + >> +void cpr_resave_fd(const char *name, int id, int fd) >> +{ >> + CprFd *elem = find_fd(&cpr_state.fds, name, id); >> + int old_fd = elem ? elem->fd : -1; >> + >> + if (old_fd < 0) { >> + cpr_save_fd(name, id, fd); >> + } else if (old_fd != fd) { >> + error_setg(&error_fatal, >> + "internal error: cpr fd '%s' id %d value %d " >> + "already saved with a different value %d", >> + name, id, fd, old_fd); > > How bad it is to trigger this? Bad, cpr will likely fail the next time it is used. I suppose I could add a blocker instead of using error_fatal. But, fundamentally something unknown has gone wrong, like for any assertion failure, so continuing to run in an uncertain state seems unwise. I have only ever seen this during development after adding buggy code. > I wonder if cpr_save_fd() should have checked this already on duplicated > entries; it looks risky there too if this happens to existing cpr_save_fd() > callers. Yes, I could check for dups in cpr_save_fd, though it would cost O(N) instead of O(1). That seems like overkill for a bug that should only bite during new code development. cpr_resave_fd is O(N), but not for error checking. Callers use it when they know the fd was (or may have been) already created. It is a programming convenience that simplifies the call sites. >> + } >> +} >> + >> +int cpr_open_fd(const char *path, int flags, const char *name, int id, >> + bool *reused, Error **errp) >> +{ >> + int fd = cpr_find_fd(name, id); >> + >> + if (reused) { >> + *reused = (fd >= 0); >> + } >> + if (fd < 0) { >> + fd = qemu_open(path, flags, errp); >> + if (fd >= 0) { >> + cpr_save_fd(name, id, fd); >> + } >> + } >> + return fd; >> +} >> + >> /*************************************************************************/ >> #define CPR_STATE "CprState" >> >> @@ -128,6 +161,11 @@ void cpr_set_incoming_mode(MigMode mode) >> incoming_mode = mode; >> } >> >> +bool cpr_is_incoming(void) >> +{ >> + return incoming_mode != MIG_MODE_NONE; >> +} > > Maybe it'll be helpful to document either this function or incoming_mode; > it's probably not yet obvious to most readers incoming_mode is only set to > !NONE during a small window when VM loads. OK, I'll add a function header comment. >> + >> int cpr_state_save(MigrationChannel *channel, Error **errp) >> { >> int ret; >> @@ -222,3 +260,9 @@ void cpr_state_close(void) >> cpr_state_file = NULL; >> } >> } >> + >> +bool cpr_needed_for_reuse(void *opaque) >> +{ >> + MigMode mode = migrate_mode(); > > Nit: can drop the var. OK. - Steve > >> + return mode == MIG_MODE_CPR_TRANSFER; >> +} >> -- >> 1.8.3.1 >> >
On Fri, Feb 14, 2025 at 03:31:29PM -0500, Steven Sistare wrote: > On 2/14/2025 11:37 AM, Peter Xu wrote: > > On Fri, Feb 14, 2025 at 06:13:44AM -0800, Steve Sistare wrote: > > > Add cpr_needed_for_reuse, cpr_resave_fd helpers, cpr_is_incoming, and > > > cpr_open_fd, for use when adding cpr support for vfio and iommufd. > > > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > > --- > > > include/migration/cpr.h | 6 ++++++ > > > migration/cpr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 50 insertions(+) > > > > > > diff --git a/include/migration/cpr.h b/include/migration/cpr.h > > > index 3a6deb7..6ad04d4 100644 > > > --- a/include/migration/cpr.h > > > +++ b/include/migration/cpr.h > > > @@ -18,15 +18,21 @@ > > > void cpr_save_fd(const char *name, int id, int fd); > > > void cpr_delete_fd(const char *name, int id); > > > int cpr_find_fd(const char *name, int id); > > > +void cpr_resave_fd(const char *name, int id, int fd); > > > +int cpr_open_fd(const char *path, int flags, const char *name, int id, > > > + bool *reused, Error **errp); > > > MigMode cpr_get_incoming_mode(void); > > > void cpr_set_incoming_mode(MigMode mode); > > > +bool cpr_is_incoming(void); > > > int cpr_state_save(MigrationChannel *channel, Error **errp); > > > int cpr_state_load(MigrationChannel *channel, Error **errp); > > > void cpr_state_close(void); > > > struct QIOChannel *cpr_state_ioc(void); > > > +bool cpr_needed_for_reuse(void *opaque); > > > + > > > QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp); > > > QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp); > > > diff --git a/migration/cpr.c b/migration/cpr.c > > > index 584b0b9..12c489b 100644 > > > --- a/migration/cpr.c > > > +++ b/migration/cpr.c > > > @@ -95,6 +95,39 @@ int cpr_find_fd(const char *name, int id) > > > trace_cpr_find_fd(name, id, fd); > > > return fd; > > > } > > > + > > > +void cpr_resave_fd(const char *name, int id, int fd) > > > +{ > > > + CprFd *elem = find_fd(&cpr_state.fds, name, id); > > > + int old_fd = elem ? elem->fd : -1; > > > + > > > + if (old_fd < 0) { > > > + cpr_save_fd(name, id, fd); > > > + } else if (old_fd != fd) { > > > + error_setg(&error_fatal, > > > + "internal error: cpr fd '%s' id %d value %d " > > > + "already saved with a different value %d", > > > + name, id, fd, old_fd); > > > > How bad it is to trigger this? > > Bad, cpr will likely fail the next time it is used. > I suppose I could add a blocker instead of using error_fatal. > But, fundamentally something unknown has gone wrong, like for > any assertion failure, so continuing to run in an uncertain > state seems unwise. > > I have only ever seen this during development after adding buggy code. > > > I wonder if cpr_save_fd() should have checked this already on duplicated > > entries; it looks risky there too if this happens to existing cpr_save_fd() > > callers. > > Yes, I could check for dups in cpr_save_fd, though it would cost O(N) instead > of O(1). That seems like overkill for a bug that should only bite during new > code development. > > cpr_resave_fd is O(N), but not for error checking. Callers use it when they > know the fd was (or may have been) already created. It is a programming > convenience that simplifies the call sites. If the caller know the fd was created, then IIUC the caller shouldn't invoke the call. For the other case, could you give an example when the caller may have been created, but maybe not? I'm a bit surprised we have such use case.
diff --git a/include/migration/cpr.h b/include/migration/cpr.h index 3a6deb7..6ad04d4 100644 --- a/include/migration/cpr.h +++ b/include/migration/cpr.h @@ -18,15 +18,21 @@ void cpr_save_fd(const char *name, int id, int fd); void cpr_delete_fd(const char *name, int id); int cpr_find_fd(const char *name, int id); +void cpr_resave_fd(const char *name, int id, int fd); +int cpr_open_fd(const char *path, int flags, const char *name, int id, + bool *reused, Error **errp); MigMode cpr_get_incoming_mode(void); void cpr_set_incoming_mode(MigMode mode); +bool cpr_is_incoming(void); int cpr_state_save(MigrationChannel *channel, Error **errp); int cpr_state_load(MigrationChannel *channel, Error **errp); void cpr_state_close(void); struct QIOChannel *cpr_state_ioc(void); +bool cpr_needed_for_reuse(void *opaque); + QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp); QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp); diff --git a/migration/cpr.c b/migration/cpr.c index 584b0b9..12c489b 100644 --- a/migration/cpr.c +++ b/migration/cpr.c @@ -95,6 +95,39 @@ int cpr_find_fd(const char *name, int id) trace_cpr_find_fd(name, id, fd); return fd; } + +void cpr_resave_fd(const char *name, int id, int fd) +{ + CprFd *elem = find_fd(&cpr_state.fds, name, id); + int old_fd = elem ? elem->fd : -1; + + if (old_fd < 0) { + cpr_save_fd(name, id, fd); + } else if (old_fd != fd) { + error_setg(&error_fatal, + "internal error: cpr fd '%s' id %d value %d " + "already saved with a different value %d", + name, id, fd, old_fd); + } +} + +int cpr_open_fd(const char *path, int flags, const char *name, int id, + bool *reused, Error **errp) +{ + int fd = cpr_find_fd(name, id); + + if (reused) { + *reused = (fd >= 0); + } + if (fd < 0) { + fd = qemu_open(path, flags, errp); + if (fd >= 0) { + cpr_save_fd(name, id, fd); + } + } + return fd; +} + /*************************************************************************/ #define CPR_STATE "CprState" @@ -128,6 +161,11 @@ void cpr_set_incoming_mode(MigMode mode) incoming_mode = mode; } +bool cpr_is_incoming(void) +{ + return incoming_mode != MIG_MODE_NONE; +} + int cpr_state_save(MigrationChannel *channel, Error **errp) { int ret; @@ -222,3 +260,9 @@ void cpr_state_close(void) cpr_state_file = NULL; } } + +bool cpr_needed_for_reuse(void *opaque) +{ + MigMode mode = migrate_mode(); + return mode == MIG_MODE_CPR_TRANSFER; +}
Add cpr_needed_for_reuse, cpr_resave_fd helpers, cpr_is_incoming, and cpr_open_fd, for use when adding cpr support for vfio and iommufd. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/migration/cpr.h | 6 ++++++ migration/cpr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)