| Message ID | 6976f129df610c8207da4e531c8c0475ec204fa4.1730203967.git.maciej.szmigiero@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Trivial patches from multifd device state transfer support patch set | expand |
"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>
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 > * >
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
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,
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 *