From: Andrew Morton <akpm@linux-foundation.org>
To: "Tomáš Mudruňka" <tomas.mudrunka@gmail.com>
Cc: Mike Rapoport <rppt@kernel.org>,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org
Subject: Re: [PATCH] Add results of early memtest to /proc/meminfo
Date: Fri, 17 Mar 2023 16:56:37 -0700 [thread overview]
Message-ID: <20230317165637.6be5414a3eb05d751da7d19f@linux-foundation.org> (raw)
In-Reply-To: <CAH2-hcJicFJ0h76JzY2DoLNF+4Nk7vGtk8gQv8JWFikt6X-wfA@mail.gmail.com>
On Fri, 17 Mar 2023 20:30:01 +0100 Tomáš Mudruňka <tomas.mudrunka@gmail.com> wrote:
> Currently the memtest results were only presented in dmesg.
> This adds /proc/meminfo entry which can be easily used by scripts.
>
/proc/meminfo is documented in Documentation/filesystems/proc.rst,
please.
meminfo is rather top-level and important. Is this data sufficiently
important to justify a place there?
Please describe the value. The use-case(s). Why would people want
this?
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -6,6 +6,7 @@
> #include <linux/hugetlb.h>
> #include <linux/mman.h>
> #include <linux/mmzone.h>
> +#include <linux/memblock.h>
> #include <linux/proc_fs.h>
> #include <linux/percpu.h>
> #include <linux/seq_file.h>
> @@ -131,6 +132,18 @@ static int meminfo_proc_show(struct seq_file *m, void
> *v)
> show_val_kb(m, "VmallocChunk: ", 0ul);
> show_val_kb(m, "Percpu: ", pcpu_nr_pages());
>
> +#ifdef CONFIG_MEMTEST
> + /* Only show 0 Bad memory when test was actually run.
> + * Make sure bad regions smaller than 1kB are not reported as 0.
> + * That way when 0 is reported we can be sure there actually was
> successful test */
Comment layout is unconventional.
> + if (early_memtest_done)
> + seq_printf(m, "EarlyMemtestBad: %5lu kB\n",
> + (unsigned long) (
> + ((early_memtest_bad_size>0) && (early_memtest_bad_size>>10 <= 0))
> + ? 1
> + : early_memtest_bad_size>>10));
Coding style is unconventional (white spaces).
I expect this code would look much cleaner if some temporaries were used.
if (early_memtest_done) {
unsigned long size = 1;
long sz = early_memtest_bad_size >> 10;
if (early_memtest_bad_size > 0 && sz <= 0)
size = sz;
seq_printf(m, "EarlyMemtestBad: %5lu kB\n", size)
}
(or something like that, I didn't try hard)
I don't understand this logic anyway. Why not just print the value of
early_memtest_bad_size>>10 and be done with it.
> +extern int early_memtest_done; /* How many memtest passes were done? */
The name implies a bool, but the comment says otherwise.
> start, phys_addr_t end)
> memtest(pattern, this_start, this_end - this_start);
> }
> }
> + early_memtest_done++;
It's a counter, but it's used as a boolean. Why not make it bool, and do
early_memtest_done = true;
here?
Also, your email client is replacing tabs with spaces.
next prev parent reply other threads:[~2023-03-17 23:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-17 19:30 [PATCH] Add results of early memtest to /proc/meminfo Tomáš Mudruňka
2023-03-17 23:56 ` Andrew Morton [this message]
2023-03-21 10:34 ` [PATCH v2] " Tomas Mudrunka
2023-03-21 20:01 ` Andrew Morton
2023-03-21 10:58 ` Re: [PATCH] " Tomas Mudrunka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230317165637.6be5414a3eb05d751da7d19f@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rppt@kernel.org \
--cc=tomas.mudrunka@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).