Re: [PATCH v4 19/33] migration: Add save_live_complete_precopy_thread handler

[Peter Xu <peterx@redhat.com>]

On Tue, Feb 04, 2025 at 08:32:15PM +0100, Maciej S. Szmigiero wrote:
> On 4.02.2025 18:54, Peter Xu wrote:
> > On Thu, Jan 30, 2025 at 11:08:40AM +0100, Maciej S. Szmigiero wrote:
> > > +static int multifd_device_state_save_thread(void *opaque)
> > > +{
> > > +    struct MultiFDDSSaveThreadData *data = opaque;
> > > +    int ret;
> > > +
> > > +    ret = data->hdlr(data->idstr, data->instance_id, &send_threads_abort,
> > > +                     data->handler_opaque);
> > 
> > I thought we discussed somewhere and the plan was we could use Error** here
> > to report errors.  Would that still make sense, or maybe I lost some
> > context?
> 
> That was about *load* threads, here these are *save* threads.

Ah OK.

> 
> Save handlers do not return an Error value, neither save_live_iterate, nor
> save_live_complete_precopy or save_state does so.

Let's try to make new APIs work with Error* if possible.

> 
> > Meanwhile, I still feel uneasy on having these globals (send_threads_abort,
> > send_threads_ret).  Can we make MultiFDDSSaveThreadData the only interface
> > between migration and the threads impl?  So I wonder if it can be:
> > 
> >    ret = data->hdlr(data);
> > 
> > With extended struct like this (I added thread_error and thread_quit):
> > 
> > struct MultiFDDSSaveThreadData {
> >      SaveLiveCompletePrecopyThreadHandler hdlr;
> >      char *idstr;
> >      uint32_t instance_id;
> >      void *handler_opaque;
> >      /*
> >       * Should be NULL when struct passed over to thread, the thread should
> >       * set this if the handler would return false.  It must be kept NULL if
> >       * the handler returned true / success.
> >       */
> >      Error *thread_error;
> 
> As I mentioned above, these handlers do not generally return Error type,
> so this would need to be an *int;
> 
> >      /*
> >       * Migration core would set this when it wants to notify thread to
> >       * quit, for example, when error occured in other threads, or migration is
> >       * cancelled by the user.
> >       */
> >      bool thread_quit;
> 
>             ^ I guess that was supposed to be a pointer too (*thread_quit).

It's my intention to make this bool, to make everything managed per-thread.

It's actually what we do with multifd, these are a bunch of extra threads
to differeciate from the "IO threads" / "multifd threads".

> 
> > };
> > 
> > Then if any multifd_device_state_save_thread() failed, for example, it
> > should notify all threads to quit by setting thread_quit, instead of
> > relying on yet another global variable to show migration needs to quit.
> 
> multifd_abort_device_state_save_threads() needs to access
> send_threads_abort too.

This may need to become something like:

  QLIST_FOREACH() {
      MultiFDDSSaveThreadData *data = ...;
      data->thread_quit = true;
  }

We may want to double check qmp 'migrate_cancel' will work when save
threads are running, but this can also be done for later.

> 
> And multifd_join_device_state_save_threads() needs to access
> send_threads_ret.

Then this one becomes:

  thread_pool_wait(send_threads);
  QLIST_FOREACH() {
      MultiFDDSSaveThreadData *data = ...;
      if (data->thread_error) {
         return false;
      }
  }
  return true;

> 
> These variables ultimately will have to be stored somewhere since
> there can be multiple save threads and so multiple instances of
> MultiFDDSSaveThreadData.
> 
> So these need to be stored somewhere where
> multifd_spawn_device_state_save_thread() can reach them to assign
> their addresses to MultiFDDSSaveThreadData members.

Then multifd_spawn_device_state_save_thread() will need to manage the
qlist, making sure migration core remembers what jobs it submitted.  It
sounds good to have that bookkeeping when I think about it, instead of
throw the job to the thread pool and forget it..

> 
> However, at that point multifd_device_state_save_thread() can
> access them too so it does not need to have them passed via
> MultiFDDSSaveThreadData.
> 
> However, nothing prevents putting send_threads* variables
> into a global struct (with internal linkage - "static", just as
> these separate ones are) if you like such construct more.

This should be better than the current global vars indeed, but less
favoured if the per-thread way could work above.

Thanks,

-- 
Peter Xu