On Fri, 17 Mar 2023 20:30:01 +0100 Tomáš Mudruňka <[email protected]> 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.
> meminfo is rather top-level and important. Is this data sufficiently
> important to justify a place there?
There is already "HardwareCorrupted" which is similar, but for
errors reported by ECC, so it makes sense to have both in single place.
> Please describe the value. The use-case(s). Why would people want
> this?
When running large fleet of devices without ECC RAM it's currently not
easy to do bulk monitoring for memory corruption. You have to parse dmesg,
but that's ring buffer so the error might disappear after some time.
In general i do not consider dmesg to be great API to query RAM status.
In several companies i've seen such errors remain undetected and cause
issues for way too long. So i think it makes sense to provide monitoring
API, so that we can safely detect and act upon them.
> Comment layout is unconventional.
Fixed in PATCH v2
> Coding style is unconventional (white spaces).
> I expect this code would look much cleaner if some temporaries were used.
Fixed in PATCH v2
> I don't understand this logic anyway. Why not just print the value of
> early_memtest_bad_size>>10 and be done with it.
I think 0 should be reported only when there was no error found at all.
If memtest detect 256 corrupt bytes it would report 0 kB without such logic.
Rounding down to 0 like that is not a good idea in my opinion,
because it will hide the fact something is wrong with the RAM in such case.
Therefore i've added logic that prevents rounding down to 0.
> The name implies a bool, but the comment says otherwise.
Fixed in PATCH v2
> It's a counter, but it's used as a boolean. Why not make it bool, and do
Fixed in PATCH v2
> Also, your email client is replacing tabs with spaces.
Fixed in PATCH v2
Currently the memtest results were only presented in dmesg.
This adds /proc/meminfo entry which can be easily used by scripts.
Signed-off-by: Tomas Mudrunka <[email protected]>
---
Documentation/filesystems/proc.rst | 8 ++++++++
fs/proc/meminfo.c | 13 +++++++++++++
include/linux/memblock.h | 2 ++
mm/memtest.c | 6 ++++++
4 files changed, 29 insertions(+)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 9d5fd9424..8740362f3 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -996,6 +996,7 @@ Example output. You may not have all of these fields.
VmallocUsed: 40444 kB
VmallocChunk: 0 kB
Percpu: 29312 kB
+ EarlyMemtestBad: 0 kB
HardwareCorrupted: 0 kB
AnonHugePages: 4149248 kB
ShmemHugePages: 0 kB
@@ -1146,6 +1147,13 @@ VmallocChunk
Percpu
Memory allocated to the percpu allocator used to back percpu
allocations. This stat excludes the cost of metadata.
+EarlyMemtestBad
+ The amount of RAM/memory in kB, that was identified as corrupted
+ by early memtest. If memtest was not run, this field will not
+ be displayed at all. Size is never rounded down to 0 kB.
+ That means if 0 kB is reported, you can safely assume
+ there was at least one pass of memtest and none of the passes
+ found a single faulty byte of RAM.
HardwareCorrupted
The amount of RAM/memory in KB, the kernel identifies as
corrupted.
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index 440960110..b43d0bd42 100644
--- 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
+ if (early_memtest_done) {
+ unsigned long early_memtest_bad_size_kb;
+
+ early_memtest_bad_size_kb = early_memtest_bad_size>>10;
+ if (early_memtest_bad_size && !early_memtest_bad_size_kb)
+ early_memtest_bad_size_kb = 1;
+ /* When 0 is reported, it means there actually was a successful test */
+ seq_printf(m, "EarlyMemtestBad: %5lu kB\n", early_memtest_bad_size_kb);
+ }
+#endif
+
#ifdef CONFIG_MEMORY_FAILURE
seq_printf(m, "HardwareCorrupted: %5lu kB\n",
atomic_long_read(&num_poisoned_pages) << (PAGE_SHIFT - 10));
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 50ad19662..f82ee3fac 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -597,6 +597,8 @@ extern int hashdist; /* Distribute hashes across NUMA nodes? */
#endif
#ifdef CONFIG_MEMTEST
+extern phys_addr_t early_memtest_bad_size; /* Size of faulty ram found by memtest */
+extern bool early_memtest_done; /* Was early memtest done? */
extern void early_memtest(phys_addr_t start, phys_addr_t end);
#else
static inline void early_memtest(phys_addr_t start, phys_addr_t end)
diff --git a/mm/memtest.c b/mm/memtest.c
index f53ace709..57149dfee 100644
--- a/mm/memtest.c
+++ b/mm/memtest.c
@@ -4,6 +4,9 @@
#include <linux/init.h>
#include <linux/memblock.h>
+bool early_memtest_done;
+phys_addr_t early_memtest_bad_size;
+
static u64 patterns[] __initdata = {
/* The first entry has to be 0 to leave memtest with zeroed memory */
0,
@@ -30,6 +33,7 @@ static void __init reserve_bad_mem(u64 pattern, phys_addr_t start_bad, phys_addr
pr_info(" %016llx bad mem addr %pa - %pa reserved\n",
cpu_to_be64(pattern), &start_bad, &end_bad);
memblock_reserve(start_bad, end_bad - start_bad);
+ early_memtest_bad_size += (end_bad - start_bad);
}
static void __init memtest(u64 pattern, phys_addr_t start_phys, phys_addr_t size)
@@ -61,6 +65,8 @@ static void __init memtest(u64 pattern, phys_addr_t start_phys, phys_addr_t size
}
if (start_bad)
reserve_bad_mem(pattern, start_bad, last_bad + incr);
+
+ early_memtest_done = true;
}
static void __init do_one_pass(u64 pattern, phys_addr_t start, phys_addr_t end)
--
2.40.0
On Tue, 21 Mar 2023 11:34:30 +0100 Tomas Mudrunka <[email protected]> wrote:
> Currently the memtest results were only presented in dmesg.
> This adds /proc/meminfo entry which can be easily used by scripts.
Looks good to me, thanks. But the changelog still doesn't explain why
we should make this change. I grabbed that from your other email and
used the below as the changelog:
: Currently the memtest results were only presented in dmesg.
:
: When running a large fleet of devices without ECC RAM it's currently not
: easy to do bulk monitoring for memory corruption. You have to parse
: dmesg, but that's a ring buffer so the error might disappear after some
: time. In general I do not consider dmesg to be a great API to query RAM
: status.
:
: In several companies I've seen such errors remain undetected and cause
: issues for way too long. So I think it makes sense to provide a monitoring
: API, so that we can safely detect and act upon them.
:
: This adds /proc/meminfo entry which can be easily used by scripts.