| Message ID | 20190625161629.302-1-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] deprecate -mem-path fallback to anonymous RAM | expand |
On Tue, Jun 25, 2019 at 12:16:29PM -0400, Igor Mammedov wrote: > Fallback might affect guest or worse whole host performance > or functionality if backing file were used to share guest RAM > with another process. > > Patch deprecates fallback so that we could remove it in future > and ensure that QEMU will provide expected behavior and fail if > it can't use user provided backing file. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > * improve text language > (Markus Armbruster <armbru@redhat.com>) > > numa.c | 6 ++++-- > qemu-deprecated.texi | 9 +++++++++ > 2 files changed, 13 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 6/25/19 11:16 AM, Igor Mammedov wrote: > Fallback might affect guest or worse whole host performance > or functionality if backing file were used to share guest RAM > with another process. > > Patch deprecates fallback so that we could remove it in future > and ensure that QEMU will provide expected behavior and fail if > it can't use user provided backing file. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > * improve text language > (Markus Armbruster <armbru@redhat.com>) > Is this deprecation introspectible? Does it need to be? Do we even need a deprecation period, or can we declare this a bug fix (it was a bug that we didn't fail outright on an impossible request) and do it immediately? If it is not a bug fix, perhaps it could be made introspectible by having a new boolean parameter to opt in to the failure now, rather than 2 releases from now?
On Tue, Jun 25, 2019 at 01:18:01PM -0500, Eric Blake wrote: > On 6/25/19 11:16 AM, Igor Mammedov wrote: > > Fallback might affect guest or worse whole host performance > > or functionality if backing file were used to share guest RAM > > with another process. > > > > Patch deprecates fallback so that we could remove it in future > > and ensure that QEMU will provide expected behavior and fail if > > it can't use user provided backing file. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v2: > > * improve text language > > (Markus Armbruster <armbru@redhat.com>) > > > > Is this deprecation introspectible? Does it need to be? > > Do we even need a deprecation period, or can we declare this a bug fix > (it was a bug that we didn't fail outright on an impossible request) and > do it immediately? I think it is hard to call it a bug when we added explicit extra code to make it work as it does today. It is really a misguided feature. > If it is not a bug fix, perhaps it could be made introspectible by > having a new boolean parameter to opt in to the failure now, rather than > 2 releases from now? From libvirt's POV I don't see a need for introspection. There's no special action we need to take to deal with the new behaviour - it is ultimately just providing the behaviour we kind of assumed it already had. Regards, Daniel
Igor Mammedov <imammedo@redhat.com> writes: > Fallback might affect guest or worse whole host performance > or functionality if backing file were used to share guest RAM > with another process. > > Patch deprecates fallback so that we could remove it in future > and ensure that QEMU will provide expected behavior and fail if > it can't use user provided backing file. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > * improve text language > (Markus Armbruster <armbru@redhat.com>) > > numa.c | 6 ++++-- > qemu-deprecated.texi | 9 +++++++++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/numa.c b/numa.c > index 91a29138a2..c15e53e92d 100644 > --- a/numa.c > +++ b/numa.c > @@ -494,8 +494,10 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner, > if (mem_prealloc) { > exit(1); > } > - error_report("falling back to regular RAM allocation."); > - > + warn_report("falling back to regular RAM allocation"); > + error_printf("This is deprecated. Make sure that -mem-path " > + " specified path has sufficient resources to allocate" > + " -m specified RAM amount or QEMU will fail to start"); The "or QEMU will fail to start" is confusing, I'm afraid. *This* QEMU won't. A future QEMU may. Suggest to simply delete the confusing part. > /* Legacy behavior: if allocation failed, fall back to > * regular RAM allocation. > */ > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > index 2fe9b72121..1b7f3b10dc 100644 > --- a/qemu-deprecated.texi > +++ b/qemu-deprecated.texi > @@ -112,6 +112,15 @@ QEMU using implicit generic or board specific splitting rule. > Use @option{memdev} with @var{memory-backend-ram} backend or @option{mem} (if > it's supported by used machine type) to define mapping explictly instead. > > +@subsection -mem-path fallback to RAM (since 4.1) > +Currently if guest RAM allocation from file pointed by @option{mem-path} > +fails, QEMU falls back to allocating from RAM, which might result > +in unpredictable behavior since the backing file specified by the user > +is ignored. In the future, users will be responsible for making sure > +the backing storage specified with @option{-mem-path} can actually provide > +the guest RAM configured with @option{-m} and fail to start up if RAM allocation > +is unsuccessful. Grammar nit: the subject of "fail to start up" is "users", oopsie. Suggest ", and QEMU will fail to start up". > + > @section QEMU Machine Protocol (QMP) commands > > @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0) With these tweaks: Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/numa.c b/numa.c index 91a29138a2..c15e53e92d 100644 --- a/numa.c +++ b/numa.c @@ -494,8 +494,10 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner, if (mem_prealloc) { exit(1); } - error_report("falling back to regular RAM allocation."); - + warn_report("falling back to regular RAM allocation"); + error_printf("This is deprecated. Make sure that -mem-path " + " specified path has sufficient resources to allocate" + " -m specified RAM amount or QEMU will fail to start"); /* Legacy behavior: if allocation failed, fall back to * regular RAM allocation. */ diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index 2fe9b72121..1b7f3b10dc 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -112,6 +112,15 @@ QEMU using implicit generic or board specific splitting rule. Use @option{memdev} with @var{memory-backend-ram} backend or @option{mem} (if it's supported by used machine type) to define mapping explictly instead. +@subsection -mem-path fallback to RAM (since 4.1) +Currently if guest RAM allocation from file pointed by @option{mem-path} +fails, QEMU falls back to allocating from RAM, which might result +in unpredictable behavior since the backing file specified by the user +is ignored. In the future, users will be responsible for making sure +the backing storage specified with @option{-mem-path} can actually provide +the guest RAM configured with @option{-m} and fail to start up if RAM allocation +is unsuccessful. + @section QEMU Machine Protocol (QMP) commands @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
Fallback might affect guest or worse whole host performance or functionality if backing file were used to share guest RAM with another process. Patch deprecates fallback so that we could remove it in future and ensure that QEMU will provide expected behavior and fail if it can't use user provided backing file. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: * improve text language (Markus Armbruster <armbru@redhat.com>) numa.c | 6 ++++-- qemu-deprecated.texi | 9 +++++++++ 2 files changed, 13 insertions(+), 2 deletions(-)