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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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);
}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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