Trivial patches from multifd device state transfer support patch set

[PATCH 4/4] migration: Document the BQL behavior of load SaveVMHandlers

Posted by Maciej S. Szmigiero 3 weeks, 4 days ago

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

Some of these SaveVMHandlers were missing the BQL behavior annotation,
making people wonder what it exactly is.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 include/migration/register.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/migration/register.h b/include/migration/register.h
index f60e797894e5..c411d84876ec 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -210,6 +210,8 @@ typedef struct SaveVMHandlers {
     void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
                                 uint64_t *can_postcopy);
 
+    /* This runs inside the BQL. */
+
     /**
      * @load_state
      *
@@ -227,6 +229,8 @@ typedef struct SaveVMHandlers {
      */
     int (*load_state)(QEMUFile *f, void *opaque, int version_id);
 
+    /* The following handlers run inside the BQL. */
+
     /**
      * @load_setup
      *

Re: [PATCH 4/4] migration: Document the BQL behavior of load SaveVMHandlers

Posted by Peter Xu 3 weeks, 3 days ago

On Tue, Oct 29, 2024 at 03:58:16PM +0100, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> Some of these SaveVMHandlers were missing the BQL behavior annotation,
> making people wonder what it exactly is.
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>  include/migration/register.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index f60e797894e5..c411d84876ec 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -210,6 +210,8 @@ typedef struct SaveVMHandlers {
>      void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>                                  uint64_t *can_postcopy);
>  
> +    /* This runs inside the BQL. */
> +
>      /**
>       * @load_state
>       *
> @@ -227,6 +229,8 @@ typedef struct SaveVMHandlers {
>       */
>      int (*load_state)(QEMUFile *f, void *opaque, int version_id);
>  
> +    /* The following handlers run inside the BQL. */

If above also requires BQL, why not move this line upper?

OTOH, I think resume_prepare() doesn't require BQL..

> +
>      /**
>       * @load_setup
>       *
> 

-- 
Peter Xu

Re: [PATCH 4/4] migration: Document the BQL behavior of load SaveVMHandlers

Posted by Maciej S. Szmigiero 3 weeks, 3 days ago

On 29.10.2024 21:35, Peter Xu wrote:
> On Tue, Oct 29, 2024 at 03:58:16PM +0100, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> Some of these SaveVMHandlers were missing the BQL behavior annotation,
>> making people wonder what it exactly is.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> ---
>>   include/migration/register.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/migration/register.h b/include/migration/register.h
>> index f60e797894e5..c411d84876ec 100644
>> --- a/include/migration/register.h
>> +++ b/include/migration/register.h
>> @@ -210,6 +210,8 @@ typedef struct SaveVMHandlers {
>>       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>>                                   uint64_t *can_postcopy);
>>   
>> +    /* This runs inside the BQL. */
>> +
>>       /**
>>        * @load_state
>>        *
>> @@ -227,6 +229,8 @@ typedef struct SaveVMHandlers {
>>        */
>>       int (*load_state)(QEMUFile *f, void *opaque, int version_id);
>>   
>> +    /* The following handlers run inside the BQL. */
> 
> If above also requires BQL, why not move this line upper?

The reason for this is that my main patch set also adds
"load_state_buffer" handler, which runs without BQL.

That handler is ordered next after "load_state" and I tried
to avoid further comment churn here.

But if you prefer to change these comments in the patch
introducing "load_state_buffer" handler instead then it's
fine.
  
> OTOH, I think resume_prepare() doesn't require BQL..

Yes, it seems like resume_prepare() is only called outside BQL.
Will update the patch.

Thanks,
Maciej

Re: [PATCH 4/4] migration: Document the BQL behavior of load SaveVMHandlers

Posted by Peter Xu 3 weeks, 3 days ago

On Tue, Oct 29, 2024 at 09:46:01PM +0100, Maciej S. Szmigiero wrote:
> On 29.10.2024 21:35, Peter Xu wrote:
> > On Tue, Oct 29, 2024 at 03:58:16PM +0100, Maciej S. Szmigiero wrote:
> > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > 
> > > Some of these SaveVMHandlers were missing the BQL behavior annotation,
> > > making people wonder what it exactly is.
> > > 
> > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > ---
> > >   include/migration/register.h | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/migration/register.h b/include/migration/register.h
> > > index f60e797894e5..c411d84876ec 100644
> > > --- a/include/migration/register.h
> > > +++ b/include/migration/register.h
> > > @@ -210,6 +210,8 @@ typedef struct SaveVMHandlers {
> > >       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
> > >                                   uint64_t *can_postcopy);
> > > +    /* This runs inside the BQL. */
> > > +
> > >       /**
> > >        * @load_state
> > >        *
> > > @@ -227,6 +229,8 @@ typedef struct SaveVMHandlers {
> > >        */
> > >       int (*load_state)(QEMUFile *f, void *opaque, int version_id);
> > > +    /* The following handlers run inside the BQL. */
> > 
> > If above also requires BQL, why not move this line upper?
> 
> The reason for this is that my main patch set also adds
> "load_state_buffer" handler, which runs without BQL.
> 
> That handler is ordered next after "load_state" and I tried
> to avoid further comment churn here.
> 
> But if you prefer to change these comments in the patch
> introducing "load_state_buffer" handler instead then it's
> fine.

Considering the current change is so small to start benefit readers.. I
think it's ok we do this in one shot after load_state_buffer() change.

> > OTOH, I think resume_prepare() doesn't require BQL..
> 
> Yes, it seems like resume_prepare() is only called outside BQL.
> Will update the patch.

Thanks,

-- 
Peter Xu

Re: [PATCH 4/4] migration: Document the BQL behavior of load SaveVMHandlers

Posted by Fabiano Rosas 3 weeks, 3 days ago

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> Some of these SaveVMHandlers were missing the BQL behavior annotation,
> making people wonder what it exactly is.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>