| Message ID | 20240523133302.103858-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: clarify that the default is backend dependent | expand |
Stefano Garzarella <sgarzare@redhat.com> writes: > The default value of the @share option of the @MemoryBackendProperties > eally depends on the backend type, so let's document it explicitly and > add the default value where it was missing. > > Cc: David Hildenbrand <david@redhat.com> > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > I followed how we document @share in memfd and epc, but I don't like it > very much, I just can't think of a better way, so if you have a suggestion > I can change them in all of them. > > Thanks, > Stefano > --- > qapi/qom.json | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/qapi/qom.json b/qapi/qom.json > index 38dde6d785..8463bd32a2 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -600,7 +600,7 @@ ## # @MemoryBackendProperties: # # Properties for objects of classes derived from memory-backend. # [...] > # preallocation threads (default: none) (since 7.2) > # > # @share: if false, the memory is private to QEMU; if true, it is > -# shared (default: false) > +# shared (default depends on the backend type) Note for later: the backends are the branches of ObjectOptions that use MemoryBackendProperties as branch type or as base of their branch type. These are memory-backend-epc (uses MemoryBackendEpcProperties) memory-backend-file (uses MemoryBackendFileProperties) memory-backend-memfd (uses MemoryBackendMemfdProperties) memory-backend-ram (uses MemoryBackendProperties) > # > # @reserve: if true, reserve swap space (or huge pages) if applicable > # (default: true) (since 6.1) > @@ -639,6 +639,8 @@ > # > # Properties for memory-backend-file objects. > # > +# The @share boolean option is false by default with file. > +# > # @align: the base address alignment when QEMU mmap(2)s @mem-path. > # Some backend stores specified by @mem-path require an alignment > # different than the default one used by QEMU, e.g. the device DAX As stated in the commit message, this matches existing documentation in memory-backend-epc # The @share boolean option is true by default with epc and memory-backend-memfd # The @share boolean option is true by default with memfd. I think "with FOO" could be clearer. Perhaps something like "with backend 'memory-backend-FOO'. However, even with your patch, we're still missing memory-backend-ram. I can see two solutions: 1. Create MemoryBackendRamProperties just to have a place for documenting @share's default. 2. Document @share's default right where it's defined, roughly like this: # @share: if false, the memory is private to QEMU; if true, it is # shared (default false for backends memory-backend-file and # memory-backend-ram, true for backends memory-backend-epc and # memory-backend-memfd) CON: we need to remember to update this whenever we add another backend. PRO: generated documentation is better, in my opinion. Thoughts?
On Mon, Jun 03, 2024 at 11:34:10AM GMT, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> writes: > >> The default value of the @share option of the @MemoryBackendProperties >> eally depends on the backend type, so let's document it explicitly and >> add the default value where it was missing. >> >> Cc: David Hildenbrand <david@redhat.com> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> I followed how we document @share in memfd and epc, but I don't like it >> very much, I just can't think of a better way, so if you have a suggestion >> I can change them in all of them. >> >> Thanks, >> Stefano >> --- >> qapi/qom.json | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/qapi/qom.json b/qapi/qom.json >> index 38dde6d785..8463bd32a2 100644 >> --- a/qapi/qom.json >> +++ b/qapi/qom.json >> @@ -600,7 +600,7 @@ > ## > # @MemoryBackendProperties: > # > # Properties for objects of classes derived from memory-backend. > # > >[...] > >> # preallocation threads (default: none) (since 7.2) >> # >> # @share: if false, the memory is private to QEMU; if true, it is >> -# shared (default: false) >> +# shared (default depends on the backend type) > >Note for later: the backends are the branches of ObjectOptions that use >MemoryBackendProperties as branch type or as base of their branch type. >These are > > memory-backend-epc (uses MemoryBackendEpcProperties) > memory-backend-file (uses MemoryBackendFileProperties) > memory-backend-memfd (uses MemoryBackendMemfdProperties) > memory-backend-ram (uses MemoryBackendProperties) > >> # >> # @reserve: if true, reserve swap space (or huge pages) if applicable >> # (default: true) (since 6.1) >> @@ -639,6 +639,8 @@ >> # >> # Properties for memory-backend-file objects. >> # >> +# The @share boolean option is false by default with file. >> +# >> # @align: the base address alignment when QEMU mmap(2)s @mem-path. >> # Some backend stores specified by @mem-path require an alignment >> # different than the default one used by QEMU, e.g. the device DAX > >As stated in the commit message, this matches existing documentation in >memory-backend-epc > > # The @share boolean option is true by default with epc > >and memory-backend-memfd > > # The @share boolean option is true by default with memfd. > >I think "with FOO" could be clearer. Perhaps something like "with >backend 'memory-backend-FOO'. Ack, I'll do. > >However, even with your patch, we're still missing memory-backend-ram. >I can see two solutions: > >1. Create MemoryBackendRamProperties just to have a place for >documenting @share's default. > >2. Document @share's default right where it's defined, roughly like >this: > > # @share: if false, the memory is private to QEMU; if true, it is > # shared (default false for backends memory-backend-file and > # memory-backend-ram, true for backends memory-backend-epc and > # memory-backend-memfd) > >CON: we need to remember to update this whenever we add another backend. > >PRO: generated documentation is better, in my opinion. > >Thoughts? > Maybe option 2 is slightly better and it's also clearer how to document the default for other backends. When I added a new backend, it was not clear to me how to define the default for an inherited parameter. I would go with 2 if you agree. Thanks, Stefano
Stefano Garzarella <sgarzare@redhat.com> writes: > On Mon, Jun 03, 2024 at 11:34:10AM GMT, Markus Armbruster wrote: >>Stefano Garzarella <sgarzare@redhat.com> writes: >> >>> The default value of the @share option of the @MemoryBackendProperties >>> eally depends on the backend type, so let's document it explicitly and >>> add the default value where it was missing. >>> >>> Cc: David Hildenbrand <david@redhat.com> >>> Suggested-by: Markus Armbruster <armbru@redhat.com> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> --- >>> I followed how we document @share in memfd and epc, but I don't like it >>> very much, I just can't think of a better way, so if you have a suggestion >>> I can change them in all of them. >>> >>> Thanks, >>> Stefano >>> --- >>> qapi/qom.json | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/qapi/qom.json b/qapi/qom.json >>> index 38dde6d785..8463bd32a2 100644 >>> --- a/qapi/qom.json >>> +++ b/qapi/qom.json >>> @@ -600,7 +600,7 @@ >> ## >> # @MemoryBackendProperties: >> # >> # Properties for objects of classes derived from memory-backend. >> # >> >>[...] >> >>> # preallocation threads (default: none) (since 7.2) >>> # >>> # @share: if false, the memory is private to QEMU; if true, it is >>> -# shared (default: false) >>> +# shared (default depends on the backend type) >> >>Note for later: the backends are the branches of ObjectOptions that use >>MemoryBackendProperties as branch type or as base of their branch type. >>These are >> >> memory-backend-epc (uses MemoryBackendEpcProperties) >> memory-backend-file (uses MemoryBackendFileProperties) >> memory-backend-memfd (uses MemoryBackendMemfdProperties) >> memory-backend-ram (uses MemoryBackendProperties) >> >>> # >>> # @reserve: if true, reserve swap space (or huge pages) if applicable >>> # (default: true) (since 6.1) >>> @@ -639,6 +639,8 @@ >>> # >>> # Properties for memory-backend-file objects. >>> # >>> +# The @share boolean option is false by default with file. >>> +# >>> # @align: the base address alignment when QEMU mmap(2)s @mem-path. >>> # Some backend stores specified by @mem-path require an alignment >>> # different than the default one used by QEMU, e.g. the device DAX >> >>As stated in the commit message, this matches existing documentation in >>memory-backend-epc >> >> # The @share boolean option is true by default with epc >> >>and memory-backend-memfd >> >> # The @share boolean option is true by default with memfd. >> >>I think "with FOO" could be clearer. Perhaps something like "with >>backend 'memory-backend-FOO'. > > Ack, I'll do. > >> >>However, even with your patch, we're still missing memory-backend-ram. >>I can see two solutions: >> >>1. Create MemoryBackendRamProperties just to have a place for >>documenting @share's default. >> >>2. Document @share's default right where it's defined, roughly like >>this: >> >> # @share: if false, the memory is private to QEMU; if true, it is >> # shared (default false for backends memory-backend-file and >> # memory-backend-ram, true for backends memory-backend-epc and >> # memory-backend-memfd) >> >>CON: we need to remember to update this whenever we add another backend. >> >>PRO: generated documentation is better, in my opinion. >> >>Thoughts? >> > > Maybe option 2 is slightly better and it's also clearer how to document the default for other backends. > > When I added a new backend, it was not clear to me how to define the default for an inherited parameter. > > I would go with 2 if you agree. I actually like 2 better :)
On Tue, Jun 04, 2024 at 04:58:49PM GMT, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> writes: > >> On Mon, Jun 03, 2024 at 11:34:10AM GMT, Markus Armbruster wrote: >>>Stefano Garzarella <sgarzare@redhat.com> writes: >>> >>>> The default value of the @share option of the @MemoryBackendProperties >>>> eally depends on the backend type, so let's document it explicitly and >>>> add the default value where it was missing. >>>> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Suggested-by: Markus Armbruster <armbru@redhat.com> >>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>>> --- >>>> I followed how we document @share in memfd and epc, but I don't like it >>>> very much, I just can't think of a better way, so if you have a suggestion >>>> I can change them in all of them. >>>> >>>> Thanks, >>>> Stefano >>>> --- >>>> qapi/qom.json | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/qapi/qom.json b/qapi/qom.json >>>> index 38dde6d785..8463bd32a2 100644 >>>> --- a/qapi/qom.json >>>> +++ b/qapi/qom.json >>>> @@ -600,7 +600,7 @@ >>> ## >>> # @MemoryBackendProperties: >>> # >>> # Properties for objects of classes derived from memory-backend. >>> # >>> >>>[...] >>> >>>> # preallocation threads (default: none) (since 7.2) >>>> # >>>> # @share: if false, the memory is private to QEMU; if true, it is >>>> -# shared (default: false) >>>> +# shared (default depends on the backend type) >>> >>>Note for later: the backends are the branches of ObjectOptions that use >>>MemoryBackendProperties as branch type or as base of their branch type. >>>These are >>> >>> memory-backend-epc (uses MemoryBackendEpcProperties) >>> memory-backend-file (uses MemoryBackendFileProperties) >>> memory-backend-memfd (uses MemoryBackendMemfdProperties) >>> memory-backend-ram (uses MemoryBackendProperties) >>> >>>> # >>>> # @reserve: if true, reserve swap space (or huge pages) if applicable >>>> # (default: true) (since 6.1) >>>> @@ -639,6 +639,8 @@ >>>> # >>>> # Properties for memory-backend-file objects. >>>> # >>>> +# The @share boolean option is false by default with file. >>>> +# >>>> # @align: the base address alignment when QEMU mmap(2)s @mem-path. >>>> # Some backend stores specified by @mem-path require an alignment >>>> # different than the default one used by QEMU, e.g. the device DAX >>> >>>As stated in the commit message, this matches existing documentation in >>>memory-backend-epc >>> >>> # The @share boolean option is true by default with epc >>> >>>and memory-backend-memfd >>> >>> # The @share boolean option is true by default with memfd. >>> >>>I think "with FOO" could be clearer. Perhaps something like "with >>>backend 'memory-backend-FOO'. >> >> Ack, I'll do. >> >>> >>>However, even with your patch, we're still missing memory-backend-ram. >>>I can see two solutions: >>> >>>1. Create MemoryBackendRamProperties just to have a place for >>>documenting @share's default. >>> >>>2. Document @share's default right where it's defined, roughly like >>>this: >>> >>> # @share: if false, the memory is private to QEMU; if true, it is >>> # shared (default false for backends memory-backend-file and >>> # memory-backend-ram, true for backends memory-backend-epc and >>> # memory-backend-memfd) >>> >>>CON: we need to remember to update this whenever we add another backend. >>> >>>PRO: generated documentation is better, in my opinion. >>> >>>Thoughts? >>> >> >> Maybe option 2 is slightly better and it's also clearer how to document the default for other backends. >> >> When I added a new backend, it was not clear to me how to define the default for an inherited parameter. >> >> I would go with 2 if you agree. > >I actually like 2 better :) > Yeah, I'll do it ;-) Thanks, Stefano
diff --git a/qapi/qom.json b/qapi/qom.json index 38dde6d785..8463bd32a2 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -600,7 +600,7 @@ # preallocation threads (default: none) (since 7.2) # # @share: if false, the memory is private to QEMU; if true, it is -# shared (default: false) +# shared (default depends on the backend type) # # @reserve: if true, reserve swap space (or huge pages) if applicable # (default: true) (since 6.1) @@ -639,6 +639,8 @@ # # Properties for memory-backend-file objects. # +# The @share boolean option is false by default with file. +# # @align: the base address alignment when QEMU mmap(2)s @mem-path. # Some backend stores specified by @mem-path require an alignment # different than the default one used by QEMU, e.g. the device DAX
The default value of the @share option of the @MemoryBackendProperties eally depends on the backend type, so let's document it explicitly and add the default value where it was missing. Cc: David Hildenbrand <david@redhat.com> Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- I followed how we document @share in memfd and epc, but I don't like it very much, I just can't think of a better way, so if you have a suggestion I can change them in all of them. Thanks, Stefano --- qapi/qom.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)