Re: [PATCH RFC 5/6] virtio-mem: support memmap_on_memory in Sub Block Mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] virtio-mem: swallowed memmap unplug errors in virtio_mem_sbm_try_remove_unplugged_mb()
--

commit b0988c80262195af51a910d0ad85c26ee566fe77
Author: Nikolaos Karaolidis <nick@karaolidis.com>

virtio-mem: support memmap_on_memory in Sub Block Mode

This commit adds memmap_on_memory support for Sub Block Mode in virtio-mem
when the self-hosted memmap spans complete subblocks. It keeps the memmap
subblocks plugged while a memory block is added to Linux, skips them when
unplugging from an added block, and unplugs them once the block is removed.

> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 688c178c760f5..d6623f67b76bb 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c

[ ... ]

> @@ -1617,35 +1628,65 @@ static int virtio_mem_sbm_unplug_mb(struct virtio_mem *vm, unsigned long mb_id)
>  {
>  	uint64_t nb_sb = vm->sbm.sbs_per_mb;
>  
> -	return virtio_mem_sbm_unplug_any_sb_raw(vm, mb_id, &nb_sb);
> +	return virtio_mem_sbm_unplug_any_sb_raw(vm, mb_id, 0, &nb_sb);
> +}
> +
> +/*
> + * Unplug the memmap subblocks of a memory block removed from Linux and mark it
> + * unused. The memmap subblocks are deducted from @nb_sb (clamped), even if
> + * unplugging them fails. On error, the memory block stays
> + * VIRTIO_MEM_SBM_MB_PLUGGED and virtio_mem_cleanup_pending_mb() retries.
> + *
> + * Will modify the state of the memory block.
> + */
> +static int virtio_mem_sbm_finish_remove_mb(struct virtio_mem *vm,
> +					   unsigned long mb_id, uint64_t *nb_sb)
> +{
> +	const int count = vm->sbm.memmap_sbs;
> +	int rc;
> +
> +	if (count) {
> +		if (nb_sb)
> +			*nb_sb -= min_t(uint64_t, *nb_sb, count);
> +		virtio_mem_sbm_set_mb_state(vm, mb_id,
> +					    VIRTIO_MEM_SBM_MB_PLUGGED);
> +		rc = virtio_mem_sbm_unplug_sb(vm, mb_id, 0, count);
> +		if (rc)
> +			return rc;
> +	}
> +	virtio_mem_sbm_set_mb_state(vm, mb_id, VIRTIO_MEM_SBM_MB_UNUSED);
> +	return 0;
>  }
>  
>  /*
> - * Try (offlining and) removing memory from Linux in case all subblocks are
> - * unplugged. Can be called on online and offline memory blocks.
> + * Try (offlining and) removing memory from Linux in case all subblocks except
> + * the memmap subblocks are unplugged. Can be called on online and offline
> + * memory blocks.
>   *
>   * May modify the state of memory blocks in virtio-mem.
>   */
>  static int virtio_mem_sbm_try_remove_unplugged_mb(struct virtio_mem *vm,
> -						  unsigned long mb_id)
> +						  unsigned long mb_id,
> +						  uint64_t *nb_sb)
>  {
> +	const int first_sb = vm->sbm.memmap_sbs;
>  	int rc;
>  
>  	/*
>  	 * Once all subblocks of a memory block were unplugged, offline and
> -	 * remove it.
> +	 * remove it. The memmap subblocks alone provide no memory to Linux.
>  	 */
> -	if (!virtio_mem_sbm_test_sb_unplugged(vm, mb_id, 0, vm->sbm.sbs_per_mb))
> +	if (!virtio_mem_sbm_test_sb_unplugged(vm, mb_id, first_sb,
> +					      vm->sbm.sbs_per_mb - first_sb))
>  		return 0;
>  
>  	/* offline_and_remove_memory() works for online and offline memory. */
>  	mutex_unlock(&vm->hotplug_mutex);
>  	rc = virtio_mem_sbm_offline_and_remove_mb(vm, mb_id);
>  	mutex_lock(&vm->hotplug_mutex);
> -	if (!rc)
> -		virtio_mem_sbm_set_mb_state(vm, mb_id,
> -					    VIRTIO_MEM_SBM_MB_UNUSED);
> -	return rc;
> +	if (rc)
> +		return rc;
> +	return virtio_mem_sbm_finish_remove_mb(vm, mb_id, nb_sb);
>  }

[ ... ]

> @@ -2072,7 +2130,7 @@ static int virtio_mem_sbm_unplug_any_sb_online(struct virtio_mem *vm,
>  	}
>  
>  unplugged:
> -	rc = virtio_mem_sbm_try_remove_unplugged_mb(vm, mb_id);
> +	rc = virtio_mem_sbm_try_remove_unplugged_mb(vm, mb_id, nb_sb);
>  	if (rc)
>  		vm->sbm.have_unplugged_mb = 1;
>  	/* Ignore errors, this is not critical. We'll retry later. */
> @@ -2389,11 +2447,11 @@ static int virtio_mem_cleanup_pending_mb(struct virtio_mem *vm)
>  
>  	mutex_lock(&vm->hotplug_mutex);
>  	virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_MOVABLE_PARTIAL)
> -		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id);
> +		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL);
>  	virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_KERNEL_PARTIAL)
> -		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id);
> +		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL);
>  	virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL)
> -		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id);
> +		rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL);
>  	mutex_unlock(&vm->hotplug_mutex);
>  
>  	if (rc)

[Severity: Medium]
When virtio_mem_sbm_offline_and_remove_mb() succeeds in
virtio_mem_sbm_try_remove_unplugged_mb(), it calls
virtio_mem_sbm_finish_remove_mb() to transition the memory block to
VIRTIO_MEM_SBM_MB_PLUGGED and unplug the memmap subblocks via
virtio_mem_sbm_unplug_sb().

Before this change, virtio_mem_sbm_try_remove_unplugged_mb() only failed when
virtio_mem_sbm_offline_and_remove_mb() failed, leaving the block added to
Linux in a *_PARTIAL state with all subblocks already unplugged from the
device. Both callers (virtio_mem_sbm_unplug_any_sb_online() and
virtio_mem_cleanup_pending_mb()) therefore set vm->sbm.have_unplugged_mb and
return 0 to ignore the offlining failure.

With this change, if virtio_mem_sbm_finish_remove_mb() fails to unplug the
memmap subblocks (for example, returning -EAGAIN on a concurrent config
change during offlining, -ETXTBSY on VIRTIO_MEM_RESP_BUSY, or -EINVAL on
VIRTIO_MEM_RESP_ERROR), both callers treat that device unplug error the same
way as an offlining failure:

1. They set vm->sbm.have_unplugged_mb even though the memory block was
   already removed from Linux and moved to VIRTIO_MEM_SBM_MB_PLUGGED rather
   than a *_PARTIAL state.

2. In virtio_mem_cleanup_pending_mb(), the VIRTIO_MEM_SBM_MB_PLUGGED loop has
   already run earlier in the function:

   virtio_mem_run_wq()
     virtio_mem_cleanup_pending_mb()
       virtio_mem_sbm_for_each_mb(..., VIRTIO_MEM_SBM_MB_PLUGGED)
         ...
       virtio_mem_sbm_for_each_mb(..., VIRTIO_MEM_SBM_MB_*_PARTIAL)
         virtio_mem_sbm_try_remove_unplugged_mb()
           virtio_mem_sbm_finish_remove_mb() -> fails, sets MB_PLUGGED
       returns 0
     virtio_mem_unplug_request() / virtio_mem_plug_request()

   Because virtio_mem_cleanup_pending_mb() swallows the error and returns 0,
   virtio_mem_run_wq() proceeds directly to virtio_mem_unplug_request() or
   virtio_mem_plug_request() in the same workqueue run while the failed
   block's memmap subblocks are still counted in vm->plugged_size. Doesn't
   this cause virtio_mem_unplug_request() to unplug extra subblocks from
   another active memory block (or virtio_mem_plug_request() to under-plug)?

3. In both callers, device errors from virtio_mem_sbm_finish_remove_mb() are
   masked instead of aborting the operation and propagating to
   virtio_mem_run_wq() (unlike virtio_mem_sbm_unplug_any_sb_offline(), which
   returns the error directly). For instance, in
   virtio_mem_sbm_unplug_any_sb_online(), returning 0 allows
   virtio_mem_sbm_unplug_request() to keep unplugging subsequent blocks on
   the device error, and in virtio_mem_run_wq(), -EAGAIN will not jump to
   retry and a fatal -EINVAL from VIRTIO_MEM_RESP_ERROR will be turned into
   -EBUSY via have_unplugged_mb instead of setting vm->broken = true.

Should virtio_mem_sbm_try_remove_unplugged_mb() distinguish an offlining
failure in virtio_mem_sbm_offline_and_remove_mb() (setting
vm->sbm.have_unplugged_mb and returning 0) from a device unplug failure in
virtio_mem_sbm_finish_remove_mb() (propagating the error code to the caller)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-virtio-mem-memmap-v1-0-308ae1da6240@karaolidis.com?part=5