[v1] qapi: clarify that the default is backend dependent

[PATCH] qapi: clarify that the default is backend dependent

Posted by Stefano Garzarella 6 months ago

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 @@
 #     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
-- 
2.45.1

Re: [PATCH] qapi: clarify that the default is backend dependent

Posted by Markus Armbruster 5 months, 3 weeks ago

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?

Re: [PATCH] qapi: clarify that the default is backend dependent

Posted by Stefano Garzarella 5 months, 3 weeks ago

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

Re: [PATCH] qapi: clarify that the default is backend dependent

Posted by Markus Armbruster 5 months, 3 weeks ago

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 :)

Re: [PATCH] qapi: clarify that the default is backend dependent

Posted by Stefano Garzarella 5 months, 3 weeks ago

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