[PATCH 1/1] migration/cpr: lock the cpr state

[PATCH 1/1] migration/cpr: lock the cpr state

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

The cpr state is a process wide registry of the resources that have to
survive an exec. Entries are added and dropped from realize and teardown
paths all over the tree, and a caller has no way to tell which thread the
next caller will run in.

The state carries no serialization of its own. It is safe today only
because its callers happen to hold the BQL, and that is a property of
where those callers live rather than anything the interface promises or
checks. A caller that is correct within its own subsystem can therefore
break this one from a distance: two that overlap corrupt the registry,
and the next lookup walks an entry that has already been freed.

Give the state a lock of its own instead of relying on that. Entries
move when a device is realized, when a resource is handed over, and when
a migration starts, so none of this is on a hot path and an uncontended
mutex costs nothing.

Take the lock in every accessor and across the save and load walks.
Holding it over the walk callbacks is safe because none of them comes
back through this interface. State that is not reached through these
accessors, such as the VFIO device list that hw/vfio manipulates
directly, stays outside it.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Cc: Mark Kanda <mark.kanda@oracle.com>
Cc: Ben Chaney <bchaney@akamai.com>
Cc: Steve Sistare <steven.sistare@oracle.com>
---
 migration/cpr.c | 59 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 10 deletions(-)

diff --git a/migration/cpr.c b/migration/cpr.c
index bca43e9bf3..503057e262 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -9,6 +9,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/lockable.h"
 #include "hw/vfio/vfio-cpr.h"
 #include "migration/cpr.h"
 #include "migration/misc.h"
@@ -26,6 +27,14 @@
 CprState cpr_state;
 static GHashTable *cpr_fds_hash;
 
+/* Callers are spread across the tree and need not share a thread. */
+static QemuMutex cpr_fds_lock;
+
+static void __attribute__((constructor)) cpr_fds_lock_init(void)
+{
+    qemu_mutex_init(&cpr_fds_lock);
+}
+
 /****************************************************************************/
 
 typedef struct CprFd {
@@ -94,6 +103,8 @@ static int cpr_fd_pre_save(void *opaque)
     GHashTableIter iter;
     CprFd *elem;
 
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
     QLIST_INIT(&state->fds);
 
     g_hash_table_iter_init(&iter, get_cpr_fds_hash());
@@ -109,6 +120,8 @@ static int cpr_fd_post_load(void *opaque, int version_id)
     CprState *state = (CprState *)opaque;
     CprFd *elem;
 
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
     while ((elem = QLIST_FIRST(&state->fds))) {
         QLIST_REMOVE(elem, next);
 
@@ -127,7 +140,7 @@ static int cpr_fd_post_load(void *opaque, int version_id)
     return 0;
 }
 
-void cpr_save_fd(const char *name, int id, int fd)
+static void do_cpr_save_fd(const char *name, int id, int fd)
 {
     CprFd *elem = g_new0(CprFd, 1);
 
@@ -139,6 +152,13 @@ void cpr_save_fd(const char *name, int id, int fd)
     cpr_fd_hash_insert(elem);
 }
 
+void cpr_save_fd(const char *name, int id, int fd)
+{
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
+    do_cpr_save_fd(name, id, fd);
+}
+
 static CprFd *find_fd(const char *name, int id)
 {
     CprFd key = {
@@ -149,6 +169,15 @@ static CprFd *find_fd(const char *name, int id)
     return g_hash_table_lookup(get_cpr_fds_hash(), &key);
 }
 
+static int do_cpr_find_fd(const char *name, int id)
+{
+    CprFd *elem = find_fd(name, id);
+    int fd = elem ? elem->fd : -1;
+
+    trace_cpr_find_fd(name, id, fd);
+    return fd;
+}
+
 void cpr_delete_fd(const char *name, int id)
 {
     CprFd key = {
@@ -156,6 +185,8 @@ void cpr_delete_fd(const char *name, int id)
         .id = id,
     };
 
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
     g_hash_table_remove(get_cpr_fds_hash(), &key);
 
     trace_cpr_delete_fd(name, id);
@@ -163,20 +194,23 @@ void cpr_delete_fd(const char *name, int id)
 
 int cpr_find_fd(const char *name, int id)
 {
-    CprFd *elem = find_fd(name, id);
-    int fd = elem ? elem->fd : -1;
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
 
-    trace_cpr_find_fd(name, id, fd);
-    return fd;
+    return do_cpr_find_fd(name, id);
 }
 
 void cpr_resave_fd(const char *name, int id, int fd)
 {
-    CprFd *elem = find_fd(name, id);
-    int old_fd = elem ? elem->fd : -1;
+    CprFd *elem;
+    int old_fd;
+
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
+    elem = find_fd(name, id);
+    old_fd = elem ? elem->fd : -1;
 
     if (old_fd < 0) {
-        cpr_save_fd(name, id, fd);
+        do_cpr_save_fd(name, id, fd);
     } else if (old_fd != fd) {
         error_report("internal error: cpr fd '%s' id %d value %d "
                      "already saved with a different value %d",
@@ -188,12 +222,15 @@ void cpr_resave_fd(const char *name, int id, int fd)
 int cpr_open_fd(const char *path, int flags, const char *name, int id,
                 Error **errp)
 {
-    int fd = cpr_find_fd(name, id);
+    int fd;
 
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
+    fd = do_cpr_find_fd(name, id);
     if (fd < 0) {
         fd = qemu_open(path, flags, errp);
         if (fd >= 0) {
-            cpr_save_fd(name, id, fd);
+            do_cpr_save_fd(name, id, fd);
         }
     }
     return fd;
@@ -204,6 +241,8 @@ bool cpr_walk_fd(cpr_walk_fd_cb cb)
     GHashTableIter iter;
     CprFd *elem;
 
+    QEMU_LOCK_GUARD(&cpr_fds_lock);
+
     g_hash_table_iter_init(&iter, get_cpr_fds_hash());
     while (g_hash_table_iter_next(&iter, (gpointer *)&elem, NULL)) {
         g_assert(elem->fd >= 0);
-- 
2.53.0

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Fabiano Rosas]

"Denis V. Lunev" <den@openvz.org> writes:

> From: Denis V. Lunev <den@openvz.org>
>
> The cpr state is a process wide registry of the resources that have to
> survive an exec. Entries are added and dropped from realize and teardown
> paths all over the tree, and a caller has no way to tell which thread the
> next caller will run in.
>
> The state carries no serialization of its own. It is safe today only
> because its callers happen to hold the BQL, and that is a property of
> where those callers live rather than anything the interface promises or
> checks. A caller that is correct within its own subsystem can therefore
> break this one from a distance: two that overlap corrupt the registry,
> and the next lookup walks an entry that has already been freed.
>
> Give the state a lock of its own instead of relying on that. Entries
> move when a device is realized, when a resource is handed over, and when
> a migration starts, so none of this is on a hot path and an uncontended
> mutex costs nothing.
>
> Take the lock in every accessor and across the save and load walks.
> Holding it over the walk callbacks is safe because none of them comes
> back through this interface. State that is not reached through these
> accessors, such as the VFIO device list that hw/vfio manipulates
> directly, stays outside it.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Mark Kanda <mark.kanda@oracle.com>
> Cc: Ben Chaney <bchaney@akamai.com>
> Cc: Steve Sistare <steven.sistare@oracle.com>
> ---
>  migration/cpr.c | 59 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/migration/cpr.c b/migration/cpr.c
> index bca43e9bf3..503057e262 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -9,6 +9,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> +#include "qemu/lockable.h"
>  #include "hw/vfio/vfio-cpr.h"
>  #include "migration/cpr.h"
>  #include "migration/misc.h"
> @@ -26,6 +27,14 @@
>  CprState cpr_state;
>  static GHashTable *cpr_fds_hash;
>  
> +/* Callers are spread across the tree and need not share a thread. */
> +static QemuMutex cpr_fds_lock;
> +
> +static void __attribute__((constructor)) cpr_fds_lock_init(void)
> +{
> +    qemu_mutex_init(&cpr_fds_lock);
> +}
> +
>  /****************************************************************************/
>  
>  typedef struct CprFd {
> @@ -94,6 +103,8 @@ static int cpr_fd_pre_save(void *opaque)
>      GHashTableIter iter;
>      CprFd *elem;
>  
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
>      QLIST_INIT(&state->fds);
>  
>      g_hash_table_iter_init(&iter, get_cpr_fds_hash());
> @@ -109,6 +120,8 @@ static int cpr_fd_post_load(void *opaque, int version_id)
>      CprState *state = (CprState *)opaque;
>      CprFd *elem;
>  
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
>      while ((elem = QLIST_FIRST(&state->fds))) {
>          QLIST_REMOVE(elem, next);
>  
> @@ -127,7 +140,7 @@ static int cpr_fd_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -void cpr_save_fd(const char *name, int id, int fd)
> +static void do_cpr_save_fd(const char *name, int id, int fd)
>  {
>      CprFd *elem = g_new0(CprFd, 1);
>  
> @@ -139,6 +152,13 @@ void cpr_save_fd(const char *name, int id, int fd)
>      cpr_fd_hash_insert(elem);
>  }
>  
> +void cpr_save_fd(const char *name, int id, int fd)
> +{
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
> +    do_cpr_save_fd(name, id, fd);
> +}
> +
>  static CprFd *find_fd(const char *name, int id)
>  {
>      CprFd key = {
> @@ -149,6 +169,15 @@ static CprFd *find_fd(const char *name, int id)
>      return g_hash_table_lookup(get_cpr_fds_hash(), &key);
>  }
>  
> +static int do_cpr_find_fd(const char *name, int id)
> +{
> +    CprFd *elem = find_fd(name, id);
> +    int fd = elem ? elem->fd : -1;
> +
> +    trace_cpr_find_fd(name, id, fd);
> +    return fd;
> +}
> +
>  void cpr_delete_fd(const char *name, int id)
>  {
>      CprFd key = {
> @@ -156,6 +185,8 @@ void cpr_delete_fd(const char *name, int id)
>          .id = id,
>      };
>  
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
>      g_hash_table_remove(get_cpr_fds_hash(), &key);
>  
>      trace_cpr_delete_fd(name, id);
> @@ -163,20 +194,23 @@ void cpr_delete_fd(const char *name, int id)
>  
>  int cpr_find_fd(const char *name, int id)
>  {
> -    CprFd *elem = find_fd(name, id);
> -    int fd = elem ? elem->fd : -1;
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
>  
> -    trace_cpr_find_fd(name, id, fd);
> -    return fd;
> +    return do_cpr_find_fd(name, id);
>  }
>  
>  void cpr_resave_fd(const char *name, int id, int fd)
>  {
> -    CprFd *elem = find_fd(name, id);
> -    int old_fd = elem ? elem->fd : -1;
> +    CprFd *elem;
> +    int old_fd;
> +
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
> +    elem = find_fd(name, id);
> +    old_fd = elem ? elem->fd : -1;
>  
>      if (old_fd < 0) {
> -        cpr_save_fd(name, id, fd);
> +        do_cpr_save_fd(name, id, fd);
>      } else if (old_fd != fd) {
>          error_report("internal error: cpr fd '%s' id %d value %d "
>                       "already saved with a different value %d",
> @@ -188,12 +222,15 @@ void cpr_resave_fd(const char *name, int id, int fd)
>  int cpr_open_fd(const char *path, int flags, const char *name, int id,
>                  Error **errp)
>  {
> -    int fd = cpr_find_fd(name, id);
> +    int fd;
>  
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
> +    fd = do_cpr_find_fd(name, id);
>      if (fd < 0) {
>          fd = qemu_open(path, flags, errp);
>          if (fd >= 0) {
> -            cpr_save_fd(name, id, fd);
> +            do_cpr_save_fd(name, id, fd);
>          }
>      }
>      return fd;
> @@ -204,6 +241,8 @@ bool cpr_walk_fd(cpr_walk_fd_cb cb)
>      GHashTableIter iter;
>      CprFd *elem;
>  
> +    QEMU_LOCK_GUARD(&cpr_fds_lock);
> +
>      g_hash_table_iter_init(&iter, get_cpr_fds_hash());
>      while (g_hash_table_iter_next(&iter, (gpointer *)&elem, NULL)) {
>          g_assert(elem->fd >= 0);

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

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> The cpr state is a process wide registry of the resources that have to
> survive an exec. Entries are added and dropped from realize and teardown
> paths all over the tree, and a caller has no way to tell which thread the
> next caller will run in.
> 
> The state carries no serialization of its own. It is safe today only
> because its callers happen to hold the BQL, and that is a property of
> where those callers live rather than anything the interface promises or
> checks. A caller that is correct within its own subsystem can therefore
> break this one from a distance: two that overlap corrupt the registry,
> and the next lookup walks an entry that has already been freed.
> 
> Give the state a lock of its own instead of relying on that. Entries
> move when a device is realized, when a resource is handed over, and when
> a migration starts, so none of this is on a hot path and an uncontended
> mutex costs nothing.
> 
> Take the lock in every accessor and across the save and load walks.
> Holding it over the walk callbacks is safe because none of them comes
> back through this interface. State that is not reached through these
> accessors, such as the VFIO device list that hw/vfio manipulates
> directly, stays outside it.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Mark Kanda <mark.kanda@oracle.com>
> Cc: Ben Chaney <bchaney@akamai.com>
> Cc: Steve Sistare <steven.sistare@oracle.com>

Is there a real problem anywhere?

My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
creating a new device or so, which should normally need bql already.

To ask in another way, shall we simply assert bql_locked()?

Thanks,

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
>> From: Denis V. Lunev <den@openvz.org>
>> 
>> The cpr state is a process wide registry of the resources that have to
>> survive an exec. Entries are added and dropped from realize and teardown
>> paths all over the tree, and a caller has no way to tell which thread the
>> next caller will run in.
>> 
>> The state carries no serialization of its own. It is safe today only
>> because its callers happen to hold the BQL, and that is a property of
>> where those callers live rather than anything the interface promises or
>> checks. A caller that is correct within its own subsystem can therefore
>> break this one from a distance: two that overlap corrupt the registry,
>> and the next lookup walks an entry that has already been freed.
>> 
>> Give the state a lock of its own instead of relying on that. Entries
>> move when a device is realized, when a resource is handed over, and when
>> a migration starts, so none of this is on a hot path and an uncontended
>> mutex costs nothing.
>> 
>> Take the lock in every accessor and across the save and load walks.
>> Holding it over the walk callbacks is safe because none of them comes
>> back through this interface. State that is not reached through these
>> accessors, such as the VFIO device list that hw/vfio manipulates
>> directly, stays outside it.
>> 
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Fabiano Rosas <farosas@suse.de>
>> Cc: Mark Kanda <mark.kanda@oracle.com>
>> Cc: Ben Chaney <bchaney@akamai.com>
>> Cc: Steve Sistare <steven.sistare@oracle.com>
>
> Is there a real problem anywhere?
>
> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
> creating a new device or so, which should normally need bql already.
>
> To ask in another way, shall we simply assert bql_locked()?
>
> Thanks,

Fair questions, let's see what Denis says.

From my point of view, if the work is already done to use a lock, I'd
take it; one less place for us to bother when dealing with BQL issues.

(on the other hand, I'm not doing any active use or testing of cpr aside
from migration-test, so my opinion is the least important here)

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Wed, Sep 16, 2026 at 03:57:06PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
> >> From: Denis V. Lunev <den@openvz.org>
> >> 
> >> The cpr state is a process wide registry of the resources that have to
> >> survive an exec. Entries are added and dropped from realize and teardown
> >> paths all over the tree, and a caller has no way to tell which thread the
> >> next caller will run in.
> >> 
> >> The state carries no serialization of its own. It is safe today only
> >> because its callers happen to hold the BQL, and that is a property of
> >> where those callers live rather than anything the interface promises or
> >> checks. A caller that is correct within its own subsystem can therefore
> >> break this one from a distance: two that overlap corrupt the registry,
> >> and the next lookup walks an entry that has already been freed.
> >> 
> >> Give the state a lock of its own instead of relying on that. Entries
> >> move when a device is realized, when a resource is handed over, and when
> >> a migration starts, so none of this is on a hot path and an uncontended
> >> mutex costs nothing.
> >> 
> >> Take the lock in every accessor and across the save and load walks.
> >> Holding it over the walk callbacks is safe because none of them comes
> >> back through this interface. State that is not reached through these
> >> accessors, such as the VFIO device list that hw/vfio manipulates
> >> directly, stays outside it.
> >> 
> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Fabiano Rosas <farosas@suse.de>
> >> Cc: Mark Kanda <mark.kanda@oracle.com>
> >> Cc: Ben Chaney <bchaney@akamai.com>
> >> Cc: Steve Sistare <steven.sistare@oracle.com>
> >
> > Is there a real problem anywhere?
> >
> > My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
> > creating a new device or so, which should normally need bql already.
> >
> > To ask in another way, shall we simply assert bql_locked()?
> >
> > Thanks,
> 
> Fair questions, let's see what Denis says.
> 
> From my point of view, if the work is already done to use a lock, I'd
> take it; one less place for us to bother when dealing with BQL issues.
> 
> (on the other hand, I'm not doing any active use or testing of cpr aside
> from migration-test, so my opinion is the least important here)

Yep, I get it.  It's actually sometimes easier to just merge it.

But one thing I want to try to avoid is we introduce a lock, but without
knowing if it's needed.  The problem is for the future: we can't get rid of
the lock easily, exactly because then we also don't know what will start to
break.. so it might be good to ask early than never.

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Maciej S. Szmigiero]

On 16.09.2026 21:37, Peter Xu wrote:
> On Wed, Sep 16, 2026 at 03: 57: 06PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@ redhat. com> writes: > > > On Wed, Sep 16, 2026 at 03: 24: 28PM +0200, Denis V. Lunev wrote: > >> From: Denis V. Lunev <den@ openvz. org>
> ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> This message came from outside your organization.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!NJ2wfiFGNEKGmN-rG1M7U-uqf7hZXHi_TaN9xxKnHvgn7ORtT3LlpKC3H5fsT7Y2H7OClToGDv4H9Y2O0elFWgCyr3UhsTA1fJL9ohOIOE9f$>
> ZjQcmQRYFpfptBannerEnd
> 
> On Wed, Sep 16, 2026 at 03:57:06PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
>> >> From: Denis V. Lunev <den@openvz.org>
>> >> 
>> >> The cpr state is a process wide registry of the resources that have to
>> >> survive an exec. Entries are added and dropped from realize and teardown
>> >> paths all over the tree, and a caller has no way to tell which thread the
>> >> next caller will run in.
>> >> 
>> >> The state carries no serialization of its own. It is safe today only
>> >> because its callers happen to hold the BQL, and that is a property of
>> >> where those callers live rather than anything the interface promises or
>> >> checks. A caller that is correct within its own subsystem can therefore
>> >> break this one from a distance: two that overlap corrupt the registry,
>> >> and the next lookup walks an entry that has already been freed.
>> >> 
>> >> Give the state a lock of its own instead of relying on that. Entries
>> >> move when a device is realized, when a resource is handed over, and when
>> >> a migration starts, so none of this is on a hot path and an uncontended
>> >> mutex costs nothing.
>> >> 
>> >> Take the lock in every accessor and across the save and load walks.
>> >> Holding it over the walk callbacks is safe because none of them comes
>> >> back through this interface. State that is not reached through these
>> >> accessors, such as the VFIO device list that hw/vfio manipulates
>> >> directly, stays outside it.
>> >> 
>> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> >> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> >> Cc: Peter Xu <peterx@redhat.com>
>> >> Cc: Fabiano Rosas <farosas@suse.de>
>> >> Cc: Mark Kanda <mark.kanda@oracle.com>
>> >> Cc: Ben Chaney <bchaney@akamai.com>
>> >> Cc: Steve Sistare <steven.sistare@oracle.com>
>> >
>> > Is there a real problem anywhere?
>> >
>> > My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
>> > creating a new device or so, which should normally need bql already.
>> >
>> > To ask in another way, shall we simply assert bql_locked()?
>> >
>> > Thanks,
>> 
>> Fair questions, let's see what Denis says.
>> 
>> From my point of view, if the work is already done to use a lock, I'd
>> take it; one less place for us to bother when dealing with BQL issues.
>> 
>> (on the other hand, I'm not doing any active use or testing of cpr aside
>> from migration-test, so my opinion is the least important here)
> 
> Yep, I get it.  It's actually sometimes easier to just merge it.
> 
> But one thing I want to try to avoid is we introduce a lock, but without
> knowing if it's needed.  The problem is for the future: we can't get rid of
> the lock easily, exactly because then we also don't know what will start to
> break.. so it might be good to ask early than never.

Isn't moving to more fine grained locking than BQL desirable in the long term,
much like BKL was split and removed from Linux kernel years ago?

For now, this new "cpr_fds_lock" would need to have its locking order with
respect to BQL explicitly documented (if not enforced via asserts).

> -- 
> Peter Xu
> 

Thanks,
Maciej

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Thu, Sep 17, 2026 at 02:24:42PM +0200, Maciej S. Szmigiero wrote:
> Isn't moving to more fine grained locking than BQL desirable in the long term,
> much like BKL was split and removed from Linux kernel years ago?

My question was more seeking the issue behind.  CPR is pretty special, I
didn't expect any new fd addition didn't already take BQL, at least before
reading the last message.

I don't think we should randomly add lock for any structure that is
currently protected by BQL, it'll unnecessarily slow things down.  We
should only do smaller granule locking when we can consider getting rid of
BQL in the first place.

> 
> For now, this new "cpr_fds_lock" would need to have its locking order with
> respect to BQL explicitly documented (if not enforced via asserts).

Yes, some document would be nicer.

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Thu, Sep 17, 2026 at 08:36:59AM -0400, Peter Xu wrote:
> On Thu, Sep 17, 2026 at 02:24:42PM +0200, Maciej S. Szmigiero wrote:
> > Isn't moving to more fine grained locking than BQL desirable in the long term,
> > much like BKL was split and removed from Linux kernel years ago?
> 
> My question was more seeking the issue behind.  CPR is pretty special, I
> didn't expect any new fd addition didn't already take BQL, at least before
> reading the last message.
> 
> I don't think we should randomly add lock for any structure that is
> currently protected by BQL, it'll unnecessarily slow things down.  We
> should only do smaller granule locking when we can consider getting rid of
> BQL in the first place.

Now having some understanding of the issue, I noticed I'm not very certain
whether this small lock can avoid all the issues.

Consider a race between cpr-exec migration at the very end, cpr_exec_cb()
does cpr_exec_preserve_fds() walking all the fds, then another connection
established in monitor iothread trying to add a new fd.

In this case, when without this smaller lock, it may crash accessing the
hash.

When with the lock, two operations are serialized, but is it enough?  It
means cpr_exec_preserve_fds() may take the lock first, mark all fds
reserved but without the new connection.

Would that be a problem?  Theoretically, cpr should persist all fds in this
case, including the newly attached?

Maybe it will never happen in practise because libvirt should never add
another connection there, but I feel like it's more than "protecting
concurrent access to a hash" problem.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Maciej S. Szmigiero]

On 17.09.2026 14:44, Peter Xu wrote:
> On Thu, Sep 17, 2026 at 08:36:59AM -0400, Peter Xu wrote:
>> On Thu, Sep 17, 2026 at 02:24:42PM +0200, Maciej S. Szmigiero wrote:
>>> Isn't moving to more fine grained locking than BQL desirable in the long term,
>>> much like BKL was split and removed from Linux kernel years ago?
>>
>> My question was more seeking the issue behind.  CPR is pretty special, I
>> didn't expect any new fd addition didn't already take BQL, at least before
>> reading the last message.
>>
>> I don't think we should randomly add lock for any structure that is
>> currently protected by BQL, it'll unnecessarily slow things down.  We
>> should only do smaller granule locking when we can consider getting rid of
>> BQL in the first place.
> 
> Now having some understanding of the issue, I noticed I'm not very certain
> whether this small lock can avoid all the issues.
> 
> Consider a race between cpr-exec migration at the very end, cpr_exec_cb()
> does cpr_exec_preserve_fds() walking all the fds, then another connection
> established in monitor iothread trying to add a new fd.
> 
> In this case, when without this smaller lock, it may crash accessing the
> hash.
> 
> When with the lock, two operations are serialized, but is it enough?  It
> means cpr_exec_preserve_fds() may take the lock first, mark all fds
> reserved but without the new connection.
> 
> Would that be a problem?  Theoretically, cpr should persist all fds in this
> case, including the newly attached?
> 
> Maybe it will never happen in practise because libvirt should never add
> another connection there, but I feel like it's more than "protecting
> concurrent access to a hash" problem.

I though monitor connection FDs aren't CPR-preserved in the first place, no?

And if you meant a monitor adding some non-monitor FD, I think this will
be handled in the main loop anyway (and so, serialized) via qmp_dispatcher?

> Thanks,
> 

Thanks,
Maciej

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Thu, Sep 17, 2026 at 04:17:54PM +0200, Maciej S. Szmigiero wrote:
> On 17.09.2026 14:44, Peter Xu wrote:
> > On Thu, Sep 17, 2026 at 08:36:59AM -0400, Peter Xu wrote:
> > > On Thu, Sep 17, 2026 at 02:24:42PM +0200, Maciej S. Szmigiero wrote:
> > > > Isn't moving to more fine grained locking than BQL desirable in the long term,
> > > > much like BKL was split and removed from Linux kernel years ago?
> > > 
> > > My question was more seeking the issue behind.  CPR is pretty special, I
> > > didn't expect any new fd addition didn't already take BQL, at least before
> > > reading the last message.
> > > 
> > > I don't think we should randomly add lock for any structure that is
> > > currently protected by BQL, it'll unnecessarily slow things down.  We
> > > should only do smaller granule locking when we can consider getting rid of
> > > BQL in the first place.
> > 
> > Now having some understanding of the issue, I noticed I'm not very certain
> > whether this small lock can avoid all the issues.
> > 
> > Consider a race between cpr-exec migration at the very end, cpr_exec_cb()
> > does cpr_exec_preserve_fds() walking all the fds, then another connection
> > established in monitor iothread trying to add a new fd.
> > 
> > In this case, when without this smaller lock, it may crash accessing the
> > hash.
> > 
> > When with the lock, two operations are serialized, but is it enough?  It
> > means cpr_exec_preserve_fds() may take the lock first, mark all fds
> > reserved but without the new connection.
> > 
> > Would that be a problem?  Theoretically, cpr should persist all fds in this
> > case, including the newly attached?
> > 
> > Maybe it will never happen in practise because libvirt should never add
> > another connection there, but I feel like it's more than "protecting
> > concurrent access to a hash" problem.
> 
> I though monitor connection FDs aren't CPR-preserved in the first place, no?
> 
> And if you meant a monitor adding some non-monitor FD, I think this will
> be handled in the main loop anyway (and so, serialized) via qmp_dispatcher?

My commnet above may not apply to master, but Denis's branch which
originally exposed this problem.  And I confess so far it's kind of my
interpretation of the issue from reading Denis's email, it may or may not
be accurate.

My understanding is the connection happens with the iothread not main
thread from a client, it may not really need bql.

Denis may have more info when he collects enough info, and correct what I
might be wrong.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Denis V. Lunev]

On 9/17/26 16:37, Peter Xu wrote:
> On Thu, Sep 17, 2026 at 04:17:54PM +0200, Maciej S. Szmigiero wrote:
>> On 17.09.2026 14:44, Peter Xu wrote:
>>> On Thu, Sep 17, 2026 at 08:36:59AM -0400, Peter Xu wrote:
>>>> On Thu, Sep 17, 2026 at 02:24:42PM +0200, Maciej S. Szmigiero wrote:
>>>>> Isn't moving to more fine grained locking than BQL desirable in the long term,
>>>>> much like BKL was split and removed from Linux kernel years ago?
>>>> My question was more seeking the issue behind.  CPR is pretty special, I
>>>> didn't expect any new fd addition didn't already take BQL, at least before
>>>> reading the last message.
>>>>
>>>> I don't think we should randomly add lock for any structure that is
>>>> currently protected by BQL, it'll unnecessarily slow things down.  We
>>>> should only do smaller granule locking when we can consider getting rid of
>>>> BQL in the first place.
>>> Now having some understanding of the issue, I noticed I'm not very certain
>>> whether this small lock can avoid all the issues.
>>>
>>> Consider a race between cpr-exec migration at the very end, cpr_exec_cb()
>>> does cpr_exec_preserve_fds() walking all the fds, then another connection
>>> established in monitor iothread trying to add a new fd.
>>>
>>> In this case, when without this smaller lock, it may crash accessing the
>>> hash.
>>>
>>> When with the lock, two operations are serialized, but is it enough?  It
>>> means cpr_exec_preserve_fds() may take the lock first, mark all fds
>>> reserved but without the new connection.
>>>
>>> Would that be a problem?  Theoretically, cpr should persist all fds in this
>>> case, including the newly attached?
>>>
>>> Maybe it will never happen in practise because libvirt should never add
>>> another connection there, but I feel like it's more than "protecting
>>> concurrent access to a hash" problem.
>> I though monitor connection FDs aren't CPR-preserved in the first place, no?
>>
>> And if you meant a monitor adding some non-monitor FD, I think this will
>> be handled in the main loop anyway (and so, serialized) via qmp_dispatcher?
> My commnet above may not apply to master, but Denis's branch which
> originally exposed this problem.  And I confess so far it's kind of my
> interpretation of the issue from reading Denis's email, it may or may not
> be accurate.
>
> My understanding is the connection happens with the iothread not main
> thread from a client, it may not really need bql.
>
> Denis may have more info when he collects enough info, and correct what I
> might be wrong.
>
> Thanks,
>
BQL is not held in IO thread and taking in there is IMHO
no go at all - these threads were specifically invented
to avoid it inside.

This is another side of the story - I have observed
BQL in disk IO threads and chasing them too. A have
at least 2 known for me places - IDE (which could be
executed even when vCPUs are looking idle) and
controller resets f.e. in virtio-scsi which call bdrv_drain()
with BQL held.

We have agreed as far as I understand to fix comment for
now until monitor CPR will come. At that moment we will
have to decide about locking.

Den

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Denis V. Lunev]

On 9/16/26 20:57, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
>> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
>>> From: Denis V. Lunev <den@openvz.org>
>>>
>>> The cpr state is a process wide registry of the resources that have to
>>> survive an exec. Entries are added and dropped from realize and teardown
>>> paths all over the tree, and a caller has no way to tell which thread the
>>> next caller will run in.
>>>
>>> The state carries no serialization of its own. It is safe today only
>>> because its callers happen to hold the BQL, and that is a property of
>>> where those callers live rather than anything the interface promises or
>>> checks. A caller that is correct within its own subsystem can therefore
>>> break this one from a distance: two that overlap corrupt the registry,
>>> and the next lookup walks an entry that has already been freed.
>>>
>>> Give the state a lock of its own instead of relying on that. Entries
>>> move when a device is realized, when a resource is handed over, and when
>>> a migration starts, so none of this is on a hot path and an uncontended
>>> mutex costs nothing.
>>>
>>> Take the lock in every accessor and across the save and load walks.
>>> Holding it over the walk callbacks is safe because none of them comes
>>> back through this interface. State that is not reached through these
>>> accessors, such as the VFIO device list that hw/vfio manipulates
>>> directly, stays outside it.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Cc: Fabiano Rosas <farosas@suse.de>
>>> Cc: Mark Kanda <mark.kanda@oracle.com>
>>> Cc: Ben Chaney <bchaney@akamai.com>
>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>> Is there a real problem anywhere?
>>
>> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
>> creating a new device or so, which should normally need bql already.
>>
>> To ask in another way, shall we simply assert bql_locked()?
>>
>> Thanks,
> Fair questions, let's see what Denis says.
>
> From my point of view, if the work is already done to use a lock, I'd
> take it; one less place for us to bother when dealing with BQL issues.
>
> (on the other hand, I'm not doing any active use or testing of cpr aside
> from migration-test, so my opinion is the least important here)
The fair answer if the following.

1. Right now FD database is already accessed from main thread and
   from vCPU threads (MSIx handling). It is fortunately protected
   with BQL.

2. There is no real today problem right now. You are right. The
   problem is in downstream.

But the problem is real. We either should fix either contract
in written - just BQL or add the lock.

I do think that CPR have value for the project - devices will
come sooner or late and the database is to be protected.

This is fair statement for today.

Thank you for your time,
    Den

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Wed, Sep 16, 2026 at 09:45:56PM +0200, Denis V. Lunev wrote:
> On 9/16/26 20:57, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
> >>> From: Denis V. Lunev <den@openvz.org>
> >>>
> >>> The cpr state is a process wide registry of the resources that have to
> >>> survive an exec. Entries are added and dropped from realize and teardown
> >>> paths all over the tree, and a caller has no way to tell which thread the
> >>> next caller will run in.
> >>>
> >>> The state carries no serialization of its own. It is safe today only
> >>> because its callers happen to hold the BQL, and that is a property of
> >>> where those callers live rather than anything the interface promises or
> >>> checks. A caller that is correct within its own subsystem can therefore
> >>> break this one from a distance: two that overlap corrupt the registry,
> >>> and the next lookup walks an entry that has already been freed.
> >>>
> >>> Give the state a lock of its own instead of relying on that. Entries
> >>> move when a device is realized, when a resource is handed over, and when
> >>> a migration starts, so none of this is on a hot path and an uncontended
> >>> mutex costs nothing.
> >>>
> >>> Take the lock in every accessor and across the save and load walks.
> >>> Holding it over the walk callbacks is safe because none of them comes
> >>> back through this interface. State that is not reached through these
> >>> accessors, such as the VFIO device list that hw/vfio manipulates
> >>> directly, stays outside it.
> >>>
> >>> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> >>> Cc: Peter Xu <peterx@redhat.com>
> >>> Cc: Fabiano Rosas <farosas@suse.de>
> >>> Cc: Mark Kanda <mark.kanda@oracle.com>
> >>> Cc: Ben Chaney <bchaney@akamai.com>
> >>> Cc: Steve Sistare <steven.sistare@oracle.com>
> >> Is there a real problem anywhere?
> >>
> >> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
> >> creating a new device or so, which should normally need bql already.
> >>
> >> To ask in another way, shall we simply assert bql_locked()?
> >>
> >> Thanks,
> > Fair questions, let's see what Denis says.
> >
> > From my point of view, if the work is already done to use a lock, I'd
> > take it; one less place for us to bother when dealing with BQL issues.
> >
> > (on the other hand, I'm not doing any active use or testing of cpr aside
> > from migration-test, so my opinion is the least important here)
> The fair answer if the following.
> 
> 1. Right now FD database is already accessed from main thread and
>    from vCPU threads (MSIx handling). It is fortunately protected
>    with BQL.
> 
> 2. There is no real today problem right now. You are right. The
>    problem is in downstream.
> 
> But the problem is real. We either should fix either contract
> in written - just BQL or add the lock.
> 
> I do think that CPR have value for the project - devices will
> come sooner or late and the database is to be protected.
> 
> This is fair statement for today.

Thanks for the explanation.

Logically we shouldn't merge anything that only works for downstream but
not upstream, but I agree this one is sensible on its own.

Perhaps we can add some more info into the commig log?  The vCPU MSIx
handling, even if not applicable to a real issue, seems to be a good
example to illustrate the potential problem. In this case, personally I'd
even prefer stating the fact that "only our downstream is affected, but we
want to upstream this patch too because .."  these kind of things, rather
than being obscure on reasoning.

Do you plan to upstream the affected component?  If so, I would still
suggest to have this patch be together with that series, because that's
where it really will make a difference (rather than purely wasting pCPU
cycles on some lock bit..).

Otherwise I don't have a strong opinion, this one is a light change, maint
cost low (it'll be different if cpr_walk_fd() can take any cb().. but it's
not the case), we can have it.  I'll leave this to Fabiano to say. Sorry if
I'm too verbose here.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Denis V. Lunev]

On 9/16/26 22:43, Peter Xu wrote:
> On Wed, Sep 16, 2026 at 09:45:56PM +0200, Denis V. Lunev wrote:
>> On 9/16/26 20:57, Fabiano Rosas wrote:
>>> Peter Xu <peterx@redhat.com> writes:
>>>
>>>> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
>>>>> From: Denis V. Lunev <den@openvz.org>
>>>>>
>>>>> The cpr state is a process wide registry of the resources that have to
>>>>> survive an exec. Entries are added and dropped from realize and teardown
>>>>> paths all over the tree, and a caller has no way to tell which thread the
>>>>> next caller will run in.
>>>>>
>>>>> The state carries no serialization of its own. It is safe today only
>>>>> because its callers happen to hold the BQL, and that is a property of
>>>>> where those callers live rather than anything the interface promises or
>>>>> checks. A caller that is correct within its own subsystem can therefore
>>>>> break this one from a distance: two that overlap corrupt the registry,
>>>>> and the next lookup walks an entry that has already been freed.
>>>>>
>>>>> Give the state a lock of its own instead of relying on that. Entries
>>>>> move when a device is realized, when a resource is handed over, and when
>>>>> a migration starts, so none of this is on a hot path and an uncontended
>>>>> mutex costs nothing.
>>>>>
>>>>> Take the lock in every accessor and across the save and load walks.
>>>>> Holding it over the walk callbacks is safe because none of them comes
>>>>> back through this interface. State that is not reached through these
>>>>> accessors, such as the VFIO device list that hw/vfio manipulates
>>>>> directly, stays outside it.
>>>>>
>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>> Cc: Peter Xu <peterx@redhat.com>
>>>>> Cc: Fabiano Rosas <farosas@suse.de>
>>>>> Cc: Mark Kanda <mark.kanda@oracle.com>
>>>>> Cc: Ben Chaney <bchaney@akamai.com>
>>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>>> Is there a real problem anywhere?
>>>>
>>>> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
>>>> creating a new device or so, which should normally need bql already.
>>>>
>>>> To ask in another way, shall we simply assert bql_locked()?
>>>>
>>>> Thanks,
>>> Fair questions, let's see what Denis says.
>>>
>>> From my point of view, if the work is already done to use a lock, I'd
>>> take it; one less place for us to bother when dealing with BQL issues.
>>>
>>> (on the other hand, I'm not doing any active use or testing of cpr aside
>>> from migration-test, so my opinion is the least important here)
>> The fair answer if the following.
>>
>> 1. Right now FD database is already accessed from main thread and
>>    from vCPU threads (MSIx handling). It is fortunately protected
>>    with BQL.
>>
>> 2. There is no real today problem right now. You are right. The
>>    problem is in downstream.
>>
>> But the problem is real. We either should fix either contract
>> in written - just BQL or add the lock.
>>
>> I do think that CPR have value for the project - devices will
>> come sooner or late and the database is to be protected.
>>
>> This is fair statement for today.
> Thanks for the explanation.
>
> Logically we shouldn't merge anything that only works for downstream but
> not upstream, but I agree this one is sensible on its own.
>
> Perhaps we can add some more info into the commig log?  The vCPU MSIx
> handling, even if not applicable to a real issue, seems to be a good
> example to illustrate the potential problem. In this case, personally I'd
> even prefer stating the fact that "only our downstream is affected, but we
> want to upstream this patch too because .."  these kind of things, rather
> than being obscure on reasoning.
>
> Do you plan to upstream the affected component?  If so, I would still
> suggest to have this patch be together with that series, because that's
> where it really will make a difference (rather than purely wasting pCPU
> cycles on some lock bit..).
>
> Otherwise I don't have a strong opinion, this one is a light change, maint
> cost low (it'll be different if cpr_walk_fd() can take any cb().. but it's
> not the case), we can have it.  I'll leave this to Fabiano to say. Sorry if
> I'm too verbose here.
>
> Thanks,
>
The problems is caused by the external communication sockets
which we are willing to keep, starting from monitor.
Monitor in reality should be kept in the other case
it would be very unclear at which moment libvirt should
start reconnecting.

This adds hooks to socket backend and dances begin.

I will take a look whether this has been sent/discussed and
come with this.

For now we could add comment: we are protected with BQL.
Will this work as a replacement? Both approaches are fine
for me.

Den

Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Peter Xu]

On Thu, Sep 17, 2026 at 11:48:52AM +0200, Denis V. Lunev wrote:
> On 9/16/26 22:43, Peter Xu wrote:
> > On Wed, Sep 16, 2026 at 09:45:56PM +0200, Denis V. Lunev wrote:
> >> On 9/16/26 20:57, Fabiano Rosas wrote:
> >>> Peter Xu <peterx@redhat.com> writes:
> >>>
> >>>> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
> >>>>> From: Denis V. Lunev <den@openvz.org>
> >>>>>
> >>>>> The cpr state is a process wide registry of the resources that have to
> >>>>> survive an exec. Entries are added and dropped from realize and teardown
> >>>>> paths all over the tree, and a caller has no way to tell which thread the
> >>>>> next caller will run in.
> >>>>>
> >>>>> The state carries no serialization of its own. It is safe today only
> >>>>> because its callers happen to hold the BQL, and that is a property of
> >>>>> where those callers live rather than anything the interface promises or
> >>>>> checks. A caller that is correct within its own subsystem can therefore
> >>>>> break this one from a distance: two that overlap corrupt the registry,
> >>>>> and the next lookup walks an entry that has already been freed.
> >>>>>
> >>>>> Give the state a lock of its own instead of relying on that. Entries
> >>>>> move when a device is realized, when a resource is handed over, and when
> >>>>> a migration starts, so none of this is on a hot path and an uncontended
> >>>>> mutex costs nothing.
> >>>>>
> >>>>> Take the lock in every accessor and across the save and load walks.
> >>>>> Holding it over the walk callbacks is safe because none of them comes
> >>>>> back through this interface. State that is not reached through these
> >>>>> accessors, such as the VFIO device list that hw/vfio manipulates
> >>>>> directly, stays outside it.
> >>>>>
> >>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>>>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> >>>>> Cc: Peter Xu <peterx@redhat.com>
> >>>>> Cc: Fabiano Rosas <farosas@suse.de>
> >>>>> Cc: Mark Kanda <mark.kanda@oracle.com>
> >>>>> Cc: Ben Chaney <bchaney@akamai.com>
> >>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
> >>>> Is there a real problem anywhere?
> >>>>
> >>>> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
> >>>> creating a new device or so, which should normally need bql already.
> >>>>
> >>>> To ask in another way, shall we simply assert bql_locked()?
> >>>>
> >>>> Thanks,
> >>> Fair questions, let's see what Denis says.
> >>>
> >>> From my point of view, if the work is already done to use a lock, I'd
> >>> take it; one less place for us to bother when dealing with BQL issues.
> >>>
> >>> (on the other hand, I'm not doing any active use or testing of cpr aside
> >>> from migration-test, so my opinion is the least important here)
> >> The fair answer if the following.
> >>
> >> 1. Right now FD database is already accessed from main thread and
> >>    from vCPU threads (MSIx handling). It is fortunately protected
> >>    with BQL.
> >>
> >> 2. There is no real today problem right now. You are right. The
> >>    problem is in downstream.
> >>
> >> But the problem is real. We either should fix either contract
> >> in written - just BQL or add the lock.
> >>
> >> I do think that CPR have value for the project - devices will
> >> come sooner or late and the database is to be protected.
> >>
> >> This is fair statement for today.
> > Thanks for the explanation.
> >
> > Logically we shouldn't merge anything that only works for downstream but
> > not upstream, but I agree this one is sensible on its own.
> >
> > Perhaps we can add some more info into the commig log?  The vCPU MSIx
> > handling, even if not applicable to a real issue, seems to be a good
> > example to illustrate the potential problem. In this case, personally I'd
> > even prefer stating the fact that "only our downstream is affected, but we
> > want to upstream this patch too because .."  these kind of things, rather
> > than being obscure on reasoning.
> >
> > Do you plan to upstream the affected component?  If so, I would still
> > suggest to have this patch be together with that series, because that's
> > where it really will make a difference (rather than purely wasting pCPU
> > cycles on some lock bit..).
> >
> > Otherwise I don't have a strong opinion, this one is a light change, maint
> > cost low (it'll be different if cpr_walk_fd() can take any cb().. but it's
> > not the case), we can have it.  I'll leave this to Fabiano to say. Sorry if
> > I'm too verbose here.
> >
> > Thanks,
> >
> The problems is caused by the external communication sockets
> which we are willing to keep, starting from monitor.
> Monitor in reality should be kept in the other case
> it would be very unclear at which moment libvirt should
> start reconnecting.
> 
> This adds hooks to socket backend and dances begin.
> 
> I will take a look whether this has been sent/discussed and
> come with this.

Ah I see, I recall Steve used to upstream something like that for chardev
backing monitors.  I thought it has landed.  I hope there're not two impl
of that problem, then it may imply we should upstream one of them.

> 
> For now we could add comment: we are protected with BQL.
> Will this work as a replacement? Both approaches are fine
> for me.

So now I'm trying to think from the problematic branch, not master.  In
that case it is libvirt connection kicking off the monitor iothread to
inject some client socket fd to CPR, right?  In that case, maybe BQL isn't
suitable anymore, because IIUC we don't want to take BQL in the monitor
iothread, at least for the same of Out-Of-Band messages to work.

Please help investigate the status quo for chardev cpr support and maybe
we'll know better, thanks.  If we want to merge this then it'll be great
the commit log contains all important bits (especially, if BQL is not the
right lock to use), because now I feel like taking BQL there is wrong then.

Thanks,

-- 
Peter Xu