On Fri, Aug 19, 2022 at 6:52 AM Juan Quintela <quint...@redhat.com> wrote:
>
> Leonardo Brás <leob...@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 <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.
>
> 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.
>
