subject:"Re\: \[PATCH V1 03\/26\] migration\: SAVEVM

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

2024-05-27 Thread Peter Xu

On Mon, May 13, 2024 at 03:27:30PM -0400, Steven Sistare wrote:
> On 5/6/2024 7:17 PM, Fabiano Rosas wrote:
> > Steve Sistare  writes:
> > 
> > > Define an abstraction SAVEVM_FOREACH to loop over all savevm state
> > > handlers, and replace QTAILQ_FOREACH.  Define variants for ALL so
> > > we can loop over all handlers vs a subset of handlers in a subsequent
> > > patch, but at this time there is no distinction between the two.
> > > No functional change.
> > > 
> > > Signed-off-by: Steve Sistare 
> > > ---
> > >   migration/savevm.c | 55 
> > > +++---
> > >   1 file changed, 32 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 4509482..6829ba3 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -237,6 +237,15 @@ static SaveState savevm_state = {
> > >   .global_section_id = 0,
> > >   };
> > > +#define SAVEVM_FOREACH(se, entry)\
> > > +QTAILQ_FOREACH(se, &savevm_state.handlers, entry)\
> > > +
> > > +#define SAVEVM_FOREACH_ALL(se, entry)\
> > > +QTAILQ_FOREACH(se, &savevm_state.handlers, entry)
> > 
> > This feels worse than SAVEVM_FOREACH_NOT_PRECREATED. We'll have to keep
> > coming back to the definition to figure out which FOREACH is the real
> > deal.
> 
> I take your point, but the majority of the loops do not care about precreated
> objects, so it seems backwards to make them more verbose with
> SAVEVM_FOREACH_NOT_PRECREATE.  I can go either way, but we need
> Peter's opinion also.

I don't have a strong opinion yet on the name, I think it'll be clearer
indeed when the _ALL() (or whatever other name) is used only when with the
real users.

OTOH, besides the name (which is much more trivial..) the precreated idea
in general is a bit scary to me.. if that was for a "workaround" to some
new ordering issue due to newly added dependencies on exec() support.
Maybe I'll understand better when I get to know better on the whole design.

-- 
Peter Xu

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

2024-05-13 Thread Steven Sistare


On 5/6/2024 7:17 PM, Fabiano Rosas wrote:

Steve Sistare  writes:


Define an abstraction SAVEVM_FOREACH to loop over all savevm state
handlers, and replace QTAILQ_FOREACH.  Define variants for ALL so
we can loop over all handlers vs a subset of handlers in a subsequent
patch, but at this time there is no distinction between the two.
No functional change.

Signed-off-by: Steve Sistare 
---
  migration/savevm.c | 55 +++---
  1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 4509482..6829ba3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -237,6 +237,15 @@ static SaveState savevm_state = {
  .global_section_id = 0,
  };
  
+#define SAVEVM_FOREACH(se, entry)\

+QTAILQ_FOREACH(se, &savevm_state.handlers, entry)\
+
+#define SAVEVM_FOREACH_ALL(se, entry)\
+QTAILQ_FOREACH(se, &savevm_state.handlers, entry)


This feels worse than SAVEVM_FOREACH_NOT_PRECREATED. We'll have to keep
coming back to the definition to figure out which FOREACH is the real
deal.


I take your point, but the majority of the loops do not care about precreated
objects, so it seems backwards to make them more verbose with 
SAVEVM_FOREACH_NOT_PRECREATE.  I can go either way, but we need

Peter's opinion also.


+
+#define SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se)   \
+QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se)
+
  static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
  
  static bool should_validate_capability(int capability)

@@ -674,7 +683,7 @@ static uint32_t calculate_new_instance_id(const char *idstr)
  SaveStateEntry *se;
  uint32_t instance_id = 0;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH_ALL(se, entry) {


In this patch we can't have both instances...


  if (strcmp(idstr, se->idstr) == 0
  && instance_id <= se->instance_id) {
  instance_id = se->instance_id + 1;
@@ -690,7 +699,7 @@ static int calculate_compat_instance_id(const char *idstr)
  SaveStateEntry *se;
  int instance_id = 0;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH(se, entry) {


...otherwise one of the two changes will go undocumented because the
actual reason for it will only be described in the next patch.


Sure, I'll move this to the precreate patch.

- Steve


  if (!se->compat) {
  continue;
  }
@@ -816,7 +825,7 @@ void unregister_savevm(VMStateIf *obj, const char *idstr, 
void *opaque)
  }
  pstrcat(id, sizeof(id), idstr);
  
-QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {

+SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
  if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
  savevm_state_handler_remove(se);
  g_free(se->compat);
@@ -939,7 +948,7 @@ void vmstate_unregister(VMStateIf *obj, const 
VMStateDescription *vmsd,
  {
  SaveStateEntry *se, *new_se;
  
-QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {

+SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
  if (se->vmsd == vmsd && se->opaque == opaque) {
  savevm_state_handler_remove(se);
  g_free(se->compat);
@@ -1223,7 +1232,7 @@ bool qemu_savevm_state_blocked(Error **errp)
  {
  SaveStateEntry *se;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH(se, entry) {
  if (se->vmsd && se->vmsd->unmigratable) {
  error_setg(errp, "State blocked by non-migratable device '%s'",
 se->idstr);
@@ -1237,7 +1246,7 @@ void qemu_savevm_non_migratable_list(strList **reasons)
  {
  SaveStateEntry *se;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH(se, entry) {
  if (se->vmsd && se->vmsd->unmigratable) {
  QAPI_LIST_PREPEND(*reasons,
g_strdup_printf("non-migratable device: %s",
@@ -1276,7 +1285,7 @@ bool qemu_savevm_state_guest_unplug_pending(void)
  {
  SaveStateEntry *se;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH(se, entry) {
  if (se->vmsd && se->vmsd->dev_unplug_pending &&
  se->vmsd->dev_unplug_pending(se->opaque)) {
  return true;
@@ -1291,7 +1300,7 @@ int qemu_savevm_state_prepare(Error **errp)
  SaveStateEntry *se;
  int ret;
  
-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {

+SAVEVM_FOREACH(se, entry) {
  if (!se->ops || !se->ops->save_prepare) {
  continue;
  }
@@ -1321,7 +1330,7 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
  json_writer_start_array(ms->vmdesc, "devices");
  
  trace_savevm_state_setup();

-QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

2024-05-06 Thread Fabiano Rosas

Steve Sistare  writes:

> Define an abstraction SAVEVM_FOREACH to loop over all savevm state
> handlers, and replace QTAILQ_FOREACH.  Define variants for ALL so
> we can loop over all handlers vs a subset of handlers in a subsequent
> patch, but at this time there is no distinction between the two.
> No functional change.
>
> Signed-off-by: Steve Sistare 
> ---
>  migration/savevm.c | 55 
> +++---
>  1 file changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4509482..6829ba3 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -237,6 +237,15 @@ static SaveState savevm_state = {
>  .global_section_id = 0,
>  };
>  
> +#define SAVEVM_FOREACH(se, entry)\
> +QTAILQ_FOREACH(se, &savevm_state.handlers, entry)\
> +
> +#define SAVEVM_FOREACH_ALL(se, entry)\
> +QTAILQ_FOREACH(se, &savevm_state.handlers, entry)

This feels worse than SAVEVM_FOREACH_NOT_PRECREATED. We'll have to keep
coming back to the definition to figure out which FOREACH is the real
deal.

> +
> +#define SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se)   \
> +QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se)
> +
>  static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
>  
>  static bool should_validate_capability(int capability)
> @@ -674,7 +683,7 @@ static uint32_t calculate_new_instance_id(const char 
> *idstr)
>  SaveStateEntry *se;
>  uint32_t instance_id = 0;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH_ALL(se, entry) {

In this patch we can't have both instances...

>  if (strcmp(idstr, se->idstr) == 0
>  && instance_id <= se->instance_id) {
>  instance_id = se->instance_id + 1;
> @@ -690,7 +699,7 @@ static int calculate_compat_instance_id(const char *idstr)
>  SaveStateEntry *se;
>  int instance_id = 0;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {

...otherwise one of the two changes will go undocumented because the
actual reason for it will only be described in the next patch.

>  if (!se->compat) {
>  continue;
>  }
> @@ -816,7 +825,7 @@ void unregister_savevm(VMStateIf *obj, const char *idstr, 
> void *opaque)
>  }
>  pstrcat(id, sizeof(id), idstr);
>  
> -QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {
> +SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
>  if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
>  savevm_state_handler_remove(se);
>  g_free(se->compat);
> @@ -939,7 +948,7 @@ void vmstate_unregister(VMStateIf *obj, const 
> VMStateDescription *vmsd,
>  {
>  SaveStateEntry *se, *new_se;
>  
> -QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {
> +SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
>  if (se->vmsd == vmsd && se->opaque == opaque) {
>  savevm_state_handler_remove(se);
>  g_free(se->compat);
> @@ -1223,7 +1232,7 @@ bool qemu_savevm_state_blocked(Error **errp)
>  {
>  SaveStateEntry *se;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {
>  if (se->vmsd && se->vmsd->unmigratable) {
>  error_setg(errp, "State blocked by non-migratable device '%s'",
> se->idstr);
> @@ -1237,7 +1246,7 @@ void qemu_savevm_non_migratable_list(strList **reasons)
>  {
>  SaveStateEntry *se;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {
>  if (se->vmsd && se->vmsd->unmigratable) {
>  QAPI_LIST_PREPEND(*reasons,
>g_strdup_printf("non-migratable device: %s",
> @@ -1276,7 +1285,7 @@ bool qemu_savevm_state_guest_unplug_pending(void)
>  {
>  SaveStateEntry *se;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {
>  if (se->vmsd && se->vmsd->dev_unplug_pending &&
>  se->vmsd->dev_unplug_pending(se->opaque)) {
>  return true;
> @@ -1291,7 +1300,7 @@ int qemu_savevm_state_prepare(Error **errp)
>  SaveStateEntry *se;
>  int ret;
>  
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {
>  if (!se->ops || !se->ops->save_prepare) {
>  continue;
>  }
> @@ -1321,7 +1330,7 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>  json_writer_start_array(ms->vmdesc, "devices");
>  
>  trace_savevm_state_setup();
> -QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +SAVEVM_FOREACH(se, entry) {
>  if (se->vmsd && se->vmsd->early_setup) {
>  ret = vmstate_save(f, se, ms->vmdesc, errp);
>  if (ret) {
> @@ -1365,7

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

3 matches

Site Navigation

Mail list logo

Footer information