On Mon, Oct 12, 2020 at 02:53:17PM +0200, David Hildenbrand wrote:
>Let's use wrappers for the low-level functions that dev_dbg/dev_warn
>and work on addr + size, such that we can reuse them for adding/removing
>in other granularity.
>
>We only warn when adding memory failed, because that's something to pay
>attention to. We won't warn when removing failed, we'll reuse that in
>racy context soon (and we do have proper BUG_ON() statements in the
>current cases where it must never happen).
>
>Cc: "Michael S. Tsirkin" <m...@redhat.com>
>Cc: Jason Wang <jasow...@redhat.com>
>Cc: Pankaj Gupta <pankaj.gupta.li...@gmail.com>
>Signed-off-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: Wei Yang <richard.weiy...@linux.alibaba.com>
>---
> drivers/virtio/virtio_mem.c | 107 ++++++++++++++++++++++++------------
> 1 file changed, 73 insertions(+), 34 deletions(-)
>
>diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>index d3ab04f655ee..eb2ad31a8d8a 100644
>--- a/drivers/virtio/virtio_mem.c
>+++ b/drivers/virtio/virtio_mem.c
>@@ -453,18 +453,16 @@ static bool virtio_mem_could_add_memory(struct
>virtio_mem *vm, uint64_t size)
> }
>
> /*
>- * Try to add a memory block to Linux. This will usually only fail
>- * if out of memory.
>+ * Try adding memory to Linux. Will usually only fail if out of memory.
> *
> * Must not be called with the vm->hotplug_mutex held (possible deadlock with
> * onlining code).
> *
>- * Will not modify the state of the memory block.
>+ * Will not modify the state of memory blocks in virtio-mem.
> */
>-static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id)
>+static int virtio_mem_add_memory(struct virtio_mem *vm, uint64_t addr,
>+ uint64_t size)
> {
>- const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>- const uint64_t size = memory_block_size_bytes();
> int rc;
>
> /*
>@@ -478,32 +476,50 @@ static int virtio_mem_mb_add(struct virtio_mem *vm,
>unsigned long mb_id)
> return -ENOMEM;
> }
>
>- dev_dbg(&vm->vdev->dev, "adding memory block: %lu\n", mb_id);
>+ dev_dbg(&vm->vdev->dev, "adding memory: 0x%llx - 0x%llx\n", addr,
>+ addr + size - 1);
> /* Memory might get onlined immediately. */
> atomic64_add(size, &vm->offline_size);
> rc = add_memory_driver_managed(vm->nid, addr, size, vm->resource_name,
> MEMHP_MERGE_RESOURCE);
>- if (rc)
>+ if (rc) {
> atomic64_sub(size, &vm->offline_size);
>+ dev_warn(&vm->vdev->dev, "adding memory failed: %d\n", rc);
>+ /*
>+ * TODO: Linux MM does not properly clean up yet in all cases
>+ * where adding of memory failed - especially on -ENOMEM.
>+ */
>+ }
> return rc;
> }
>
> /*
>- * Try to remove a memory block from Linux. Will only fail if the memory block
>- * is not offline.
>+ * See virtio_mem_add_memory(): Try adding a single Linux memory block.
>+ */
>+static int virtio_mem_sbm_add_mb(struct virtio_mem *vm, unsigned long mb_id)
>+{
>+ const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>+ const uint64_t size = memory_block_size_bytes();
>+
>+ return virtio_mem_add_memory(vm, addr, size);
>+}
>+
>+/*
>+ * Try removing memory from Linux. Will only fail if memory blocks aren't
>+ * offline.
> *
> * Must not be called with the vm->hotplug_mutex held (possible deadlock with
> * onlining code).
> *
>- * Will not modify the state of the memory block.
>+ * Will not modify the state of memory blocks in virtio-mem.
> */
>-static int virtio_mem_mb_remove(struct virtio_mem *vm, unsigned long mb_id)
>+static int virtio_mem_remove_memory(struct virtio_mem *vm, uint64_t addr,
>+ uint64_t size)
> {
>- const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>- const uint64_t size = memory_block_size_bytes();
> int rc;
>
>- dev_dbg(&vm->vdev->dev, "removing memory block: %lu\n", mb_id);
>+ dev_dbg(&vm->vdev->dev, "removing memory: 0x%llx - 0x%llx\n", addr,
>+ addr + size - 1);
> rc = remove_memory(vm->nid, addr, size);
> if (!rc) {
> atomic64_sub(size, &vm->offline_size);
>@@ -512,27 +528,41 @@ static int virtio_mem_mb_remove(struct virtio_mem *vm,
>unsigned long mb_id)
> * immediately instead of waiting.
> */
> virtio_mem_retry(vm);
>+ } else {
>+ dev_dbg(&vm->vdev->dev, "removing memory failed: %d\n", rc);
> }
> return rc;
> }
>
> /*
>- * Try to offline and remove a memory block from Linux.
>+ * See virtio_mem_remove_memory(): Try removing a single Linux memory block.
>+ */
>+static int virtio_mem_sbm_remove_mb(struct virtio_mem *vm, unsigned long
>mb_id)
>+{
>+ const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>+ const uint64_t size = memory_block_size_bytes();
>+
>+ return virtio_mem_remove_memory(vm, addr, size);
>+}
>+
>+/*
>+ * Try offlining and removing memory from Linux.
> *
> * Must not be called with the vm->hotplug_mutex held (possible deadlock with
> * onlining code).
> *
>- * Will not modify the state of the memory block.
>+ * Will not modify the state of memory blocks in virtio-mem.
> */
>-static int virtio_mem_mb_offline_and_remove(struct virtio_mem *vm,
>- unsigned long mb_id)
>+static int virtio_mem_offline_and_remove_memory(struct virtio_mem *vm,
>+ uint64_t addr,
>+ uint64_t size)
> {
>- const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>- const uint64_t size = memory_block_size_bytes();
> int rc;
>
>- dev_dbg(&vm->vdev->dev, "offlining and removing memory block: %lu\n",
>- mb_id);
>+ dev_dbg(&vm->vdev->dev,
>+ "offlining and removing memory: 0x%llx - 0x%llx\n", addr,
>+ addr + size - 1);
>+
> rc = offline_and_remove_memory(vm->nid, addr, size);
> if (!rc) {
> atomic64_sub(size, &vm->offline_size);
>@@ -541,10 +571,26 @@ static int virtio_mem_mb_offline_and_remove(struct
>virtio_mem *vm,
> * immediately instead of waiting.
> */
> virtio_mem_retry(vm);
>+ } else {
>+ dev_dbg(&vm->vdev->dev,
>+ "offlining and removing memory failed: %d\n", rc);
> }
> return rc;
> }
>
>+/*
>+ * See virtio_mem_offline_and_remove_memory(): Try offlining and removing
>+ * a single Linux memory block.
>+ */
>+static int virtio_mem_sbm_offline_and_remove_mb(struct virtio_mem *vm,
>+ unsigned long mb_id)
>+{
>+ const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id);
>+ const uint64_t size = memory_block_size_bytes();
>+
>+ return virtio_mem_offline_and_remove_memory(vm, addr, size);
>+}
>+
> /*
> * Trigger the workqueue so the device can perform its magic.
> */
>@@ -1230,17 +1276,10 @@ static int virtio_mem_sbm_plug_and_add_mb(struct
>virtio_mem *vm,
> VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL);
>
> /* Add the memory block to linux - if that fails, try to unplug. */
>- rc = virtio_mem_mb_add(vm, mb_id);
>+ rc = virtio_mem_sbm_add_mb(vm, mb_id);
> if (rc) {
> int new_state = VIRTIO_MEM_SBM_MB_UNUSED;
>
>- dev_err(&vm->vdev->dev,
>- "adding memory block %lu failed with %d\n", mb_id, rc);
>-
>- /*
>- * TODO: Linux MM does not properly clean up yet in all cases
>- * where adding of memory failed - especially on -ENOMEM.
>- */
> if (virtio_mem_sbm_unplug_sb(vm, mb_id, 0, count))
> new_state = VIRTIO_MEM_SBM_MB_PLUGGED;
> virtio_mem_sbm_set_mb_state(vm, mb_id, new_state);
>@@ -1411,7 +1450,7 @@ static int virtio_mem_sbm_unplug_any_sb_offline(struct
>virtio_mem *vm,
> VIRTIO_MEM_SBM_MB_UNUSED);
>
> mutex_unlock(&vm->hotplug_mutex);
>- rc = virtio_mem_mb_remove(vm, mb_id);
>+ rc = virtio_mem_sbm_remove_mb(vm, mb_id);
> BUG_ON(rc);
> mutex_lock(&vm->hotplug_mutex);
> }
>@@ -1504,7 +1543,7 @@ static int virtio_mem_sbm_unplug_any_sb_online(struct
>virtio_mem *vm,
> */
> if (virtio_mem_sbm_test_sb_unplugged(vm, mb_id, 0, vm->sbm.sbs_per_mb))
> {
> mutex_unlock(&vm->hotplug_mutex);
>- rc = virtio_mem_mb_offline_and_remove(vm, mb_id);
>+ 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,
>@@ -1991,7 +2030,7 @@ static void virtio_mem_remove(struct virtio_device *vdev)
> */
> virtio_mem_sbm_for_each_mb(vm, mb_id,
> VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL) {
>- rc = virtio_mem_mb_remove(vm, mb_id);
>+ rc = virtio_mem_sbm_remove_mb(vm, mb_id);
> BUG_ON(rc);
> virtio_mem_sbm_set_mb_state(vm, mb_id,
> VIRTIO_MEM_SBM_MB_UNUSED);
>--
>2.26.2
--
Wei Yang
Help you, Help me
