| Message ID | 20240524200017.150339-2-zide.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve -overcommit cpu-pm=on|off | expand |
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 <thuth@redhat.com> > Signed-off-by: Zide Chen <zide.chen@intel.com> > --- > > 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 <thuth@redhat.com>
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 <zide.chen@intel.com> > 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 <thuth@redhat.com> > Signed-off-by: Zide Chen <zide.chen@intel.com> > --- > > 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 <zhao1.liu@intel.com>
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 <thuth@redhat.com> > 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 <thuth@redhat.com> > > Signed-off-by: Zide Chen <zide.chen@intel.com> > > --- > > > > 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 <thuth@redhat.com> > 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.
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 <thuth@redhat.com> >> 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 <thuth@redhat.com> >>> Signed-off-by: Zide Chen <zide.chen@intel.com> >>> --- >>> >>> 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 <thuth@redhat.com> >> > > 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
Thomas Huth <thuth@redhat.com> 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.
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: {
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 <thuth@redhat.com> Signed-off-by: Zide Chen <zide.chen@intel.com> --- 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(-)