Message-ID: <063c51c3-c276-3b68-f130-db0751b35d42@linuxfoundation.org>
Date:   Fri, 27 Jan 2023 12:18:01 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] docs: add workload-tracing document to admin-guide
Content-Language: en-US
To:     Jonathan Corbet <corbet@lwn.net>
Cc:     sshefali021@gmail.com, kstewart@linuxfoundation.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20230127165440.19158-1-skhan@linuxfoundation.org>
 <87bkmj99yv.fsf@meer.lwn.net>
From:   Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <87bkmj99yv.fsf@meer.lwn.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/27/23 10:13, Jonathan Corbet wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> Add a new section to the admin-guide with information of interest to
>> application developers and system integrators doing analysis of the
>> Linux kernel for safety critical applications.
>>
>> This section will contain documents supporting analysis of kernel
>> interactions with applications, and key kernel subsystems expectations.
>>
>> Add a new workload-tracing document to this new section.
>>
>> Signed-off-by: Shefali Sharma <sshefali021@gmail.com>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   Documentation/admin-guide/index.rst           |  11 +
>>   .../admin-guide/workload-tracing.rst          | 486 ++++++++++++++++++
>>   2 files changed, 497 insertions(+)
>>   create mode 100644 Documentation/admin-guide/workload-tracing.rst
> 
> This seems like generally good stuff, thanks.  I have a few nits...

Thanks for the review.

> 
>> +Key Points
>> +==========
>> +
>> + * Understanding system resources necessary to build and run a workload
>> +   is important.
>> + * Linux tracing and strace can be used to discover the system resources
>> +   in use by a workload. The completeness of the system usage information
>> +   depends on the completeness of coverage of a workload.
>> + * Performance and security of the operating system can be analyzed with
>> +   the help of tools such as `perf <https://man7.org/linux/man-pages/man1/perf.1.html>`_, `stress-ng <https://www.mankier.com/1/stress-ng>`_, `paxtest <https://github.com/opntr/paxtest-freebsd/blob/hardenedbsd/0.9.14-hbsd/README>`_.
>> + * Once we discover and understand the workload needs, we can focus on them
>> +   to avoid regressions and use it to evaluate safety considerations.
>> +
>> +Methodology
>> +===========
>> +
>> +`strace <https://man7.org/linux/man-pages/man1/strace.1.html>`_ is a diagnostic, instructional, and debugging tool and can be used to discover the system resources in use by a workload. Once we discover and understand the workload needs, we can focus on them to avoid regressions and use it to evaluate safety considerations. We chose strace tool to trace workloads.
> 
> I know that the 80-column limit is controversial to some; I happen to
> believe that text should certainly adhere to it for basic readability
> reasons.  But can we all agree that nearly 400 columns is too many? :)
> Please line-break the whole document properly.

Yeah that is my bad. I noticed it once the patch was in my Inbox.
I will fix them all up.

> 
>> +Install tools to build Linux kernel and tools in kernel repository. scripts/ver_linux is a good way to check if your system already has the necessary tools.
>> +::
>> +
>> +  sudo apt-get build-essentials flex bison yacc
>> +  sudo apt install libelf-dev systemtap-sdt-dev libaudit-dev libslang2-dev libperl-dev libdw-dev
>> +
>> +Browsing kernel sources
>> +::
>> +
>> +  sudo apt-get install cscope
>> +
>> +Install stress-ng and paxtest
>> +::
>> +
>> +  apt-get install stress-ng
>> +  apt-get install paxtest
> 
> For literal blocks, the "::" can go at the end of the previous line,
> which makes the document a bit more readable.

Makes sense. Will fix it.

> 
> [...]
> 
>> +SPDX-License-Identifier: CC-BY-4.0
>> +==================================
>> +This document is released under the Creative Commons Attribution 4.0 International License, available at https://creativecommons.org/licenses/by/4.0/legalcode. Pursuant to Section 5 of the license, please note that the following disclaimers apply (capitalized terms have the meanings set forth in the license). To the extent possible, the Licensor offers the Licensed Material as-is and as-available, and makes no representations or warranties of any kind concerning the Licensed Material, whether express, implied, statutory, or other. This includes, without limitation, warranties of title, merchantability, fitness for a particular purpose, non-infringement, absence of latent or other defects, accuracy, or the presence or absence of errors, whether or not known or discoverable. Where disclaimers of warranties are not allowed in full or in part, this disclaimer may not apply to You.
>> +
>> +To the extent possible, in no event will the Licensor be liable to You on any legal theory (including, without limitation, negligence) or otherwise for any direct, special, indirect, incidental, consequential, punitive, exemplary, or other losses, costs, expenses, or damages arising out of this Public License or use of the Licensed Material, even if the Licensor has been advised of the possibility of such losses, costs, expenses, or damages. Where a limitation of liability is not allowed in full or in part, this limitation may not apply to You.
>> +
>> +The disclaimer of warranties and limitation of liability provided above shall be interpreted in a manner that, to the extent possible, most closely approximates an absolute disclaimer and waiver of all liability.
>> +
>> +Note: Only the content of this RST file as found in the Linux kernel sources
>> +is available under CC-BY-4.0, as versions of this text that were processed
>> +(for example by the kernel's build system) might contain content taken from
>> +files which use a more restrictive license.
> 
> This is just the kind of license boilerplate that we have been trying
> hard to get rid of.  *Dual*-licensing with CC4 is OK, and the SPDX line
> at the top properly reflects that; this additional text shouldn't be here.
> 

Okay - I can get rid of this.

>> +..
>> +   end-of-content
> 
> ...and that seems a bit redundant somehow...:)

Yup. Correct.

thanks,
-- Shuah