Message-ID: <2436ecd5-8dc5-44c8-9a81-f7f7f00d7b70@oracle.com>
Date: Fri, 22 Dec 2023 11:36:30 +0100
From: Vegard Nossum <vegard.nossum@...cle.com>
To: Ruipeng Qi <ruipengqi7@...il.com>, corbet@....net,
        rafael.j.wysocki@...el.com, gregkh@...uxfoundation.org,
        bagasdotme@...il.com, carlos.bilbao@....com
Cc: skhan@...uxfoundation.org, srinivas.pandruvada@...ux.intel.com,
        qiruipeng@...iang.com, linux@...mhuis.info, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 3/7] doc: Add osdump guide

Hi, comments below.

On 21/12/2023 14:29, Ruipeng Qi wrote:
> +++ b/Documentation/admin-guide/osdump.rst
> @@ -0,0 +1,94 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===================================================
> +Documentation for osdump - The Os Minidump Solution
> +===================================================

I would title this simply: osdump

Also, a general comment: is it "osdump" or "Osdump"? I think you should
choose one and use it consistently.

> +
> +Overview
> +========
> +
> +Osdump is a new crash dumping solution aiming at specific embedded
> +devices within Automotive or Industrial having the following features:
> + - limited memory.
> + - require quick reboots after system faults.
> +
> +To minimize the dump file size and maximize the dump process speed,
> +Osdump provides the following features:
> + - excludes userspace memory, focusing solely on resolving OS-related issues.
> + - eliminates constant data such as text segments.
> + - core OS data for dumping, this includes:
> +    - static allocated bss and data segments
> +    - dynamic data such as:
> +       - slub data for small-sized data
> +       - some large-sized data alloced in pages.
> + - compresses dump data to help reduce dump file size
> +
> +Considering the large volume of non-contiguous data, a binary data format is
> +used directly.
> +
> +By utilizing the dump file and vmlinux, a standard elf format file can be
> +reassembled and parsed using crash tool.

> +Declare
> +========
> +Mainly test on arm64 with 2G DDR, selecting slub as SLAB allocator, 39-bit for
> +address space size.

This declaration isn't very useful information, I would take it out. You
already wrote that it targets small memory configurations -- if there
are hard limits, those should obviously be stated.

> +
> +Configure and Build
> +===================
> +
> +Kernel config
> +-------------
> +Disable CRASH_CORE, SLUB_DEBUG, and SLUB_TINY, and then select CONFIG_OS_MINIDUMP.
> +
> +Dts config
> +----------
> +Add one reserved region for osdump. Its size depends DDR size you are using. Here

I'm not sure about the use of "DDR" here and throughout the document.
Why not simply refer to it as "amount of memory" or RAM?

> +add one 16M reserved region with 2G's DDR.
> +
> +osdump {
> +	compatible = "osdump";
> +	reg = <0 0x64000000 0 0x1000000>;
> +};
> +
> +Build the image after you have done with configuration.
> +
> +Trigger one panic and analysis it
> +=================================

*analyse (or analyze)

> +
> +0) Precondition
> +---------------
> +DDR won't poweroff during panic. Its content should not be lose.

*lost

> +
> +1) Generate dump file
> +---------------------
> +When a panic occurs, it will dump core data into specific reserved region.

what does "it" refer to? Maybe write "the kernel".

I would also say "the core data" (instead of just "core data") since you
defined the "core" term above.

> +
> +echo c > /proc/sysrq-trigger
> +
> +2) Get dump file
> +----------------
> +When system reboot from panic, run the following command to get dump file.
> +
> +cat /proc/osdump > ./osdump
> +
> +Got 13M dump file on my side.

This isn't a sentence. Maybe write it out as "On a sample configuration
with X amount of memory, this results in a 13M dump file." or something
like that.

> +
> +3) Run crash tool to generate standard elf file
> +-----------------------------------------------
> +
> +crash --enable_raw_dumpfile ./vmlinux ./osdump  -m vabits_actual=39 --machdep\
> +	kimage_voffset=0xffffffbf8f000000 --machdep phys_offset=0x80000000
> +
> +Add new feature for crash tool that reassemble a standard elf format file with
> +the dump file and vmlinux.

I'm not sure what this line refers to.

> Will submit this patch later.
This information doesn't belong in the documentation.

> +
> +4) Analysis with crash tool
> +---------------------------
> +
> +crash ./vmlinux ./osdump_elf

I would add a sentence above this saying something like "You should now
be able to use the `crash` utility as usual:"

> +
> +Note
> +====
> +disable when select CRASH_CORE, SLUB_DEBUG, or SLUB_TINY.

Again, this needs a full sentence. Something like: "osdump is
incompatible with the following options: CRASH_CORE, ..."

Although you kind of already wrote this above, so maybe those two things
could be merged and not repeated.

Please also make sure to thread your emails (using In-Reply-To: or
git-send-email --thread) when sending out a patch series, that should be
the default when using git send-email.

Hope this helps.


Vegard

Powered by blists - more mailing lists