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

[Leonardo Brás]

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.
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.

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?

Best regards, 
Leo