1. Patchew
  2. QEMU
  3. [PATCH 0/3] improve -overcommit cpu-pm=on|off
  4. View patch

[PATCH 1/3] vl: Allow multiple -overcommit commands

Zide Chen posted 3 patches 6 months, 1 week ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>
There is a newer version of this series
[PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Zide Chen 6 months, 1 week ago 
Both cpu-pm and mem-lock are related to system resource overcommit, but
they are separate from each other, in terms of how they are realized,
and of course, they are applied to different system resources.

It's tempting to use separate command lines to specify their behavior.
e.g., in the following example, the cpu-pm command is quietly
overwritten, and it's not easy to notice it without careful inspection.

  --overcommit mem-lock=on
  --overcommit cpu-pm=on

Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
Signed-off-by: Zide Chen <zide.chen@intel.com>
---
 system/vl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..ed682643805b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
                 if (!opts) {
                     exit(1);
                 }
-                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
-                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
+
+                /* Don't override the -overcommit option if set */
+                enable_mlock = enable_mlock ||
+                    qemu_opt_get_bool(opts, "mem-lock", false);
+                enable_cpu_pm = enable_cpu_pm ||
+                    qemu_opt_get_bool(opts, "cpu-pm", false);
                 break;
             case QEMU_OPTION_compat:
                 {
-- 
2.34.1
Re: [PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Thomas Huth 6 months, 1 week ago 
On 20/05/2024 19.47, Zide Chen wrote:
> Both cpu-pm and mem-lock are related to system resource overcommit, but
> they are separate from each other, in terms of how they are realized,
> and of course, they are applied to different system resources.
> 
> It's tempting to use separate command lines to specify their behavior.
> e.g., in the following example, the cpu-pm command is quietly
> overwritten, and it's not easy to notice it without careful inspection.
> 
>    --overcommit mem-lock=on
>    --overcommit cpu-pm=on
> 
> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
>   system/vl.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/system/vl.c b/system/vl.c
> index a3eede5fa5b8..ed682643805b 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>                   if (!opts) {
>                       exit(1);
>                   }
> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
> +
> +                /* Don't override the -overcommit option if set */
> +                enable_mlock = enable_mlock ||
> +                    qemu_opt_get_bool(opts, "mem-lock", false);
> +                enable_cpu_pm = enable_cpu_pm ||
> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>                   break;
>               case QEMU_OPTION_compat:
>                   {

Reviewed-by: Thomas Huth <thuth@redhat.com>
Re: [PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Thomas Huth 6 months, 1 week ago 
On 21/05/2024 07.08, Thomas Huth wrote:
> On 20/05/2024 19.47, Zide Chen wrote:
>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>> they are separate from each other, in terms of how they are realized,
>> and of course, they are applied to different system resources.
>>
>> It's tempting to use separate command lines to specify their behavior.
>> e.g., in the following example, the cpu-pm command is quietly
>> overwritten, and it's not easy to notice it without careful inspection.
>>
>>    --overcommit mem-lock=on
>>    --overcommit cpu-pm=on
>>
>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>> ---
>>   system/vl.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index a3eede5fa5b8..ed682643805b 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>                   if (!opts) {
>>                       exit(1);
>>                   }
>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
>> +
>> +                /* Don't override the -overcommit option if set */
>> +                enable_mlock = enable_mlock ||
>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>> +                enable_cpu_pm = enable_cpu_pm ||
>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>                   break;
>>               case QEMU_OPTION_compat:
>>                   {
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Ah, wait, actually, this is a bad idea, too, since now you cannot disable an 
enabled option anymore. Imagine that you have for example enabled the option 
in the config file, and now you'd like to disable it on the command line 
again - you're stuck with the enabled setting in that case.

I think the better solution is to replace the "false" default value at the end:

         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);

What do you think about this?

  Thomas
Re: [PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Chen, Zide 6 months, 1 week ago 

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 
> What do you think about this?

This is great! Thank you very much! I will update it in V2.

> 
>  Thomas
>
Re: [PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Chen, Zide 6 months, 1 week ago 

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 
> What do you think about this?

Thank you very much! This is a very good idea! I can update it in V2.

>  Thomas
>
Re: [PATCH 1/3] vl: Allow multiple -overcommit commands
Posted by Chen, Zide 6 months, 1 week ago 

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 

> What do you think about this?

Thank you very much! This is a very good idea, and I can update it in V2.

> 
>  Thomas
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.