Re: [PATCH hci-8.0 2/2] migration/cpr: lock the ramblocks #VSTOR-145529

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

1. Minor: the ordering claim belongs in code, and in patch 1. "Nothing
takes the two in the other order" checks out: the walk callbacks in
cpr-exec only do madvise and mprotect, and the ramblock vmstate has no
callbacks. But the order was already forced by patch 1, so fold the
statement into the lock comment there.

  2. Nit: ram_block_add() does find, then save or delete, in three
separate acquisitions. It is correct only because the ramlist mutex is
held across all three at system/physmem.c:1913-2058. A one-line comment
saying the ramlist mutex is the real serializer there would save the
next reader the same analysis.

On 9/16/26 5:38 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> The ramblock entries live in the same registry as the descriptors and
> are reached the same way, from RAM block setup and teardown, so they
> need the same protection.
> 
> Take the lock in their accessors too. qemu_ram_free() already holds the
> ramlist mutex when it calls in here, and nothing takes the two in the
> other order.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  migration/cpr.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/cpr.c b/migration/cpr.c
> index 78cf430e386..5c0d2c142aa 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -244,8 +244,13 @@ static CprRamBlock *find_ramblock(CprRamBlockList *head, const char *name)
>  
>  void cpr_save_ramblock(const char *name, cpr_ramblock_t ram)
>  {
> -    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
> -    cpr_ramblock_t old_ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
> +    CprRamBlock *elem;
> +    cpr_ramblock_t old_ram;
> +
> +    QEMU_LOCK_GUARD(&cpr_state_lock);
> +
> +    elem = find_ramblock(&cpr_state.ramblocks, name);
> +    old_ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
>  
>      if (old_ram.addr == 0) {
>          do_cpr_save_ramblock(name, ram);
> @@ -260,7 +265,11 @@ void cpr_save_ramblock(const char *name, cpr_ramblock_t ram)
>  
>  void cpr_delete_ramblock(const char *name)
>  {
> -    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
> +    CprRamBlock *elem;
> +
> +    QEMU_LOCK_GUARD(&cpr_state_lock);
> +
> +    elem = find_ramblock(&cpr_state.ramblocks, name);
>  
>      if (elem) {
>          QLIST_REMOVE(elem, next);
> @@ -273,8 +282,13 @@ void cpr_delete_ramblock(const char *name)
>  
>  cpr_ramblock_t cpr_find_ramblock(const char *name)
>  {
> -    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
> -    cpr_ramblock_t ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
> +    CprRamBlock *elem;
> +    cpr_ramblock_t ram;
> +
> +    QEMU_LOCK_GUARD(&cpr_state_lock);
> +
> +    elem = find_ramblock(&cpr_state.ramblocks, name);
> +    ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
>  
>      trace_cpr_find_ramblock(name, (void *) ram.addr, ram.length);
>      return ram;
> @@ -284,6 +298,8 @@ bool cpr_walk_ramblock(cpr_walk_ramblock_cb cb)
>  {
>      CprRamBlock *elem;
>  
> +    QEMU_LOCK_GUARD(&cpr_state_lock);
> +
>      QLIST_FOREACH(elem, &cpr_state.ramblocks, next) {
>          g_assert(elem->ram.addr && elem->ram.length);
>          if (!cb(elem->ram, elem->name)) {