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 05094C2B9F4 for ; Thu, 17 Jun 2021 07:24:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DC6D8613BF for ; Thu, 17 Jun 2021 07:24:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229599AbhFQH0j (ORCPT ); Thu, 17 Jun 2021 03:26:39 -0400 Received: from mail.kernel.org ([198.145.29.99]:50350 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229580AbhFQH0i (ORCPT ); Thu, 17 Jun 2021 03:26:38 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id B5C0A61241; Thu, 17 Jun 2021 07:24:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1623914671; bh=kNxTxEwPcYv3NN2DfpZ8GnBN8QRikI5mGsAdJMShdj8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=vmjn0HbF4vDkLycAFVfFgocX33s32jQVUNaGP0J3DSVKZAgg2IaQrobYDvDawDma7 x3ikJMr5SuhJwFPLP6HEpPDeER5nEM2UPQEAeKtSe//vF6YDonr90WBlk5yNgc90ZN Ffb7TLPNWmUzQseIO/Xbs7cENsIfzLUJos/2MPo0= Date: Thu, 17 Jun 2021 09:24:28 +0200 From: Greg KH To: Jing Zhang Cc: KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Paolo Bonzini , 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 v10 2/5] KVM: stats: Add fd-based API to read binary stats data Message-ID: References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210617044146.2667540-3-jingzhangos@google.com> Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > Provides a file descriptor per VM to read VM stats info/data. > Provides a file descriptor per vCPU to read vCPU stats info/data. Shouldn't this be two separate patches, one for each thing as these are two different features being added? Anyway, an implementation question for both of these: > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > + char __user *user_buffer, size_t size, loff_t *offset) > +{ > + ssize_t copylen, len, remain = size; You are "burying" the fact that remain is initialized here, not nice, I totally missed it when reading this the first time. This should be: ssize_t copylen; ssize_t len; ssize_t remain = size; to be obvious. Remember you will be looking at this code for the next 20 years, make it easy to read. > + size_t size_header, size_desc; > + loff_t pos = *offset; > + char __user *dest = user_buffer; > + void *src; > + > + size_header = sizeof(*header); > + size_desc = header->header.count * sizeof(*desc); > + > + len = size_header + size_desc + size_stats - pos; > + len = min(len, remain); > + if (len <= 0) > + return 0; > + remain = len; > + > + /* Copy kvm stats header */ > + copylen = size_header - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)header + pos; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } I thought you said that you would not provide the header for each read, if you keep reading from the fd. It looks like you are adding it here to each read, or is there some "magic" with pos happening here that I do not understand? And if there is "magic" with pos, you should document it as it's not very obvious :) > + /* Copy kvm stats descriptors */ > + copylen = header->header.desc_offset + size_desc - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)desc + pos - header->header.desc_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + /* Copy kvm stats values */ New lines between code blocks of doing things? And again, why copy the decriptor again? or is it being skipped somehow? Ah, I think I see how it's being skipped, if I look really closely. But again, it's not obvious, and I could be wrong. Please document this REALLY well. Write code for the developer first, compiler second. Again, you are going to be maintaining it for 20+ years, think of your future self... > + copylen = header->header.data_offset + size_stats - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = stats + pos - header->header.data_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + > + *offset = pos; > + return len; > +} thanks, greg k-h 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=-3.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,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 28A5CC2B9F4 for ; Thu, 17 Jun 2021 07:24:42 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id A13056135C for ; Thu, 17 Jun 2021 07:24:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A13056135C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linuxfoundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 2AAB240870; Thu, 17 Jun 2021 03:24:41 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@linuxfoundation.org Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Acz13U+9F-DC; Thu, 17 Jun 2021 03:24:37 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 51D7449DE7; Thu, 17 Jun 2021 03:24:37 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E92AE40874 for ; Thu, 17 Jun 2021 03:24:36 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nJxE-kX20rn8 for ; Thu, 17 Jun 2021 03:24:32 -0400 (EDT) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 9B77B40870 for ; Thu, 17 Jun 2021 03:24:32 -0400 (EDT) Received: by mail.kernel.org (Postfix) with ESMTPSA id B5C0A61241; Thu, 17 Jun 2021 07:24:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1623914671; bh=kNxTxEwPcYv3NN2DfpZ8GnBN8QRikI5mGsAdJMShdj8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=vmjn0HbF4vDkLycAFVfFgocX33s32jQVUNaGP0J3DSVKZAgg2IaQrobYDvDawDma7 x3ikJMr5SuhJwFPLP6HEpPDeER5nEM2UPQEAeKtSe//vF6YDonr90WBlk5yNgc90ZN Ffb7TLPNWmUzQseIO/Xbs7cENsIfzLUJos/2MPo0= Date: Thu, 17 Jun 2021 09:24:28 +0200 From: Greg KH To: Jing Zhang Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data Message-ID: References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210617044146.2667540-3-jingzhangos@google.com> Cc: KVM , David Hildenbrand , Paul Mackerras , Linuxkselftest , Claudio Imbrenda , Will Deacon , KVMARM , Emanuele Giuseppe Esposito , LinuxS390 , Janosch Frank , Marc Zyngier , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , David Rientjes , KVMPPC , Krish Sadhukhan , David Matlack , Jim Mattson , Thomas Bogendoerfer , Sean Christopherson , Cornelia Huck , Peter Shier , LinuxMIPS , Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > Provides a file descriptor per VM to read VM stats info/data. > Provides a file descriptor per vCPU to read vCPU stats info/data. Shouldn't this be two separate patches, one for each thing as these are two different features being added? Anyway, an implementation question for both of these: > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > + char __user *user_buffer, size_t size, loff_t *offset) > +{ > + ssize_t copylen, len, remain = size; You are "burying" the fact that remain is initialized here, not nice, I totally missed it when reading this the first time. This should be: ssize_t copylen; ssize_t len; ssize_t remain = size; to be obvious. Remember you will be looking at this code for the next 20 years, make it easy to read. > + size_t size_header, size_desc; > + loff_t pos = *offset; > + char __user *dest = user_buffer; > + void *src; > + > + size_header = sizeof(*header); > + size_desc = header->header.count * sizeof(*desc); > + > + len = size_header + size_desc + size_stats - pos; > + len = min(len, remain); > + if (len <= 0) > + return 0; > + remain = len; > + > + /* Copy kvm stats header */ > + copylen = size_header - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)header + pos; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } I thought you said that you would not provide the header for each read, if you keep reading from the fd. It looks like you are adding it here to each read, or is there some "magic" with pos happening here that I do not understand? And if there is "magic" with pos, you should document it as it's not very obvious :) > + /* Copy kvm stats descriptors */ > + copylen = header->header.desc_offset + size_desc - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)desc + pos - header->header.desc_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + /* Copy kvm stats values */ New lines between code blocks of doing things? And again, why copy the decriptor again? or is it being skipped somehow? Ah, I think I see how it's being skipped, if I look really closely. But again, it's not obvious, and I could be wrong. Please document this REALLY well. Write code for the developer first, compiler second. Again, you are going to be maintaining it for 20+ years, think of your future self... > + copylen = header->header.data_offset + size_stats - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = stats + pos - header->header.data_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + > + *offset = pos; > + return len; > +} thanks, greg k-h _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg KH Date: Thu, 17 Jun 2021 07:24:28 +0000 Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data Message-Id: List-Id: References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> In-Reply-To: <20210617044146.2667540-3-jingzhangos@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jing Zhang Cc: KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Paolo Bonzini , 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 On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > Provides a file descriptor per VM to read VM stats info/data. > Provides a file descriptor per vCPU to read vCPU stats info/data. Shouldn't this be two separate patches, one for each thing as these are two different features being added? Anyway, an implementation question for both of these: > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > + char __user *user_buffer, size_t size, loff_t *offset) > +{ > + ssize_t copylen, len, remain = size; You are "burying" the fact that remain is initialized here, not nice, I totally missed it when reading this the first time. This should be: ssize_t copylen; ssize_t len; ssize_t remain = size; to be obvious. Remember you will be looking at this code for the next 20 years, make it easy to read. > + size_t size_header, size_desc; > + loff_t pos = *offset; > + char __user *dest = user_buffer; > + void *src; > + > + size_header = sizeof(*header); > + size_desc = header->header.count * sizeof(*desc); > + > + len = size_header + size_desc + size_stats - pos; > + len = min(len, remain); > + if (len <= 0) > + return 0; > + remain = len; > + > + /* Copy kvm stats header */ > + copylen = size_header - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)header + pos; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } I thought you said that you would not provide the header for each read, if you keep reading from the fd. It looks like you are adding it here to each read, or is there some "magic" with pos happening here that I do not understand? And if there is "magic" with pos, you should document it as it's not very obvious :) > + /* Copy kvm stats descriptors */ > + copylen = header->header.desc_offset + size_desc - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = (void *)desc + pos - header->header.desc_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + /* Copy kvm stats values */ New lines between code blocks of doing things? And again, why copy the decriptor again? or is it being skipped somehow? Ah, I think I see how it's being skipped, if I look really closely. But again, it's not obvious, and I could be wrong. Please document this REALLY well. Write code for the developer first, compiler second. Again, you are going to be maintaining it for 20+ years, think of your future self... > + copylen = header->header.data_offset + size_stats - pos; > + copylen = min(copylen, remain); > + if (copylen > 0) { > + src = stats + pos - header->header.data_offset; > + if (copy_to_user(dest, src, copylen)) > + return -EFAULT; > + remain -= copylen; > + pos += copylen; > + dest += copylen; > + } > + > + *offset = pos; > + return len; > +} thanks, greg k-h