Re: [PATCH 2/2] Add -mem-shared option
On Tue, 10 Dec 2019 11:34:32 +0100 Markus Armbruster wrote: > Eduardo Habkost writes: > > > +Markus > > > > On Tue, Dec 03, 2019 at 03:43:03PM +0100, Igor Mammedov wrote: > >> On Tue, 3 Dec 2019 09:56:15 +0100 > >> Thomas Huth wrote: > >> > >> > On 02/12/2019 22.00, Eduardo Habkost wrote: > >> > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: > >> > >> On Fri, 29 Nov 2019 18:46:12 +0100 > >> > >> Paolo Bonzini wrote: > >> > >> > >> > >>> On 29/11/19 13:16, Igor Mammedov wrote: > >> > As for "-m", I'd make it just an alias that translates > >> > -m/mem-path/mem-prealloc > >> > >>> > >> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. > >> > >>> CCing > >> > >>> Thomas as mister deprecation. :) > >> > >> > >> > >> I'll add that to my series > >> > > > >> > > Considering that the plan is to eventually reimplement those > >> > > options as syntactic sugar for memory backend options (hopefully > >> > > in less than 2 QEMU releases), what's the point of deprecating > >> > > them? > >> > > >> > Well, it depends on the "classification" [1] of the parameter... > >> > > >> > Let's ask: What's the main purpose of the option? > >> > > >> > Is it easier to use than the "full" option, and thus likely to be used > >> > by a lot of people who run QEMU directly from the CLI? In that case it > >> > should stay as "convenience option" and not be deprecated. > >> > > >> > Or is the option merely there to give the upper layers like libvirt or > >> > some few users and their scripts some more grace period to adapt their > >> > code, but we all agree that the options are rather ugly and should > >> > finally go away? Then it's rather a "legacy option" and the deprecation > >> > process is the right way to go. Our QEMU interface is still way > >> > overcrowded, we should try to keep it as clean as possible. > >> > >> After switching to memdev for main RAM, users could use relatively > >> short global options > >> -global memory-backend.prealloc|share=on > >> and > >> -global memory-backend-file.mem-path=X|prealloc|share=on > >> > >> instead of us adding and maintaining slightly shorter > >> -mem-shared/-mem-path/-mem-prealloc > > > > Global properties are a convenient way to expose knobs through > > the command line with little effort, but we have no documentation > > on which QOM properties are really supposed to be touched by > > users using -global. > > > > Unless we fix the lack of documentation, I'd prefer to have > > syntactic sugar translated to -global instead of recommending > > direct usage of -global. > > Fair point. > > I'd take QOM property documentation over still more sugar. > > Sometimes, the practical way to make simple things simple is sugar. I > can accept that. This doesn't look like such a case, though. I can document concrete globals as replacement at the place -mem-path/-mem-prealloc are documented during deprecation and then in 2 releases we will just drop legacy syntax and keep only globals over there. (eventually it will spread various globals over man page, which I don't like but we probably should start somwhere and consolidate later if globals in man page become normal practice.)
Re: [PATCH 2/2] Add -mem-shared option
Eduardo Habkost writes: > +Markus > > On Tue, Dec 03, 2019 at 03:43:03PM +0100, Igor Mammedov wrote: >> On Tue, 3 Dec 2019 09:56:15 +0100 >> Thomas Huth wrote: >> >> > On 02/12/2019 22.00, Eduardo Habkost wrote: >> > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: >> > >> On Fri, 29 Nov 2019 18:46:12 +0100 >> > >> Paolo Bonzini wrote: >> > >> >> > >>> On 29/11/19 13:16, Igor Mammedov wrote: >> > As for "-m", I'd make it just an alias that translates >> > -m/mem-path/mem-prealloc >> > >>> >> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing >> > >>> Thomas as mister deprecation. :) >> > >> >> > >> I'll add that to my series >> > > >> > > Considering that the plan is to eventually reimplement those >> > > options as syntactic sugar for memory backend options (hopefully >> > > in less than 2 QEMU releases), what's the point of deprecating >> > > them? >> > >> > Well, it depends on the "classification" [1] of the parameter... >> > >> > Let's ask: What's the main purpose of the option? >> > >> > Is it easier to use than the "full" option, and thus likely to be used >> > by a lot of people who run QEMU directly from the CLI? In that case it >> > should stay as "convenience option" and not be deprecated. >> > >> > Or is the option merely there to give the upper layers like libvirt or >> > some few users and their scripts some more grace period to adapt their >> > code, but we all agree that the options are rather ugly and should >> > finally go away? Then it's rather a "legacy option" and the deprecation >> > process is the right way to go. Our QEMU interface is still way >> > overcrowded, we should try to keep it as clean as possible. >> >> After switching to memdev for main RAM, users could use relatively >> short global options >> -global memory-backend.prealloc|share=on >> and >> -global memory-backend-file.mem-path=X|prealloc|share=on >> >> instead of us adding and maintaining slightly shorter >> -mem-shared/-mem-path/-mem-prealloc > > Global properties are a convenient way to expose knobs through > the command line with little effort, but we have no documentation > on which QOM properties are really supposed to be touched by > users using -global. > > Unless we fix the lack of documentation, I'd prefer to have > syntactic sugar translated to -global instead of recommending > direct usage of -global. Fair point. I'd take QOM property documentation over still more sugar. Sometimes, the practical way to make simple things simple is sugar. I can accept that. This doesn't look like such a case, though.
Re: [PATCH 2/2] Add -mem-shared option
+Markus On Tue, Dec 03, 2019 at 03:43:03PM +0100, Igor Mammedov wrote: > On Tue, 3 Dec 2019 09:56:15 +0100 > Thomas Huth wrote: > > > On 02/12/2019 22.00, Eduardo Habkost wrote: > > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: > > >> On Fri, 29 Nov 2019 18:46:12 +0100 > > >> Paolo Bonzini wrote: > > >> > > >>> On 29/11/19 13:16, Igor Mammedov wrote: > > As for "-m", I'd make it just an alias that translates > > -m/mem-path/mem-prealloc > > >>> > > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing > > >>> Thomas as mister deprecation. :) > > >> > > >> I'll add that to my series > > > > > > Considering that the plan is to eventually reimplement those > > > options as syntactic sugar for memory backend options (hopefully > > > in less than 2 QEMU releases), what's the point of deprecating > > > them? > > > > Well, it depends on the "classification" [1] of the parameter... > > > > Let's ask: What's the main purpose of the option? > > > > Is it easier to use than the "full" option, and thus likely to be used > > by a lot of people who run QEMU directly from the CLI? In that case it > > should stay as "convenience option" and not be deprecated. > > > > Or is the option merely there to give the upper layers like libvirt or > > some few users and their scripts some more grace period to adapt their > > code, but we all agree that the options are rather ugly and should > > finally go away? Then it's rather a "legacy option" and the deprecation > > process is the right way to go. Our QEMU interface is still way > > overcrowded, we should try to keep it as clean as possible. > > After switching to memdev for main RAM, users could use relatively > short global options > -global memory-backend.prealloc|share=on > and > -global memory-backend-file.mem-path=X|prealloc|share=on > > instead of us adding and maintaining slightly shorter > -mem-shared/-mem-path/-mem-prealloc Global properties are a convenient way to expose knobs through the command line with little effort, but we have no documentation on which QOM properties are really supposed to be touched by users using -global. Unless we fix the lack of documentation, I'd prefer to have syntactic sugar translated to -global instead of recommending direct usage of -global. -- Eduardo
Re: [PATCH 2/2] Add -mem-shared option
On Tue, Dec 03, 2019 at 09:56:15AM +0100, Thomas Huth wrote: > On 02/12/2019 22.00, Eduardo Habkost wrote: > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: > >> On Fri, 29 Nov 2019 18:46:12 +0100 > >> Paolo Bonzini wrote: > >> > >>> On 29/11/19 13:16, Igor Mammedov wrote: > As for "-m", I'd make it just an alias that translates > -m/mem-path/mem-prealloc > >>> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing > >>> Thomas as mister deprecation. :) > >> > >> I'll add that to my series > > > > Considering that the plan is to eventually reimplement those > > options as syntactic sugar for memory backend options (hopefully > > in less than 2 QEMU releases), what's the point of deprecating > > them? > > Well, it depends on the "classification" [1] of the parameter... > > Let's ask: What's the main purpose of the option? > > Is it easier to use than the "full" option, and thus likely to be used > by a lot of people who run QEMU directly from the CLI? In that case it > should stay as "convenience option" and not be deprecated. > > Or is the option merely there to give the upper layers like libvirt or > some few users and their scripts some more grace period to adapt their > code, but we all agree that the options are rather ugly and should > finally go away? Then it's rather a "legacy option" and the deprecation > process is the right way to go. Our QEMU interface is still way to > overcrowded, we should try to keep it as clean as possible. That's a good way to describe the questions involved. To me they are clearly convenience options. We could still replace them with new (more consistent and less ugly) convenience options, though. > > Thomas > > > [1] Using the terms from: > https://www.youtube.com/watch?v=Oscjpkns7tM=8m -- Eduardo
Re: [PATCH 2/2] Add -mem-shared option
On Tue, 3 Dec 2019 09:56:15 +0100 Thomas Huth wrote: > On 02/12/2019 22.00, Eduardo Habkost wrote: > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: > >> On Fri, 29 Nov 2019 18:46:12 +0100 > >> Paolo Bonzini wrote: > >> > >>> On 29/11/19 13:16, Igor Mammedov wrote: > As for "-m", I'd make it just an alias that translates > -m/mem-path/mem-prealloc > >>> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing > >>> Thomas as mister deprecation. :) > >> > >> I'll add that to my series > > > > Considering that the plan is to eventually reimplement those > > options as syntactic sugar for memory backend options (hopefully > > in less than 2 QEMU releases), what's the point of deprecating > > them? > > Well, it depends on the "classification" [1] of the parameter... > > Let's ask: What's the main purpose of the option? > > Is it easier to use than the "full" option, and thus likely to be used > by a lot of people who run QEMU directly from the CLI? In that case it > should stay as "convenience option" and not be deprecated. > > Or is the option merely there to give the upper layers like libvirt or > some few users and their scripts some more grace period to adapt their > code, but we all agree that the options are rather ugly and should > finally go away? Then it's rather a "legacy option" and the deprecation > process is the right way to go. Our QEMU interface is still way > overcrowded, we should try to keep it as clean as possible. After switching to memdev for main RAM, users could use relatively short global options -global memory-backend.prealloc|share=on and -global memory-backend-file.mem-path=X|prealloc|share=on instead of us adding and maintaining slightly shorter -mem-shared/-mem-path/-mem-prealloc > Thomas > > > [1] Using the terms from: > https://www.youtube.com/watch?v=Oscjpkns7tM=8m
Re: [PATCH 2/2] Add -mem-shared option
On 02/12/2019 22.00, Eduardo Habkost wrote: > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: >> On Fri, 29 Nov 2019 18:46:12 +0100 >> Paolo Bonzini wrote: >> >>> On 29/11/19 13:16, Igor Mammedov wrote: As for "-m", I'd make it just an alias that translates -m/mem-path/mem-prealloc >>> >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing >>> Thomas as mister deprecation. :) >> >> I'll add that to my series > > Considering that the plan is to eventually reimplement those > options as syntactic sugar for memory backend options (hopefully > in less than 2 QEMU releases), what's the point of deprecating > them? Well, it depends on the "classification" [1] of the parameter... Let's ask: What's the main purpose of the option? Is it easier to use than the "full" option, and thus likely to be used by a lot of people who run QEMU directly from the CLI? In that case it should stay as "convenience option" and not be deprecated. Or is the option merely there to give the upper layers like libvirt or some few users and their scripts some more grace period to adapt their code, but we all agree that the options are rather ugly and should finally go away? Then it's rather a "legacy option" and the deprecation process is the right way to go. Our QEMU interface is still way to overcrowded, we should try to keep it as clean as possible. Thomas [1] Using the terms from: https://www.youtube.com/watch?v=Oscjpkns7tM=8m
Re: [PATCH 2/2] Add -mem-shared option
On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote: > On Fri, 29 Nov 2019 18:46:12 +0100 > Paolo Bonzini wrote: > > > On 29/11/19 13:16, Igor Mammedov wrote: > > > As for "-m", I'd make it just an alias that translates > > > -m/mem-path/mem-prealloc > > > > I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing > > Thomas as mister deprecation. :) > > I'll add that to my series Considering that the plan is to eventually reimplement those options as syntactic sugar for memory backend options (hopefully in less than 2 QEMU releases), what's the point of deprecating them? -- Eduardo
Re: [PATCH 2/2] Add -mem-shared option
On Fri, 29 Nov 2019 18:46:12 +0100 Paolo Bonzini wrote: > On 29/11/19 13:16, Igor Mammedov wrote: > > As for "-m", I'd make it just an alias that translates > > -m/mem-path/mem-prealloc > > I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing > Thomas as mister deprecation. :) I'll add that to my series [...]
Re: [PATCH 2/2] Add -mem-shared option
On Fri, Nov 29, 2019 at 01:01:54PM +0100, Markus Armbruster wrote:
> Paolo Bonzini writes:
>
> > On 29/11/19 11:07, Igor Mammedov wrote:
> So user who wants something non trivial could override default
> non-numa behavior with
> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
> -machine memdev=mem
> or use any other backend that suits theirs needs.
> >>> That's nice, but not as friendly as a simple -mem-shared.
> >> (I still do not like idea of convenience options but it won't
> >> get onto the way much if implemented as "global property" to memdev,
> >> so I won't object if there is real demand for it)
> >
> > I agree with Igor, we should always think about the generic ("object
> > model") options and only then add convenience option.
>
> +1
I agree with this. I just hope we don't forget about the second
part.
--
Eduardo
Re: [PATCH 2/2] Add -mem-shared option
On 29/11/19 13:16, Igor Mammedov wrote: > As for "-m", I'd make it just an alias that translates > -m/mem-path/mem-prealloc I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing Thomas as mister deprecation. :) > combination to appropriate '-object' for '-machine memdev' consumption. > That should cover compat purposes for old machines and the rest of > -m options (maxmem/slots) would be aliased to appropriate machine options. > > That will allow us to get rid of ad-hoc '-m' parser. After that it would > be possible to deprecate '-m' in favor of machine properties, but that > probably will get quite a push back so unless I find compelling reason > to do it I won't care much as '-m' would be a lightweight shim over > machine properties. Well, deprecation and ultimately removal is always a long path. However, I understand your plan better now and having "-machine memdev" makes sense if you want to also move/alias "-m maxmem"/"-m slots" from "-m" to "-machine". So ultimately you'd have two ways of configuring memory: - -m N,maxmem=P,slots=Q " -machine memdev=M,maxmem=P,slots=Q Paolo
Re: [PATCH 2/2] Add -mem-shared option
Paolo Bonzini writes:
> On 29/11/19 11:07, Igor Mammedov wrote:
So user who wants something non trivial could override default
non-numa behavior with
-object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
-machine memdev=mem
or use any other backend that suits theirs needs.
>>> That's nice, but not as friendly as a simple -mem-shared.
>> (I still do not like idea of convenience options but it won't
>> get onto the way much if implemented as "global property" to memdev,
>> so I won't object if there is real demand for it)
>
> I agree with Igor, we should always think about the generic ("object
> model") options and only then add convenience option.
+1
[...]
Re: [PATCH 2/2] Add -mem-shared option
On Fri, 29 Nov 2019 11:11:09 +0100
Paolo Bonzini wrote:
> On 29/11/19 11:07, Igor Mammedov wrote:
> >>> So user who wants something non trivial could override default
> >>> non-numa behavior with
> >>> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
> >>> -machine memdev=mem
> >>> or use any other backend that suits theirs needs.
> >> That's nice, but not as friendly as a simple -mem-shared.
> > (I still do not like idea of convenience options but it won't
> > get onto the way much if implemented as "global property" to memdev,
> > so I won't object if there is real demand for it)
>
> I agree with Igor, we should always think about the generic ("object
> model") options and only then add convenience option.
>
> It looks like the remaining point is to decide between "-m memdev" and
> "-machine memdev".
I'm still entertaining idea, to use -device pc-dimm|some_ram_dev
for main RAM but that's not generic enough so I'd probably post
'-machine memdev' variant for now and think some more on -device
(we can add front-end re-factoring if necessary on top).
As for "-m", I'd make it just an alias that translates
-m/mem-path/mem-prealloc
combination to appropriate '-object' for '-machine memdev' consumption.
That should cover compat purposes for old machines and the rest of
-m options (maxmem/slots) would be aliased to appropriate machine options.
That will allow us to get rid of ad-hoc '-m' parser. After that it would
be possible to deprecate '-m' in favor of machine properties, but that
probably will get quite a push back so unless I find compelling reason
to do it I won't care much as '-m' would be a lightweight shim over
machine properties.
>
> Paolo
>
Re: [PATCH 2/2] Add -mem-shared option
On 29/11/19 11:07, Igor Mammedov wrote:
>>> So user who wants something non trivial could override default
>>> non-numa behavior with
>>> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
>>> -machine memdev=mem
>>> or use any other backend that suits theirs needs.
>> That's nice, but not as friendly as a simple -mem-shared.
> (I still do not like idea of convenience options but it won't
> get onto the way much if implemented as "global property" to memdev,
> so I won't object if there is real demand for it)
I agree with Igor, we should always think about the generic ("object
model") options and only then add convenience option.
It looks like the remaining point is to decide between "-m memdev" and
"-machine memdev".
Paolo
Re: [PATCH 2/2] Add -mem-shared option
On Fri, 29 Nov 2019 00:31:32 +0400
Marc-André Lureau wrote:
> Hi
>
> On Thu, Nov 28, 2019 at 9:25 PM Igor Mammedov wrote:
> >
> > On Thu, 28 Nov 2019 18:15:18 +0400
> > Marc-André Lureau wrote:
> >
> > > Add an option to simplify shared memory / vhost-user setup.
> > >
> > > Currently, using vhost-user requires NUMA setup such as:
> > > -m 4G -object
> > > memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on -numa
> > > node,memdev=mem
> > >
> > > As there is no other way to allocate shareable RAM, afaik.
> > >
> > > -mem-shared aims to have a simple way instead: -m 4G -mem-shared
> > User always can write a wrapper script if verbose CLI is too much,
> > and we won't have to deal with myriad permutations to maintain.
>
> Sure, but that's not exactly making it easier for the user,
> documentation etc (or machine that do not support numa as David
> mentionned).
on positive side it makes behavior consistent (and need to be documented
in one place (memdev)), i.e. user knows that he will get what he specified
on CLI.
With global options like you propose, good luck with figuring out
(and documenting it in user documentation will be nightmare)
if your config actually does what it's supposed to do or not.
That's unfortunate the state we are in right now with
mem-path/prealloc.
> > Also current -mem-path/prealloc in combination with memdevs is
> > the source of problems (as ram allocation uses 2 different paths).
> > It's possible to fix with a kludge but I'd rather fix it properly.
>
> I agree, however I think it's a separate problems. We don't have to
> fix both simultaneously. The semantic of a new CLI -mem-shared (or
> shared=on etc) can be defined and implemented in a simple way, before
> internal refactoring.
Well, it adds more invariants to already complex RAM allocation
I have to untangle. So lets look into it after memdev re-factoring.
I'll plan to post it right after merge window is open.
> > So during 5.0, I'm planning to consolidate -mem-path/prealloc
> > handling around memory backend internally (and possibly deprecate them),
> > so the only way to allocate RAM for guest would be via memdevs.
> > (reducing number of options an globals that they use)
> >
>
> That would be great indeed. I tried to look at that in the past, but
> was a it overwhelmed by the amount of details and/or code complexity.
Complexity is only one side of issue, I'm mainly refactoring it because
different approaches to allocate RAM lead to subtle bugs that's hard to
spot and fix. That's why I'm against adding more invariants to the pile.
> > So user who wants something non trivial could override default
> > non-numa behavior with
> > -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
> > -machine memdev=mem
> > or use any other backend that suits theirs needs.
>
> That's nice, but not as friendly as a simple -mem-shared.
(I still do not like idea of convenience options but it won't
get onto the way much if implemented as "global property" to memdev,
so I won't object if there is real demand for it)
Simplicity in global options is deceiving once you have mixed
RAM (main and as -device), good luck with documenting how it
works in all possible configs and making it robust (so it
won't explode later) (I wouldn't take on such task).
And then poor user will have to figure it out as well.
Verbose way makes allocating backing storage consistent,
which is much more important (including end user).
The rest could be scripted or one could use higher level
mgmt tools if simplicity is desired.
> thanks
>
> >
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > > exec.c | 11 ++-
> > > hw/core/numa.c | 16 +++-
> > > include/sysemu/sysemu.h | 1 +
> > > qemu-options.hx | 10 ++
> > > vl.c| 4
> > > 5 files changed, 40 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/exec.c b/exec.c
> > > index ffdb518535..4e53937eaf 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -72,6 +72,10 @@
> > > #include "qemu/mmap-alloc.h"
> > > #endif
> > >
> > > +#ifdef CONFIG_POSIX
> > > +#include "qemu/memfd.h"
> > > +#endif
> > > +
> > > #include "monitor/monitor.h"
> > >
> > > //#define DEBUG_SUBPAGE
> > > @@ -2347,7 +2351,12 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t
> > > size, MemoryRegion *mr,
> > > bool created;
> > > RAMBlock *block;
> > >
> > > -fd = file_ram_open(mem_path, memory_region_name(mr), , errp);
> > > +if (mem_path) {
> > > +fd = file_ram_open(mem_path, memory_region_name(mr), ,
> > > errp);
> > > +} else {
> > > +fd = qemu_memfd_open(mr->name, size,
> > > + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > errp);
> > > +}
> >
> > that's what I'm mostly against, as it spills out memdev impl. details
> > into generic code.
> >
> > > if (fd < 0) {
> > > return NULL;
> > > }
> >
Re: [PATCH 2/2] Add -mem-shared option
Hi
On Thu, Nov 28, 2019 at 9:25 PM Igor Mammedov wrote:
>
> On Thu, 28 Nov 2019 18:15:18 +0400
> Marc-André Lureau wrote:
>
> > Add an option to simplify shared memory / vhost-user setup.
> >
> > Currently, using vhost-user requires NUMA setup such as:
> > -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> > -numa node,memdev=mem
> >
> > As there is no other way to allocate shareable RAM, afaik.
> >
> > -mem-shared aims to have a simple way instead: -m 4G -mem-shared
> User always can write a wrapper script if verbose CLI is too much,
> and we won't have to deal with myriad permutations to maintain.
Sure, but that's not exactly making it easier for the user,
documentation etc (or machine that do not support numa as David
mentionned).
>
> Also current -mem-path/prealloc in combination with memdevs is
> the source of problems (as ram allocation uses 2 different paths).
> It's possible to fix with a kludge but I'd rather fix it properly.
I agree, however I think it's a separate problems. We don't have to
fix both simultaneously. The semantic of a new CLI -mem-shared (or
shared=on etc) can be defined and implemented in a simple way, before
internal refactoring.
> So during 5.0, I'm planning to consolidate -mem-path/prealloc
> handling around memory backend internally (and possibly deprecate them),
> so the only way to allocate RAM for guest would be via memdevs.
> (reducing number of options an globals that they use)
>
That would be great indeed. I tried to look at that in the past, but
was a it overwhelmed by the amount of details and/or code complexity.
> So user who wants something non trivial could override default
> non-numa behavior with
> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
> -machine memdev=mem
> or use any other backend that suits theirs needs.
That's nice, but not as friendly as a simple -mem-shared.
thanks
>
> > Signed-off-by: Marc-André Lureau
> > ---
> > exec.c | 11 ++-
> > hw/core/numa.c | 16 +++-
> > include/sysemu/sysemu.h | 1 +
> > qemu-options.hx | 10 ++
> > vl.c| 4
> > 5 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index ffdb518535..4e53937eaf 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -72,6 +72,10 @@
> > #include "qemu/mmap-alloc.h"
> > #endif
> >
> > +#ifdef CONFIG_POSIX
> > +#include "qemu/memfd.h"
> > +#endif
> > +
> > #include "monitor/monitor.h"
> >
> > //#define DEBUG_SUBPAGE
> > @@ -2347,7 +2351,12 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
> > MemoryRegion *mr,
> > bool created;
> > RAMBlock *block;
> >
> > -fd = file_ram_open(mem_path, memory_region_name(mr), , errp);
> > +if (mem_path) {
> > +fd = file_ram_open(mem_path, memory_region_name(mr), ,
> > errp);
> > +} else {
> > +fd = qemu_memfd_open(mr->name, size,
> > + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > errp);
> > +}
>
> that's what I'm mostly against, as it spills out memdev impl. details
> into generic code.
>
> > if (fd < 0) {
> > return NULL;
> > }
> > diff --git a/hw/core/numa.c b/hw/core/numa.c
> > index e3332a984f..6f72cddb1c 100644
> > --- a/hw/core/numa.c
> > +++ b/hw/core/numa.c
> > @@ -493,7 +493,8 @@ static void allocate_system_memory_nonnuma(MemoryRegion
> > *mr, Object *owner,
> > if (mem_path) {
> > #ifdef __linux__
> > Error *err = NULL;
> > -memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, 0,
> > +memory_region_init_ram_from_file(mr, owner, name, ram_size, 0,
> > + mem_shared ? RAM_SHARED : 0,
> > mem_path, );
> this will be gone and replaced by memory region that memdev initializes.
>
> > if (err) {
> > error_report_err(err);
> > @@ -513,6 +514,19 @@ static void
> > allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> > #else
> > fprintf(stderr, "-mem-path not supported on this host\n");
> > exit(1);
> > +#endif
> > +} else if (mem_shared) {
> > +#ifdef CONFIG_POSIX
> > +Error *err = NULL;
> > +memory_region_init_ram_from_file(mr, owner, NULL, ram_size, 0,
> > + RAM_SHARED, NULL, );
> > +if (err) {
> > +error_report_err(err);
> > +exit(1);
> > +}
> > +#else
> > +fprintf(stderr, "-mem-shared not supported on this host\n");
> > +exit(1);
> > #endif
> > } else {
> > memory_region_init_ram_nomigrate(mr, owner, name, ram_size,
> > _fatal);
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 80c57fdc4e..80db8465a9 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -55,6 +55,7 @@ extern bool enable_cpu_pm;
> > extern
Re: [PATCH 2/2] Add -mem-shared option
On Thu, 28 Nov 2019 18:15:18 +0400
Marc-André Lureau wrote:
> Add an option to simplify shared memory / vhost-user setup.
>
> Currently, using vhost-user requires NUMA setup such as:
> -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> -numa node,memdev=mem
>
> As there is no other way to allocate shareable RAM, afaik.
>
> -mem-shared aims to have a simple way instead: -m 4G -mem-shared
User always can write a wrapper script if verbose CLI is too much,
and we won't have to deal with myriad permutations to maintain.
Also current -mem-path/prealloc in combination with memdevs is
the source of problems (as ram allocation uses 2 different paths).
It's possible to fix with a kludge but I'd rather fix it properly.
So during 5.0, I'm planning to consolidate -mem-path/prealloc
handling around memory backend internally (and possibly deprecate them),
so the only way to allocate RAM for guest would be via memdevs.
(reducing number of options an globals that they use)
So user who wants something non trivial could override default
non-numa behavior with
-object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
-machine memdev=mem
or use any other backend that suits theirs needs.
> Signed-off-by: Marc-André Lureau
> ---
> exec.c | 11 ++-
> hw/core/numa.c | 16 +++-
> include/sysemu/sysemu.h | 1 +
> qemu-options.hx | 10 ++
> vl.c| 4
> 5 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index ffdb518535..4e53937eaf 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -72,6 +72,10 @@
> #include "qemu/mmap-alloc.h"
> #endif
>
> +#ifdef CONFIG_POSIX
> +#include "qemu/memfd.h"
> +#endif
> +
> #include "monitor/monitor.h"
>
> //#define DEBUG_SUBPAGE
> @@ -2347,7 +2351,12 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
> MemoryRegion *mr,
> bool created;
> RAMBlock *block;
>
> -fd = file_ram_open(mem_path, memory_region_name(mr), , errp);
> +if (mem_path) {
> +fd = file_ram_open(mem_path, memory_region_name(mr), , errp);
> +} else {
> +fd = qemu_memfd_open(mr->name, size,
> + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> errp);
> +}
that's what I'm mostly against, as it spills out memdev impl. details
into generic code.
> if (fd < 0) {
> return NULL;
> }
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index e3332a984f..6f72cddb1c 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -493,7 +493,8 @@ static void allocate_system_memory_nonnuma(MemoryRegion
> *mr, Object *owner,
> if (mem_path) {
> #ifdef __linux__
> Error *err = NULL;
> -memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, 0,
> +memory_region_init_ram_from_file(mr, owner, name, ram_size, 0,
> + mem_shared ? RAM_SHARED : 0,
> mem_path, );
this will be gone and replaced by memory region that memdev initializes.
> if (err) {
> error_report_err(err);
> @@ -513,6 +514,19 @@ static void allocate_system_memory_nonnuma(MemoryRegion
> *mr, Object *owner,
> #else
> fprintf(stderr, "-mem-path not supported on this host\n");
> exit(1);
> +#endif
> +} else if (mem_shared) {
> +#ifdef CONFIG_POSIX
> +Error *err = NULL;
> +memory_region_init_ram_from_file(mr, owner, NULL, ram_size, 0,
> + RAM_SHARED, NULL, );
> +if (err) {
> +error_report_err(err);
> +exit(1);
> +}
> +#else
> +fprintf(stderr, "-mem-shared not supported on this host\n");
> +exit(1);
> #endif
> } else {
> memory_region_init_ram_nomigrate(mr, owner, name, ram_size,
> _fatal);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 80c57fdc4e..80db8465a9 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -55,6 +55,7 @@ extern bool enable_cpu_pm;
> extern QEMUClockType rtc_clock;
> extern const char *mem_path;
> extern int mem_prealloc;
> +extern int mem_shared;
>
> #define MAX_OPTION_ROMS 16
> typedef struct QEMUOptionRom {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 65c9473b73..4c69b03ad3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -394,6 +394,16 @@ STEXI
> Preallocate memory when using -mem-path.
> ETEXI
>
> +DEF("mem-shared", 0, QEMU_OPTION_mem_shared,
> +"-mem-shared allocate shared memory\n", QEMU_ARCH_ALL)
> +STEXI
> +@item -mem-shared
> +@findex -mem-shared
> +Allocate guest RAM with shared mapping. Whether the allocation is
> +anonymous or not (with -mem-path), QEMU will allocate a shared memory that
> +can be shared by unrelated processes, such as vhost-user backends.
> +ETEXI
> +
> DEF("k", HAS_ARG, QEMU_OPTION_k,
> "-k language
Re: [PATCH 2/2] Add -mem-shared option
+Igor
On Thu, Nov 28, 2019 at 06:15:18PM +0400, Marc-André Lureau wrote:
> Add an option to simplify shared memory / vhost-user setup.
>
> Currently, using vhost-user requires NUMA setup such as:
> -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> -numa node,memdev=mem
>
> As there is no other way to allocate shareable RAM, afaik.
>
> -mem-shared aims to have a simple way instead: -m 4G -mem-shared
>
> Signed-off-by: Marc-André Lureau
> ---
[...]
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index e3332a984f..6f72cddb1c 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -493,7 +493,8 @@ static void allocate_system_memory_nonnuma(MemoryRegion
> *mr, Object *owner,
> if (mem_path) {
> #ifdef __linux__
> Error *err = NULL;
> -memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, 0,
> +memory_region_init_ram_from_file(mr, owner, name, ram_size, 0,
> + mem_shared ? RAM_SHARED : 0,
> mem_path, );
> if (err) {
> error_report_err(err);
> @@ -513,6 +514,19 @@ static void allocate_system_memory_nonnuma(MemoryRegion
> *mr, Object *owner,
> #else
> fprintf(stderr, "-mem-path not supported on this host\n");
> exit(1);
> +#endif
> +} else if (mem_shared) {
> +#ifdef CONFIG_POSIX
> +Error *err = NULL;
> +memory_region_init_ram_from_file(mr, owner, NULL, ram_size, 0,
> + RAM_SHARED, NULL, );
> +if (err) {
> +error_report_err(err);
> +exit(1);
> +}
> +#else
> +fprintf(stderr, "-mem-shared not supported on this host\n");
> +exit(1);
> #endif
> } else {
> memory_region_init_ram_nomigrate(mr, owner, name, ram_size,
> _fatal);
I'd really like make allocate_system_memory_nonnuma() just create
a memory backend object. This way non-NUMA and NUMA
configuration would be able to use exactly the same set of
options.
I have the impression we tried to do this in the past. Igor, do
you remember if we did?
--
Eduardo
