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=-2.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 087C5C433E1 for ; Thu, 4 Jun 2020 15:34:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D579F207DF for ; Thu, 4 Jun 2020 15:34:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="vZt0DMCf" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729587AbgFDPeo (ORCPT ); Thu, 4 Jun 2020 11:34:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35368 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729035AbgFDPem (ORCPT ); Thu, 4 Jun 2020 11:34:42 -0400 Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1F96EC08C5C0; Thu, 4 Jun 2020 08:34:42 -0700 (PDT) Received: by mail-wr1-x442.google.com with SMTP id p5so6586897wrw.9; Thu, 04 Jun 2020 08:34:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=vZt0DMCfoJUEXzeTrBICKPyqGfuBe7vx1mxfTRfe7AMZfEO9MgZiR1mPQATjGg7sos L2gEppsjZRToGPjruQDj6PgcZ9k1LIUGD0iWIyOtnINMSac6a+7rCj3z4QC4lrZCdKFE k2yM8OXoWQeV2IMPxu2sJJA6xUKeMYmYvndml0HnOewX3ksq8F1r9goi9s79mQpwsSIb c5mAXaNTKzUmdLKel6x7irCbEI4DTv2oXtv/qN7Oxn1zzp0W/r8QazRbJWdm+blAf/6N mqCzGRjWNAgSpkjxibd5bm0M9tIZAW+bPd5JKLWcPAMtTD2J+9FPsJ68L+Qr8Bw98u2/ uiog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=JZ8eG8tE4Lrj45800Q9lSdVl1iphQ/O0A/WBiZSX/qfUZrp9kuz9mx7Aas4wk9Cz2t YOyhEGAOox3UiNgIhxGiEAdmW7/I6HpNeqd3ac3pf7rNg0bQSMSYM2OoJx6UDo0UrMhT IODk77ghGzgKxAftTjwQDWJZcTMSBxjDQtlOKmdYnIHBeNr7DuFOpkeRD5sNTnD7QDMW c66hlInF0eD4sknRVCkMOquRbw11MXQ6XFqsX+FDPbfLLc9Ix6P21O0X9JkCpZuCNoQ8 cMzt5mI8yG+KHkq896a/kZcL3/uZ/Pf1OayiCs2sbzNkK7lddWbrIhuV6t1jFQ1bFWE7 GH+w== X-Gm-Message-State: AOAM531sL81mAp0DcxfV/kkabS5Uu5LKAFtK6gzhHQqXxIpKx19EmvSx J6WaYub0DBy0Omn3nc9kPcKtaZmjiMY= X-Google-Smtp-Source: ABdhPJxfqtpJqbRTF++Qa4Ke2KxsNtWe0xpkNWVokflegjt7JKKLmL/tz6wJQHjKWgmRMZK5uAnt/Q== X-Received: by 2002:adf:f0d2:: with SMTP id x18mr4926753wro.250.1591284880431; Thu, 04 Jun 2020 08:34:40 -0700 (PDT) Received: from [192.168.8.102] ([194.230.155.251]) by smtp.gmail.com with ESMTPSA id q1sm7431317wmc.12.2020.06.04.08.34.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Jun 2020 08:34:39 -0700 (PDT) Subject: Re: [PATCH v3 2/7] documentation for stats_fs To: Randy Dunlap , Emanuele Giuseppe Esposito , kvm@vger.kernel.org Cc: Christian Borntraeger , Paolo Bonzini , Jim Mattson , Alexander Viro , David Rientjes , Jonathan Adams , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org References: <20200526110318.69006-1-eesposit@redhat.com> <20200526110318.69006-3-eesposit@redhat.com> From: Emanuele Giuseppe Esposito Message-ID: Date: Thu, 4 Jun 2020 17:34:37 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, >> + >> +The STATS_FS_HIDDEN attribute won't affect the aggregation, it will only >> +block the creation of the files. > > Why does HIDDEN block the creation of files? instead of their visibility? The file itself is used to allow the user to view the content of a value. In order to make it hidden, the framework just doesn't create the file. The structure is still present and considered in statsfs, however. Hidden in this case means not visible at all thus not created, not the hidden file concept of dotted files (".filename") > >> + >> +Add values to parent and child (also here order doesn't matter):: >> + >> + struct kvm *base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source, kvm_stats, base_ptr, 0); >> + stats_fs_source_add_values(parent_source, kvm_stats, NULL, STATS_FS_HIDDEN); >> + >> +``child_source`` will be a simple value, since it has a non-NULL base >> +pointer, while ``parent_source`` will be an aggregate. During the adding >> +phase, also values can optionally be marked as hidden, so that the folder >> +and other values can be still shown. >> + >> +Of course the same ``struct stats_fs_value`` array can be also passed with a >> +different base pointer, to represent the same value but in another instance >> +of the kvm struct. >> + >> +Search: >> + >> +Fetch a value from the child source, returning the value >> +pointed by ``(uint64_t *) base_ptr + kvm_stats[0].offset``:: >> + >> + uint64_t ret_child, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + >> +Fetch an aggregate value, searching all subsources of ``parent_source`` for >> +the specified ``struct stats_fs_value``:: >> + >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + >> + assert(ret_child == ret_parent); // check expected result >> + >> +To make it more interesting, add another child:: >> + >> + struct stats_fs_source child_source2 = stats_fs_source_create(0, "child2"); >> + >> + stats_fs_source_add_subordinate(parent_source, child_source2); >> + // now the structure is parent -> child1 >> + // -> child2 > > Is that the same as parent -> child1 -> child2 > ? It could almost be read as > parent -> child1 > parent -> child2 No the example in the documentation shows the relationship parent -> child1 and parent -> child2. It's not the same as parent -> child1 -> child2. In order to do the latter, one would need to do: stats_fs_source_add_subordinate(parent_source, child_source1); stats_fs_source_add_subordinate(child_source1, child_source2); Hope that this clarifies it. > > Whichever it is, can you make it more explicit, please? > > >> + >> + struct kvm *other_base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source2, kvm_stats, other_base_ptr, 0); >> + >> +Note that other_base_ptr points to another instance of kvm, so the struct >> +stats_fs_value is the same but the address at which they point is not. >> + >> +Now get the aggregate value:: >> + >> + uint64_t ret_child, ret_child2, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + stats_fs_source_get_value(child_source2, &kvm_stats[0], &ret_child2); >> + >> + assert((ret_child + ret_child2) == ret_parent); >> + >> +Cleanup:: >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source); >> + stats_fs_source_revoke(child_source); >> + stats_fs_source_put(child_source); >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source2); >> + stats_fs_source_revoke(child_source2); >> + stats_fs_source_put(child_source2); >> + >> + stats_fs_source_put(parent_source); >> + kfree(other_base_ptr); >> + kfree(base_ptr); >> + >> +Calling stats_fs_source_revoke is very important, because it will ensure > > stats_fs_source_revoke() > >> +that stats_fs will not access the data that were passed to >> +stats_fs_source_add_value for this source. >> + >> +Because open files increase the reference count for a stats_fs_source, the >> +source can end up living longer than the data that provides the values for >> +the source. Calling stats_fs_source_revoke just before the backing data > > stats_fs_source_revoke() > >> +is freed avoids accesses to freed data structures. The sources will return >> +0. >> + >> +This is not needed for the parent_source, since it just contains >> +aggregates that would be 0 anyways if no matching child value exist. >> + >> +API Documentation >> +================= >> + >> +.. kernel-doc:: include/linux/stats_fs.h >> + :export: fs/stats_fs/*.c >> \ No newline at end of file > > Please fix that. ^^^^^ > > > Thanks for the documentation. > Thank you for the feedback, Emanuele 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=-1.8 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 5E270C433E0 for ; Thu, 4 Jun 2020 21:03:05 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E92E4206C3 for ; Thu, 4 Jun 2020 21:03:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="vZt0DMCf" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E92E4206C3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 49dJCC2V24zDqcN for ; Fri, 5 Jun 2020 07:03:03 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=2a00:1450:4864:20::444; helo=mail-wr1-x444.google.com; envelope-from=e.emanuelegiuseppe@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=vZt0DMCf; dkim-atps=neutral Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 49d8wT04x0zDqfy for ; Fri, 5 Jun 2020 01:34:44 +1000 (AEST) Received: by mail-wr1-x444.google.com with SMTP id x13so6612547wrv.4 for ; Thu, 04 Jun 2020 08:34:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=vZt0DMCfoJUEXzeTrBICKPyqGfuBe7vx1mxfTRfe7AMZfEO9MgZiR1mPQATjGg7sos L2gEppsjZRToGPjruQDj6PgcZ9k1LIUGD0iWIyOtnINMSac6a+7rCj3z4QC4lrZCdKFE k2yM8OXoWQeV2IMPxu2sJJA6xUKeMYmYvndml0HnOewX3ksq8F1r9goi9s79mQpwsSIb c5mAXaNTKzUmdLKel6x7irCbEI4DTv2oXtv/qN7Oxn1zzp0W/r8QazRbJWdm+blAf/6N mqCzGRjWNAgSpkjxibd5bm0M9tIZAW+bPd5JKLWcPAMtTD2J+9FPsJ68L+Qr8Bw98u2/ uiog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=RUujY/CbTl3znCUeeQmYsYO+1Qj4YuIEps5jdulrXHfT+O8Mu+n1UWm3+pQXPAQ1uV zpmXT/5x2+idpWDYA/IDIBDbO+Yq+SQLA/LvzUR+eGnN9UgAjXIsfesZflcpmIZUqe7B xTsr1Q3W7qe4PHVq4C6CyhcwEN8X2AVVhghbKUAjpnm+YHWhn0RPNPHD1SWBUzOiozy6 E/tSWWGGuygEkGQMNqTRaC64LG8SpvEZhcADNmzdPfBNSjqm3ZPMNj7wufG5q9UN///q i3lJ/Rf46TIScgv7ZlLk2ils49Lfc0F8J7LAeySlkNbPfQStSdqK1SPlSOVO3Jm5xpsN VjOQ== X-Gm-Message-State: AOAM5316Ti29c0OJRfl7OLTuB5C+MD/V/IhlsGa9NluDh2IX09rnkAsJ s3JmBHsD3GCiuRld0Xek+o8= X-Google-Smtp-Source: ABdhPJxfqtpJqbRTF++Qa4Ke2KxsNtWe0xpkNWVokflegjt7JKKLmL/tz6wJQHjKWgmRMZK5uAnt/Q== X-Received: by 2002:adf:f0d2:: with SMTP id x18mr4926753wro.250.1591284880431; Thu, 04 Jun 2020 08:34:40 -0700 (PDT) Received: from [192.168.8.102] ([194.230.155.251]) by smtp.gmail.com with ESMTPSA id q1sm7431317wmc.12.2020.06.04.08.34.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Jun 2020 08:34:39 -0700 (PDT) Subject: Re: [PATCH v3 2/7] documentation for stats_fs To: Randy Dunlap , Emanuele Giuseppe Esposito , kvm@vger.kernel.org References: <20200526110318.69006-1-eesposit@redhat.com> <20200526110318.69006-3-eesposit@redhat.com> From: Emanuele Giuseppe Esposito Message-ID: Date: Thu, 4 Jun 2020 17:34:37 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Fri, 05 Jun 2020 07:01:19 +1000 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-s390@vger.kernel.org, linux-doc@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, Jonathan Adams , Christian Borntraeger , Alexander Viro , David Rientjes , linux-fsdevel@vger.kernel.org, Paolo Bonzini , linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, Jim Mattson Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" Hi, >> + >> +The STATS_FS_HIDDEN attribute won't affect the aggregation, it will only >> +block the creation of the files. > > Why does HIDDEN block the creation of files? instead of their visibility? The file itself is used to allow the user to view the content of a value. In order to make it hidden, the framework just doesn't create the file. The structure is still present and considered in statsfs, however. Hidden in this case means not visible at all thus not created, not the hidden file concept of dotted files (".filename") > >> + >> +Add values to parent and child (also here order doesn't matter):: >> + >> + struct kvm *base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source, kvm_stats, base_ptr, 0); >> + stats_fs_source_add_values(parent_source, kvm_stats, NULL, STATS_FS_HIDDEN); >> + >> +``child_source`` will be a simple value, since it has a non-NULL base >> +pointer, while ``parent_source`` will be an aggregate. During the adding >> +phase, also values can optionally be marked as hidden, so that the folder >> +and other values can be still shown. >> + >> +Of course the same ``struct stats_fs_value`` array can be also passed with a >> +different base pointer, to represent the same value but in another instance >> +of the kvm struct. >> + >> +Search: >> + >> +Fetch a value from the child source, returning the value >> +pointed by ``(uint64_t *) base_ptr + kvm_stats[0].offset``:: >> + >> + uint64_t ret_child, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + >> +Fetch an aggregate value, searching all subsources of ``parent_source`` for >> +the specified ``struct stats_fs_value``:: >> + >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + >> + assert(ret_child == ret_parent); // check expected result >> + >> +To make it more interesting, add another child:: >> + >> + struct stats_fs_source child_source2 = stats_fs_source_create(0, "child2"); >> + >> + stats_fs_source_add_subordinate(parent_source, child_source2); >> + // now the structure is parent -> child1 >> + // -> child2 > > Is that the same as parent -> child1 -> child2 > ? It could almost be read as > parent -> child1 > parent -> child2 No the example in the documentation shows the relationship parent -> child1 and parent -> child2. It's not the same as parent -> child1 -> child2. In order to do the latter, one would need to do: stats_fs_source_add_subordinate(parent_source, child_source1); stats_fs_source_add_subordinate(child_source1, child_source2); Hope that this clarifies it. > > Whichever it is, can you make it more explicit, please? > > >> + >> + struct kvm *other_base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source2, kvm_stats, other_base_ptr, 0); >> + >> +Note that other_base_ptr points to another instance of kvm, so the struct >> +stats_fs_value is the same but the address at which they point is not. >> + >> +Now get the aggregate value:: >> + >> + uint64_t ret_child, ret_child2, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + stats_fs_source_get_value(child_source2, &kvm_stats[0], &ret_child2); >> + >> + assert((ret_child + ret_child2) == ret_parent); >> + >> +Cleanup:: >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source); >> + stats_fs_source_revoke(child_source); >> + stats_fs_source_put(child_source); >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source2); >> + stats_fs_source_revoke(child_source2); >> + stats_fs_source_put(child_source2); >> + >> + stats_fs_source_put(parent_source); >> + kfree(other_base_ptr); >> + kfree(base_ptr); >> + >> +Calling stats_fs_source_revoke is very important, because it will ensure > > stats_fs_source_revoke() > >> +that stats_fs will not access the data that were passed to >> +stats_fs_source_add_value for this source. >> + >> +Because open files increase the reference count for a stats_fs_source, the >> +source can end up living longer than the data that provides the values for >> +the source. Calling stats_fs_source_revoke just before the backing data > > stats_fs_source_revoke() > >> +is freed avoids accesses to freed data structures. The sources will return >> +0. >> + >> +This is not needed for the parent_source, since it just contains >> +aggregates that would be 0 anyways if no matching child value exist. >> + >> +API Documentation >> +================= >> + >> +.. kernel-doc:: include/linux/stats_fs.h >> + :export: fs/stats_fs/*.c >> \ No newline at end of file > > Please fix that. ^^^^^ > > > Thanks for the documentation. > Thank you for the feedback, Emanuele 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=-2.0 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 A9971C433E0 for ; Thu, 4 Jun 2020 15:34:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7F2C3207DF for ; Thu, 4 Jun 2020 15:34:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="VP8uwZiA"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="vZt0DMCf" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7F2C3207DF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=KimCM+7cMS8z6j72yfES0uA5wbc1DMlApVQF0g0/sJQ=; b=VP8uwZiAnem2xavLwb3fk8ELf RonKNrhU5bzSG5WCuWsRv0r1/97RGrhCtLyoEUYrPg0jPVG8zbd0nPw+zKc7rkcvVv6KVG7NBPPjK CAdCwoQTO/YBo51KQVZxB5XwyD1cBHTQEVB05/hrVINXZVU1Gwy4Y8BvKREn1JNmQTIx5ocyb65ng rg5wwCUXsGnlJzNaryrf5dh8ysb0ep70iFdthvf9R2EwfmI6dYm0pJhugtJD9nP7kkC8rdfqWMdGz PoGUjrt6lwJ+BPbh0PqTfC1rxnAtLEAUgvjf3v/XvOF70Gq85Rzj1NsTyBi12XGSiYDpmD/kkqLNW q58bCcPeA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgrtG-0008V8-5n; Thu, 04 Jun 2020 15:34:46 +0000 Received: from mail-wr1-x444.google.com ([2a00:1450:4864:20::444]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgrtC-0008UF-SI for linux-arm-kernel@lists.infradead.org; Thu, 04 Jun 2020 15:34:44 +0000 Received: by mail-wr1-x444.google.com with SMTP id y17so6584901wrn.11 for ; Thu, 04 Jun 2020 08:34:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=vZt0DMCfoJUEXzeTrBICKPyqGfuBe7vx1mxfTRfe7AMZfEO9MgZiR1mPQATjGg7sos L2gEppsjZRToGPjruQDj6PgcZ9k1LIUGD0iWIyOtnINMSac6a+7rCj3z4QC4lrZCdKFE k2yM8OXoWQeV2IMPxu2sJJA6xUKeMYmYvndml0HnOewX3ksq8F1r9goi9s79mQpwsSIb c5mAXaNTKzUmdLKel6x7irCbEI4DTv2oXtv/qN7Oxn1zzp0W/r8QazRbJWdm+blAf/6N mqCzGRjWNAgSpkjxibd5bm0M9tIZAW+bPd5JKLWcPAMtTD2J+9FPsJ68L+Qr8Bw98u2/ uiog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=D3By0h98qnolfLM0+QRpl2dqCpgdvgnNY+OucjDnKDY=; b=cAzSC4BX6xTBQa9TUj/tOwcYrAJJqI9ml1IGVgemy9NCt5z6NE2Lc3sl669ZynOR8r 2c6lAWlVvl+s6CjBSn+WuGWz00hQXO+4HFgCwIuJP3U8hNcSWEXFn1rTKsnuxemt/QV5 ZSFbCF/J1PdIvT0Gh/d8TG5MuVWT3CvV2O46ymscIic3LZIOAf/4gndiuUbBvIr+Fxjt tN5t7ZezdDk4wUFCAShlAuKIy0zCPyc1CuyByNGKFgFPkArNoS5lqZRmfGb/SSZDwRk8 C9tngdDPUEaoVmUBAWRAXB3FitYwaSm6kS22+QI2HGRLRnRkhsqTz/PKN0LEXU4Pb6fL LJGg== X-Gm-Message-State: AOAM532jNTgjo13RImPyP7/cVsJiqeoidGp6ALwdVvg5OBdFMogHj6+f qVlIbtG663BDrOxfLxTlBCy2588+kY4= X-Google-Smtp-Source: ABdhPJxfqtpJqbRTF++Qa4Ke2KxsNtWe0xpkNWVokflegjt7JKKLmL/tz6wJQHjKWgmRMZK5uAnt/Q== X-Received: by 2002:adf:f0d2:: with SMTP id x18mr4926753wro.250.1591284880431; Thu, 04 Jun 2020 08:34:40 -0700 (PDT) Received: from [192.168.8.102] ([194.230.155.251]) by smtp.gmail.com with ESMTPSA id q1sm7431317wmc.12.2020.06.04.08.34.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Jun 2020 08:34:39 -0700 (PDT) Subject: Re: [PATCH v3 2/7] documentation for stats_fs To: Randy Dunlap , Emanuele Giuseppe Esposito , kvm@vger.kernel.org References: <20200526110318.69006-1-eesposit@redhat.com> <20200526110318.69006-3-eesposit@redhat.com> From: Emanuele Giuseppe Esposito Message-ID: Date: Thu, 4 Jun 2020 17:34:37 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200604_083442_916225_75B0F66F X-CRM114-Status: GOOD ( 22.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-s390@vger.kernel.org, linux-doc@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, Jonathan Adams , Christian Borntraeger , Alexander Viro , David Rientjes , linux-fsdevel@vger.kernel.org, Paolo Bonzini , linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, Jim Mattson Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, >> + >> +The STATS_FS_HIDDEN attribute won't affect the aggregation, it will only >> +block the creation of the files. > > Why does HIDDEN block the creation of files? instead of their visibility? The file itself is used to allow the user to view the content of a value. In order to make it hidden, the framework just doesn't create the file. The structure is still present and considered in statsfs, however. Hidden in this case means not visible at all thus not created, not the hidden file concept of dotted files (".filename") > >> + >> +Add values to parent and child (also here order doesn't matter):: >> + >> + struct kvm *base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source, kvm_stats, base_ptr, 0); >> + stats_fs_source_add_values(parent_source, kvm_stats, NULL, STATS_FS_HIDDEN); >> + >> +``child_source`` will be a simple value, since it has a non-NULL base >> +pointer, while ``parent_source`` will be an aggregate. During the adding >> +phase, also values can optionally be marked as hidden, so that the folder >> +and other values can be still shown. >> + >> +Of course the same ``struct stats_fs_value`` array can be also passed with a >> +different base pointer, to represent the same value but in another instance >> +of the kvm struct. >> + >> +Search: >> + >> +Fetch a value from the child source, returning the value >> +pointed by ``(uint64_t *) base_ptr + kvm_stats[0].offset``:: >> + >> + uint64_t ret_child, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + >> +Fetch an aggregate value, searching all subsources of ``parent_source`` for >> +the specified ``struct stats_fs_value``:: >> + >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + >> + assert(ret_child == ret_parent); // check expected result >> + >> +To make it more interesting, add another child:: >> + >> + struct stats_fs_source child_source2 = stats_fs_source_create(0, "child2"); >> + >> + stats_fs_source_add_subordinate(parent_source, child_source2); >> + // now the structure is parent -> child1 >> + // -> child2 > > Is that the same as parent -> child1 -> child2 > ? It could almost be read as > parent -> child1 > parent -> child2 No the example in the documentation shows the relationship parent -> child1 and parent -> child2. It's not the same as parent -> child1 -> child2. In order to do the latter, one would need to do: stats_fs_source_add_subordinate(parent_source, child_source1); stats_fs_source_add_subordinate(child_source1, child_source2); Hope that this clarifies it. > > Whichever it is, can you make it more explicit, please? > > >> + >> + struct kvm *other_base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source2, kvm_stats, other_base_ptr, 0); >> + >> +Note that other_base_ptr points to another instance of kvm, so the struct >> +stats_fs_value is the same but the address at which they point is not. >> + >> +Now get the aggregate value:: >> + >> + uint64_t ret_child, ret_child2, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + stats_fs_source_get_value(child_source2, &kvm_stats[0], &ret_child2); >> + >> + assert((ret_child + ret_child2) == ret_parent); >> + >> +Cleanup:: >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source); >> + stats_fs_source_revoke(child_source); >> + stats_fs_source_put(child_source); >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source2); >> + stats_fs_source_revoke(child_source2); >> + stats_fs_source_put(child_source2); >> + >> + stats_fs_source_put(parent_source); >> + kfree(other_base_ptr); >> + kfree(base_ptr); >> + >> +Calling stats_fs_source_revoke is very important, because it will ensure > > stats_fs_source_revoke() > >> +that stats_fs will not access the data that were passed to >> +stats_fs_source_add_value for this source. >> + >> +Because open files increase the reference count for a stats_fs_source, the >> +source can end up living longer than the data that provides the values for >> +the source. Calling stats_fs_source_revoke just before the backing data > > stats_fs_source_revoke() > >> +is freed avoids accesses to freed data structures. The sources will return >> +0. >> + >> +This is not needed for the parent_source, since it just contains >> +aggregates that would be 0 anyways if no matching child value exist. >> + >> +API Documentation >> +================= >> + >> +.. kernel-doc:: include/linux/stats_fs.h >> + :export: fs/stats_fs/*.c >> \ No newline at end of file > > Please fix that. ^^^^^ > > > Thanks for the documentation. > Thank you for the feedback, Emanuele _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Emanuele Giuseppe Esposito Date: Thu, 04 Jun 2020 15:34:37 +0000 Subject: Re: [PATCH v3 2/7] documentation for stats_fs Message-Id: List-Id: References: <20200526110318.69006-1-eesposit@redhat.com> <20200526110318.69006-3-eesposit@redhat.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Randy Dunlap , Emanuele Giuseppe Esposito , kvm@vger.kernel.org Cc: Christian Borntraeger , Paolo Bonzini , Jim Mattson , Alexander Viro , David Rientjes , Jonathan Adams , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org Hi, >> + >> +The STATS_FS_HIDDEN attribute won't affect the aggregation, it will only >> +block the creation of the files. > > Why does HIDDEN block the creation of files? instead of their visibility? The file itself is used to allow the user to view the content of a value. In order to make it hidden, the framework just doesn't create the file. The structure is still present and considered in statsfs, however. Hidden in this case means not visible at all thus not created, not the hidden file concept of dotted files (".filename") > >> + >> +Add values to parent and child (also here order doesn't matter):: >> + >> + struct kvm *base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source, kvm_stats, base_ptr, 0); >> + stats_fs_source_add_values(parent_source, kvm_stats, NULL, STATS_FS_HIDDEN); >> + >> +``child_source`` will be a simple value, since it has a non-NULL base >> +pointer, while ``parent_source`` will be an aggregate. During the adding >> +phase, also values can optionally be marked as hidden, so that the folder >> +and other values can be still shown. >> + >> +Of course the same ``struct stats_fs_value`` array can be also passed with a >> +different base pointer, to represent the same value but in another instance >> +of the kvm struct. >> + >> +Search: >> + >> +Fetch a value from the child source, returning the value >> +pointed by ``(uint64_t *) base_ptr + kvm_stats[0].offset``:: >> + >> + uint64_t ret_child, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + >> +Fetch an aggregate value, searching all subsources of ``parent_source`` for >> +the specified ``struct stats_fs_value``:: >> + >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + >> + assert(ret_child = ret_parent); // check expected result >> + >> +To make it more interesting, add another child:: >> + >> + struct stats_fs_source child_source2 = stats_fs_source_create(0, "child2"); >> + >> + stats_fs_source_add_subordinate(parent_source, child_source2); >> + // now the structure is parent -> child1 >> + // -> child2 > > Is that the same as parent -> child1 -> child2 > ? It could almost be read as > parent -> child1 > parent -> child2 No the example in the documentation shows the relationship parent -> child1 and parent -> child2. It's not the same as parent -> child1 -> child2. In order to do the latter, one would need to do: stats_fs_source_add_subordinate(parent_source, child_source1); stats_fs_source_add_subordinate(child_source1, child_source2); Hope that this clarifies it. > > Whichever it is, can you make it more explicit, please? > > >> + >> + struct kvm *other_base_ptr = kmalloc(..., sizeof(struct kvm)); >> + ... >> + stats_fs_source_add_values(child_source2, kvm_stats, other_base_ptr, 0); >> + >> +Note that other_base_ptr points to another instance of kvm, so the struct >> +stats_fs_value is the same but the address at which they point is not. >> + >> +Now get the aggregate value:: >> + >> + uint64_t ret_child, ret_child2, ret_parent; >> + >> + stats_fs_source_get_value(child_source, &kvm_stats[0], &ret_child); >> + stats_fs_source_get_value(parent_source, &kvm_stats[0], &ret_parent); >> + stats_fs_source_get_value(child_source2, &kvm_stats[0], &ret_child2); >> + >> + assert((ret_child + ret_child2) = ret_parent); >> + >> +Cleanup:: >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source); >> + stats_fs_source_revoke(child_source); >> + stats_fs_source_put(child_source); >> + >> + stats_fs_source_remove_subordinate(parent_source, child_source2); >> + stats_fs_source_revoke(child_source2); >> + stats_fs_source_put(child_source2); >> + >> + stats_fs_source_put(parent_source); >> + kfree(other_base_ptr); >> + kfree(base_ptr); >> + >> +Calling stats_fs_source_revoke is very important, because it will ensure > > stats_fs_source_revoke() > >> +that stats_fs will not access the data that were passed to >> +stats_fs_source_add_value for this source. >> + >> +Because open files increase the reference count for a stats_fs_source, the >> +source can end up living longer than the data that provides the values for >> +the source. Calling stats_fs_source_revoke just before the backing data > > stats_fs_source_revoke() > >> +is freed avoids accesses to freed data structures. The sources will return >> +0. >> + >> +This is not needed for the parent_source, since it just contains >> +aggregates that would be 0 anyways if no matching child value exist. >> + >> +API Documentation >> +========>> + >> +.. kernel-doc:: include/linux/stats_fs.h >> + :export: fs/stats_fs/*.c >> \ No newline at end of file > > Please fix that. ^^^^^ > > > Thanks for the documentation. > Thank you for the feedback, Emanuele