Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer

[Leonardo Bras Soares Passos]

On Fri, Aug 19, 2022 at 6:52 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> We are going to create a new function for multifd latest in the series.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > Double Signed-off-by again.
> >
> >> ---
> >>  migration/ram.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 85d89d61ac..499d9b2a90 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -310,6 +310,9 @@ typedef struct {
> >>      bool preempted;
> >>  } PostcopyPreemptState;
> >>
> >> +typedef struct RAMState RAMState;
> >> +typedef struct PageSearchStatus PageSearchStatus;
> >> +
> >>  /* State of RAM for migration */
> >>  struct RAMState {
> >>      /* QEMUFile used for this migration */
> >> @@ -372,8 +375,9 @@ struct RAMState {
> >>       * is enabled.
> >>       */
> >>      unsigned int postcopy_channel;
> >> +
> >> +    int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> >>  };
> >> -typedef struct RAMState RAMState;
> >>
> >>  static RAMState *ram_state;
> >>
> >> @@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs, 
> >> RAMBlock *block, ram_addr_t offset)
> >>  }
> >>
> >>  /**
> >> - * ram_save_target_page: save one target page
> >> + * ram_save_target_page_legacy: save one target page
> >>   *
> >>   * Returns the number of pages written
> >>   *
> >>   * @rs: current RAM state
> >>   * @pss: data about the page we want to send
> >>   */
> >> -static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
> >> +static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus 
> >> *pss)
> >>  {
> >>      RAMBlock *block = pss->block;
> >>      ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> >> @@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs, 
> >> PageSearchStatus *pss)
> >>
> >>          /* Check the pages is dirty and if it is send it */
> >>          if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> >> -            tmppages = ram_save_target_page(rs, pss);
> >> +            tmppages = rs->ram_save_target_page(rs, pss);
> >>              if (tmppages < 0) {
> >>                  return tmppages;
> >>              }
> >> @@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >>      ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> >>      ram_control_after_iterate(f, RAM_CONTROL_SETUP);
> >>
> >> +    (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> >>      ret =  multifd_send_sync_main(f);
> >>      if (ret < 0) {
> >>          return ret;
> >
> >
> > So, IIUC:
> > - Rename ram_save_target_page -> ram_save_target_page_legacy
> > - Add a function pointer to RAMState (or a callback)
> > - Assign function pointer = ram_save_target_page_legacy at setup
> > - Replace ram_save_target_page() by indirect function call using above 
> > pointer.
> >
> > I could see no issue in this, so I belive it works fine.
> >
> > The only thing that concerns me is the name RAMState.
>
> Every device state is setup in RAMState.
>
> > IMHO, a struct named RAMState is supposed to just reflect the state of ram 
> > (or
> > according to this struct's comments, the state of RAM for migration. Having 
> > a
> > function pointer here that saves a page seems counterintuitive, since it 
> > does
> > not reflect the state of RAM.
>
> The big problem for adding another struct is that we would have to
> change all the callers, or yet another global variable.  Both are bad
> idea in my humble opinion.
>
> > Maybe we could rename the struct, or even better, create another struct that
> > could look something like this:
> >
> > struct RAMMigration {
> >     RAMState state;
> >     int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> >     /* Other callbacks or further info.*/
> > }
> >
> > What do you think about it?
>
> Really this depends on configuration.  What is setup for qemu
> migration.  I think this is the easiest way to do it, we can add a new
> struct, but it gets everything much more complicated:
>
> - the value that we receive in ram_save_setup() is a RAMState
> - We would have to change all the callers form
>   * ram_save_iterate()
>   * ram_find_and_save_block()
>   * ram_save_host_page()

Maybe RAMState could be part of a bigger struct, and we could use
something like a container_of().
So whenever you want to use it, it would be available.

What about that?

>
> So I think it is quite a bit of churn for not a lot of gain.
>
> Later, Juan.
>