From: Greg KH <gregkh@linuxfoundation.org>
To: Paolo Bonzini <bonzini@gnu.org>
Cc: Jing Zhang <jingzhangos@google.com>, KVM <kvm@vger.kernel.org>,
KVMARM <kvmarm@lists.cs.columbia.edu>,
LinuxMIPS <linux-mips@vger.kernel.org>,
KVMPPC <kvm-ppc@vger.kernel.org>,
LinuxS390 <linux-s390@vger.kernel.org>,
Linuxkselftest <linux-kselftest@vger.kernel.org>,
Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
Julien Thierry <julien.thierry.kdev@gmail.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Will Deacon <will@kernel.org>,
Huacai Chen <chenhuacai@kernel.org>,
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Paul Mackerras <paulus@ozlabs.org>,
Christian Borntraeger <borntraeger@de.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
David Hildenbrand <david@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Sean Christopherson <seanjc@google.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Jim Mattson <jmattson@google.com>,
Peter Shier <pshier@google.com>, Oliver Upton <oupton@google.com>,
David Rientjes <rientjes@google.com>,
Emanuele Giuseppe Esposito <eesposit@redhat.com>,
David Matlack <dmatlack@google.com>,
Ricardo Koller <ricarkol@google.com>,
Krish Sadhukhan <krish.sadhukhan@oracle.com>,
Fuad Tabba <tabba@google.com>
Subject: Re: [PATCH v11 2/7] KVM: stats: Add fd-based API to read binary stats data
Date: Fri, 18 Jun 2021 10:23:39 +0200 [thread overview]
Message-ID: <YMxYC8syYRBhbBAq@kroah.com> (raw)
In-Reply-To: <f2616b8e-0cf8-570f-4bd3-7ef5cbcb37b0@gnu.org>
On Fri, Jun 18, 2021 at 10:02:57AM +0200, Paolo Bonzini wrote:
> On 18/06/21 09:00, Greg KH wrote:
> > > +struct kvm_stats_header {
> > > + __u32 name_size;
> > > + __u32 count;
> > > + __u32 desc_offset;
> > > + __u32 data_offset;
> > > + char id[];
> > > +};
> >
> > You mentioned before that the size of this really is the size of the
> > structure + KVM_STATS_ID_MAXLEN, right? Or is it - KVM_STATS_ID_MAXLEN?
> >
> > If so, why not put that value explicitly in:
> > char id[THE_REST_OF_THE_HEADER_SPACE];
> >
> > As this is not a variable header size at all, and you can not change it
> > going forward, so the variable length array here feels disingenuous.
>
> It can change; the header goes up to desc_offset. Let's rename desc_offset
> to header_size.
"Traditionally" the first field of a variable length structure like this
has the size. So maybe this needs to be:
struct kvm_stats_header {
__u32 header_size;
__u32 name_size;
__u32 num_desc;
__u32 desc_offset;
__u32 data_offset;
char id[];
};
I just guessed at what "count" is as I do not remember at the moment,
but obviously "count" wasn't descriptive :)
Wait, what is "name_size" here for?
> > > +struct kvm_stats_desc {
> > > + __u32 flags;
> > > + __s16 exponent;
> > > + __u16 size;
> > > + __u32 offset;
> > > + __u32 unused;
> > > + char name[];
> > > +};
> >
> > What is the max length of name?
>
> It's name_size in the header.
So it's specified in the _previous_ header? That feels wrong, shouldn't
this descriptor define what is in it?
I'm not trying to nit-pick here, I'm actually confused now. Structures
that contain "headers" should have in those headers at least two things:
- declare the size of themselves if they are variable length
- declare offsets to other structures
Don't put a size in this header for the size of a later structure,
that's just extra complexity that is not needed.
Think of this as a stream of bytes across the wire like a hardware
descriptor. We have loads of experience dealing with this with
protocols like USB and Greybus and PCI and the like. Let's learn from
those experiences and not try to mess things up where we don't need to
:)
>
> > Why aren't these structures defined here in kerneldoc so that we can
> > understand them better? Putting them in a .rst file guarantees they
> > will get out of sync, and you can always directly import the kerneldoc
> > into the .rst file.
>
> This is a problem in general with Documentation/virt/kvm/api.rst. The file
> is organized to match the kerneldoc structs to the ioctl that they are used
> for, and sometimes a ioctl includes different structs for each architecture.
>
> It is probably possible to do it using :identifiers: and/or :doc:, but it
> would require running scripts/kernel-doc on the uAPI headers dozens of
> times. That is quite expensive at 0.3s each run, but that's what you get
> with Perl (gcc -fsyntax-only is 20 times faster).
Is that what v4l and drm do today? That's still safer and more
"obvious" than trying to keep two different files in sync which, as I
well know, almost impossible to do well over the _years_ in which you
will have to maintain these files.
Let's make it easier for everyone, put it only in one place and if
people want to see the documentation, they can generate it (it's
auto-generated on kernel.org anyway), no need to worry about multiple
passes or not.
thanks,
greg k-h
next prev parent reply other threads:[~2021-06-18 8:23 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-18 4:48 [PATCH v11 0/7] KVM statistics data fd-based binary interface Jing Zhang
2021-06-18 4:48 ` [PATCH v11 1/7] KVM: stats: Separate generic stats from architecture specific ones Jing Zhang
2021-06-18 4:48 ` [PATCH v11 2/7] KVM: stats: Add fd-based API to read binary stats data Jing Zhang
2021-06-18 6:57 ` Greg KH
2021-06-18 8:28 ` Paolo Bonzini
2021-06-18 12:40 ` Jing Zhang
2021-06-18 7:00 ` Greg KH
2021-06-18 8:02 ` Paolo Bonzini
2021-06-18 8:23 ` Greg KH [this message]
2021-06-18 13:02 ` Jing Zhang
2021-06-18 15:51 ` Paolo Bonzini
2021-06-18 17:57 ` Jing Zhang
2021-06-18 12:53 ` Jing Zhang
2021-06-18 4:48 ` [PATCH v11 3/7] KVM: stats: Support binary stats retrieval for a VM Jing Zhang
2021-06-18 6:58 ` Greg KH
2021-06-18 12:34 ` Jing Zhang
2021-06-18 4:48 ` [PATCH v11 4/7] KVM: stats: Support binary stats retrieval for a VCPU Jing Zhang
2021-06-18 4:48 ` [PATCH v11 5/7] KVM: stats: Add documentation for binary statistics interface Jing Zhang
2021-06-18 7:02 ` Greg KH
2021-06-18 8:26 ` Paolo Bonzini
2021-06-18 8:31 ` Greg KH
2021-06-18 8:37 ` Paolo Bonzini
2021-06-18 4:48 ` [PATCH v11 6/7] KVM: selftests: Add selftest for KVM statistics data binary interface Jing Zhang
2021-06-18 4:48 ` [PATCH v11 7/7] KVM: stats: Remove code duplication for binary and debugfs stats Jing Zhang
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=YMxYC8syYRBhbBAq@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=aleksandar.qemu.devel@gmail.com \
--cc=bonzini@gnu.org \
--cc=borntraeger@de.ibm.com \
--cc=chenhuacai@kernel.org \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=dmatlack@google.com \
--cc=eesposit@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=james.morse@arm.com \
--cc=jingzhangos@google.com \
--cc=jmattson@google.com \
--cc=julien.thierry.kdev@gmail.com \
--cc=krish.sadhukhan@oracle.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oupton@google.com \
--cc=paulus@ozlabs.org \
--cc=pshier@google.com \
--cc=ricarkol@google.com \
--cc=rientjes@google.com \
--cc=seanjc@google.com \
--cc=suzuki.poulose@arm.com \
--cc=tabba@google.com \
--cc=tsbogend@alpha.franken.de \
--cc=vkuznets@redhat.com \
--cc=will@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).