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

[Maciej S. Szmigiero]

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.

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

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

};

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.

And multifd_join_device_state_save_threads() needs to access
send_threads_ret.

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.

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.

Thanks,

Thanks,
Maciej