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

[Thomas Huth]

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