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 <quint...@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> Signed-off-by: Juan Quintela <quint...@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
