| Message ID | 20240520174733.32979-2-zide.chen@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | improve -overcommit cpu-pm=on|off | expand |
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>
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
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 >
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 >
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 >
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: {
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(-)