All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Stefano Garzarella <sgarzare@redhat.com>
Cc: qemu-devel@nongnu.org, "Eric Blake" <eblake@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH] qapi: clarify that the default is backend dependent
Date: Mon, 03 Jun 2024 11:34:10 +0200	[thread overview]
Message-ID: <877cf61ib1.fsf@pond.sub.org> (raw)
In-Reply-To: <20240523133302.103858-1-sgarzare@redhat.com> (Stefano Garzarella's message of "Thu, 23 May 2024 15:33:02 +0200")

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?



  reply	other threads:[~2024-06-03  9:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-23 13:33 [PATCH] qapi: clarify that the default is backend dependent Stefano Garzarella
2024-06-03  9:34 ` Markus Armbruster [this message]
2024-06-04 13:21   ` Stefano Garzarella
2024-06-04 14:58     ` Markus Armbruster
2024-06-06 14:06       ` Stefano Garzarella

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877cf61ib1.fsf@pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=eblake@redhat.com \
    --cc=eduardo@habkost.net \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=sgarzare@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.