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

[Markus Armbruster]

Thomas Huth  writes:

> On 30/05/2024 16.01, Zhao Liu wrote:
>> Hi Thomas,
>> BTW, do you think it's a good idea to define the overcommit via QAPI way
>> (defined in a json file)? ;-)
>> My rough understanding is that all APIs are better to be defined via
>> QAPI to go JSON compatible.
>
> Sorry, no clue whether it makes sense here... CC:-ing Markus for 
> recommendations.

I'd love to have a machine-friendly, QAPI-based CLI with a
human-friendly CLI layered on top, similar to machine-friendly,
QAPI-based QMP / human-friendly HMP.

To get this with reasonable effort, we need better infrastructure.  We
have done a few complex options manually, such as -blockdev.  I
recommend this only when there's a clear need for JSON on the command
line.

I doubt this is the case for -overcommit.

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

[Thomas Huth]

On 30/05/2024 16.01, Zhao Liu wrote:

On Mon, May 27, 2024 at 07:19:56AM +0200, Thomas Huth wrote:

Date: Mon, 27 May 2024 07:19:56 +0200
From: Thomas Huth 
Subject: Re: [PATCH V2 1/3] vl: Allow multiple -overcommit commands

On 24/05/2024 22.00, 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")
Suggested-by: Thomas Huth 
Signed-off-by: Zide Chen 
---

v2:

Thanks to Thomas' suggestion, changed to this better approach, which
is more generic and can handle situations like: "enabled the option in
the config file, and now you'd like to disable it on the command line
again".

   system/vl.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..dfa6cdd9283b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3545,8 +3545,8 @@ 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);
+enable_mlock = qemu_opt_get_bool(opts, "mem-lock", 
enable_mlock);
+enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
enable_cpu_pm);
   break;
   case QEMU_OPTION_compat:
   {


Reviewed-by: Thomas Huth 



Hi Thomas,

BTW, do you think it's a good idea to define the overcommit via QAPI way
(defined in a json file)? ;-)

My rough understanding is that all APIs are better to be defined via
QAPI to go JSON compatible.


Sorry, no clue whether it makes sense here... CC:-ing Markus for 
recommendations.


 Thomas

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

[Zhao Liu]

On Mon, May 27, 2024 at 07:19:56AM +0200, Thomas Huth wrote:
> Date: Mon, 27 May 2024 07:19:56 +0200
> From: Thomas Huth 
> Subject: Re: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
> 
> On 24/05/2024 22.00, 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")
> > Suggested-by: Thomas Huth 
> > Signed-off-by: Zide Chen 
> > ---
> > 
> > v2:
> > 
> > Thanks to Thomas' suggestion, changed to this better approach, which
> > is more generic and can handle situations like: "enabled the option in
> > the config file, and now you'd like to disable it on the command line
> > again".
> > 
> >   system/vl.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/system/vl.c b/system/vl.c
> > index a3eede5fa5b8..dfa6cdd9283b 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -3545,8 +3545,8 @@ 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);
> > +enable_mlock = qemu_opt_get_bool(opts, "mem-lock", 
> > enable_mlock);
> > +enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
> > enable_cpu_pm);
> >   break;
> >   case QEMU_OPTION_compat:
> >   {
> 
> Reviewed-by: Thomas Huth 
> 

Hi Thomas,

BTW, do you think it's a good idea to define the overcommit via QAPI way
(defined in a json file)? ;-)

My rough understanding is that all APIs are better to be defined via
QAPI to go JSON compatible.

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

[Zhao Liu]

On Fri, May 24, 2024 at 01:00:15PM -0700, Zide Chen wrote:
> Date: Fri, 24 May 2024 13:00:15 -0700
> From: Zide Chen 
> Subject: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
> X-Mailer: git-send-email 2.34.1
> 
> 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")
> Suggested-by: Thomas Huth 
> Signed-off-by: Zide Chen 
> ---
> 
> v2:
> 
> Thanks to Thomas' suggestion, changed to this better approach, which
> is more generic and can handle situations like: "enabled the option in
> the config file, and now you'd like to disable it on the command line
> again".
> 
>  system/vl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu 

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

[Thomas Huth]

On 24/05/2024 22.00, 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")
Suggested-by: Thomas Huth 
Signed-off-by: Zide Chen 
---

v2:

Thanks to Thomas' suggestion, changed to this better approach, which
is more generic and can handle situations like: "enabled the option in
the config file, and now you'd like to disable it on the command line
again".

  system/vl.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..dfa6cdd9283b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3545,8 +3545,8 @@ 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);
+enable_mlock = qemu_opt_get_bool(opts, "mem-lock", 
enable_mlock);
+enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
enable_cpu_pm);
  break;
  case QEMU_OPTION_compat:
  {


Reviewed-by: Thomas Huth 

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

[Zide Chen]

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")
Suggested-by: Thomas Huth 
Signed-off-by: Zide Chen 
---

v2:

Thanks to Thomas' suggestion, changed to this better approach, which
is more generic and can handle situations like: "enabled the option in
the config file, and now you'd like to disable it on the command line
again".

 system/vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..dfa6cdd9283b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3545,8 +3545,8 @@ 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);
+enable_mlock = qemu_opt_get_bool(opts, "mem-lock", 
enable_mlock);
+enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
enable_cpu_pm);
 break;
 case QEMU_OPTION_compat:
 {
-- 
2.34.1