libxl_domain_setmaxmem() should update the domain's memory/static-max
Xenstore node, as otherwise "xl mem-set" won't be able to set the
memory size to the new maximum.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_mem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libs/light/libxl_mem.c b/tools/libs/light/libxl_mem.c
index c739d00f39..2f4f9d4a4a 100644
--- a/tools/libs/light/libxl_mem.c
+++ b/tools/libs/light/libxl_mem.c
@@ -82,6 +82,15 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
         goto out;
     }
 
+    rc = libxl__xs_printf(gc, XBT_NULL,
+                          GCSPRINTF("%s/memory/static-max", dompath),
+                          "%"PRIu64, max_memkb);
+    if (rc != 0) {
+        LOGED(ERROR, domid, "Couldn't set %s/memory/static-max, rc=%d\n",
+              dompath, rc);
+        goto out;
+    }
+
     rc = 0;
 out:
     libxl_domain_config_dispose(&d_config);
-- 
2.34.1

On Thu, Mar 31, 2022 at 09:07:55AM +0200, Juergen Gross wrote:
> libxl_domain_setmaxmem() should update the domain's memory/static-max
> Xenstore node, as otherwise "xl mem-set" won't be able to set the
> memory size to the new maximum.

`xl mem-set` doesn't call libxl_domain_setmaxmem(), but calls
libxl_set_memory_target().

Or maybe you are speaking about `xl mem-max` followed by `xl mem-set`?
In this case, it is documented in `man 1 xl` that `mem-max` has no
effect to `mem-set`.

quote from man, about `xl mem-max`:
    It is allowed to be higher than the configured maximum
    memory size of the domain (B<maxmem> parameter in the domain's
    configuration). Note however that the initial B<maxmem> value is still
    used as an upper limit for B<xl mem-set>.  Also note that calling B<xl
    mem-set> will reset this value.

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libs/light/libxl_mem.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/libs/light/libxl_mem.c b/tools/libs/light/libxl_mem.c
> index c739d00f39..2f4f9d4a4a 100644
> --- a/tools/libs/light/libxl_mem.c
> +++ b/tools/libs/light/libxl_mem.c
> @@ -82,6 +82,15 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)

There's a comment on this functions:
    /*
     * Set the maximum memory size of the domain in the hypervisor. There is no
     * change of the current memory size involved. The specified memory size can
     * even be above the configured maxmem size of the domain, but the related
     * Xenstore entry memory/static-max isn't modified!
     */
    int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)

So it was already known that "static-max" wasn't set.
At the very least, this comment needs updating.

>          goto out;
>      }
>  
> +    rc = libxl__xs_printf(gc, XBT_NULL,
> +                          GCSPRINTF("%s/memory/static-max", dompath),
> +                          "%"PRIu64, max_memkb);
> +    if (rc != 0) {
> +        LOGED(ERROR, domid, "Couldn't set %s/memory/static-max, rc=%d\n",
> +              dompath, rc);
> +        goto out;



So, I don't know whether increasing "static-max" is fine or not, but
according to the documentation, it isn't expected.

Is a guest fine with "static-max" been changed?

If yes, there's documentation and comments that needs to change with the
code change.

Thanks,

-- 
Anthony PERARD

On 07.04.22 16:44, Anthony PERARD wrote:
> On Thu, Mar 31, 2022 at 09:07:55AM +0200, Juergen Gross wrote:
>> libxl_domain_setmaxmem() should update the domain's memory/static-max
>> Xenstore node, as otherwise "xl mem-set" won't be able to set the
>> memory size to the new maximum.
> 
> `xl mem-set` doesn't call libxl_domain_setmaxmem(), but calls
> libxl_set_memory_target().

Correct. And it refuses to do so when memory/static-max is below the
new memory size to be set.

> 
> Or maybe you are speaking about `xl mem-max` followed by `xl mem-set`?
> In this case, it is documented in `man 1 xl` that `mem-max` has no
> effect to `mem-set`.

When having e.g. a domain with 1G of maxmem, then calling

xl mem-max <domain> 2048

it should be possible to then do

xl mem-set <domain> 2048

but this isn't possible, as xl mem-set will look at the memory/static-max
node of the domain and refuse to do the setting if it has a too low
value, which is the case today.

> 
> quote from man, about `xl mem-max`:
>      It is allowed to be higher than the configured maximum
>      memory size of the domain (B<maxmem> parameter in the domain's
>      configuration). Note however that the initial B<maxmem> value is still
>      used as an upper limit for B<xl mem-set>.  Also note that calling B<xl
>      mem-set> will reset this value.

Oh, this should then be adapted. Today hotplugging memory is just
a mess as you need to:

xl mem-max <domid> <value>
xenstore-write /local/domain/<domid>/memory/static-max $((<value> * 1024))
xl mem-set <domid> <value>

> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/libs/light/libxl_mem.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/tools/libs/light/libxl_mem.c b/tools/libs/light/libxl_mem.c
>> index c739d00f39..2f4f9d4a4a 100644
>> --- a/tools/libs/light/libxl_mem.c
>> +++ b/tools/libs/light/libxl_mem.c
>> @@ -82,6 +82,15 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
> 
> There's a comment on this functions:
>      /*
>       * Set the maximum memory size of the domain in the hypervisor. There is no
>       * change of the current memory size involved. The specified memory size can
>       * even be above the configured maxmem size of the domain, but the related
>       * Xenstore entry memory/static-max isn't modified!
>       */
>      int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
> 
> So it was already known that "static-max" wasn't set.
> At the very least, this comment needs updating.

Yes.

> 
>>           goto out;
>>       }
>>   
>> +    rc = libxl__xs_printf(gc, XBT_NULL,
>> +                          GCSPRINTF("%s/memory/static-max", dompath),
>> +                          "%"PRIu64, max_memkb);
>> +    if (rc != 0) {
>> +        LOGED(ERROR, domid, "Couldn't set %s/memory/static-max, rc=%d\n",
>> +              dompath, rc);
>> +        goto out;
> 
> 
> 
> So, I don't know whether increasing "static-max" is fine or not, but
> according to the documentation, it isn't expected.
> 
> Is a guest fine with "static-max" been changed?

Normally it doesn't care at all.

> 
> If yes, there's documentation and comments that needs to change with the
> code change.

I agree. Will do so.


Juergen