From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DC4C4C49EA3 for ; Fri, 18 Jun 2021 08:23:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BBFFB6128C for ; Fri, 18 Jun 2021 08:23:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233580AbhFRIZx (ORCPT ); Fri, 18 Jun 2021 04:25:53 -0400 Received: from mail.kernel.org ([198.145.29.99]:39350 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232000AbhFRIZw (ORCPT ); Fri, 18 Jun 2021 04:25:52 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 1C11F6121D; Fri, 18 Jun 2021 08:23:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1624004622; bh=8kIa5sNlt0Pl69kW6P6wTngWvlRlxxluJS87VDmBN58=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UCkJDdBNLND5GZiz2ir5ovYNPwCGZK758duSM4PdsRVPZV0Mhoa2Lr/wFa597auAS Cg+eqS67Tefemzt2Ct2SrkgUota8ywVzoO3dhRgt/YFEZjUwVRJfaawwoZRqyFAIXF htQDGfut8cBg5MTrkT24Anjlgi3/h5hx6t3dhEGU= Date: Fri, 18 Jun 2021 10:23:39 +0200 From: Greg KH To: Paolo Bonzini Cc: Jing Zhang , KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Marc Zyngier , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito , David Matlack , Ricardo Koller , Krish Sadhukhan , Fuad Tabba Subject: Re: [PATCH v11 2/7] KVM: stats: Add fd-based API to read binary stats data Message-ID: References: <20210618044819.3690166-1-jingzhangos@google.com> <20210618044819.3690166-3-jingzhangos@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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