diff mbox series

[v1,1/2] hostmem: use a static size for maxnode, validate policy everywhere

Message ID 20211207070607.1422670-1-d-tatianin@yandex-team.ru
State New
Headers show
Series [v1,1/2] hostmem: use a static size for maxnode, validate policy everywhere | expand

Commit Message

Daniil Tatianin Dec. 7, 2021, 7:06 a.m. UTC 
Previously we would calculate the last set bit in the mask, and add
2 to that value to get the maxnode value. This is unnecessary since
the mbind syscall allows the bitmap to be any (reasonable) size as
long as all the unused bits are clear. This also adds policy validation
in multiple places so that it's guaranteed to be valid when we call
mbind.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 backends/hostmem.c | 64 +++++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 21 deletions(-)

Comments

David Hildenbrand Dec. 7, 2021, 8:31 a.m. UTC | #1 
On 07.12.21 08:06, Daniil Tatianin wrote:
> Previously we would calculate the last set bit in the mask, and add
> 2 to that value to get the maxnode value. This is unnecessary since
> the mbind syscall allows the bitmap to be any (reasonable) size as
> long as all the unused bits are clear. This also adds policy validation
> in multiple places so that it's guaranteed to be valid when we call
> mbind.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  backends/hostmem.c | 64 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 4c05862ed5..392026efe6 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -38,6 +38,29 @@ host_memory_backend_get_name(HostMemoryBackend *backend)
>      return object_get_canonical_path(OBJECT(backend));
>  }
>  
> +static bool
> +validate_policy(HostMemPolicy policy, bool nodes_empty, Error **errp)
> +{
> +    /*
> +     * check for invalid host-nodes and policies and give more verbose
> +     * error messages than mbind().
> +     */
> +    if (!nodes_empty && policy == MPOL_DEFAULT) {
> +        error_setg(errp, "host-nodes must be empty for policy default,"
> +                   " or you should explicitly specify a policy other"
> +                   " than default");
> +        return false;
> +    }
> +
> +    if (nodes_empty && policy != MPOL_DEFAULT) {
> +        error_setg(errp, "host-nodes must be set for policy %s",
> +                   HostMemPolicy_str(policy));
> +        return false;
> +    }
> +
> +    return true;
> +}

Hm, we set two properties individually but bail out when the current combination 
is impossible, which is nasty. It means we have modify properties in the right order
(which will differ based on the policy) to make a change.

Do we have any sane use case of modifying the policy/host-nodes at runtime?
I mean, it's just completely wrong when we already have any memory
preallocated/touched inside the range, as we won't issue another mbind call.

I suggest instead to fix this hole:

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4c05862ed5..7edc3a075e 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -111,6 +111,11 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name,
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
     uint16List *l, *host_nodes = NULL;
 
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "Property 'host-nodes' cannot be changed anymore.");
+        return;
+    }
+
     visit_type_uint16List(v, name, &host_nodes, errp);
 
     for (l = host_nodes; l; l = l->next) {
@@ -142,6 +147,12 @@ static void
 host_memory_backend_set_policy(Object *obj, int policy, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "Property 'policy' cannot be changed anymore.");
+        return;
+    }
+
     backend->policy = policy;
 
 #ifndef CONFIG_NUMA
diff mbox series

Patch

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4c05862ed5..392026efe6 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -38,6 +38,29 @@  host_memory_backend_get_name(HostMemoryBackend *backend)
     return object_get_canonical_path(OBJECT(backend));
 }
 
+static bool
+validate_policy(HostMemPolicy policy, bool nodes_empty, Error **errp)
+{
+    /*
+     * check for invalid host-nodes and policies and give more verbose
+     * error messages than mbind().
+     */
+    if (!nodes_empty && policy == MPOL_DEFAULT) {
+        error_setg(errp, "host-nodes must be empty for policy default,"
+                   " or you should explicitly specify a policy other"
+                   " than default");
+        return false;
+    }
+
+    if (nodes_empty && policy != MPOL_DEFAULT) {
+        error_setg(errp, "host-nodes must be set for policy %s",
+                   HostMemPolicy_str(policy));
+        return false;
+    }
+
+    return true;
+}
+
 static void
 host_memory_backend_get_size(Object *obj, Visitor *v, const char *name,
                              void *opaque, Error **errp)
@@ -110,6 +133,7 @@  host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name,
 #ifdef CONFIG_NUMA
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
     uint16List *l, *host_nodes = NULL;
+    bool nodes_empty = bitmap_empty(backend->host_nodes, MAX_NODES + 1);
 
     visit_type_uint16List(v, name, &host_nodes, errp);
 
@@ -118,6 +142,13 @@  host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name,
             error_setg(errp, "Invalid host-nodes value: %d", l->value);
             goto out;
         }
+
+        nodes_empty = false;
+    }
+
+    if (host_memory_backend_mr_inited(backend) &&
+        !validate_policy(backend->policy, nodes_empty, errp)) {
+        goto out;
     }
 
     for (l = host_nodes; l; l = l->next) {
@@ -142,6 +173,13 @@  static void
 host_memory_backend_set_policy(Object *obj, int policy, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    bool nodes_empty = bitmap_empty(backend->host_nodes, MAX_NODES + 1);
+
+    if (host_memory_backend_mr_inited(backend) &&
+        !validate_policy(policy, nodes_empty, errp)) {
+        return;
+    }
+
     backend->policy = policy;
 
 #ifndef CONFIG_NUMA
@@ -347,24 +385,9 @@  host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
             qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
         }
 #ifdef CONFIG_NUMA
-        unsigned long lastbit = find_last_bit(backend->host_nodes, MAX_NODES);
-        /* lastbit == MAX_NODES means maxnode = 0 */
-        unsigned long maxnode = (lastbit + 1) % (MAX_NODES + 1);
-        /* ensure policy won't be ignored in case memory is preallocated
-         * before mbind(). note: MPOL_MF_STRICT is ignored on hugepages so
-         * this doesn't catch hugepage case. */
         unsigned flags = MPOL_MF_STRICT | MPOL_MF_MOVE;
-
-        /* check for invalid host-nodes and policies and give more verbose
-         * error messages than mbind(). */
-        if (maxnode && backend->policy == MPOL_DEFAULT) {
-            error_setg(errp, "host-nodes must be empty for policy default,"
-                       " or you should explicitly specify a policy other"
-                       " than default");
-            return;
-        } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
-            error_setg(errp, "host-nodes must be set for policy %s",
-                       HostMemPolicy_str(backend->policy));
+        bool nodes_empty = bitmap_empty(backend->host_nodes, MAX_NODES + 1);
+        if (!validate_policy(backend->policy, nodes_empty, errp)) {
             return;
         }
 
@@ -373,12 +396,11 @@  host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
          * cuts off the last specified node. This means backend->host_nodes
          * must have MAX_NODES+1 bits available.
          */
-        assert(sizeof(backend->host_nodes) >=
+        QEMU_BUILD_BUG_ON(sizeof(backend->host_nodes) <
                BITS_TO_LONGS(MAX_NODES + 1) * sizeof(unsigned long));
-        assert(maxnode <= MAX_NODES);
 
-        if (maxnode &&
-            mbind(ptr, sz, backend->policy, backend->host_nodes, maxnode + 1,
+        if (!nodes_empty &&
+            mbind(ptr, sz, backend->policy, backend->host_nodes, MAX_NODES + 1,
                   flags)) {
             if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
                 error_setg_errno(errp, errno,