On Fri, 17 Mar 2023 20:30:01 +0100 Tomáš Mudruňka <tomas.mudrunka@xxxxxxxxx> 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.
