| Message ID | CAH2-hcJicFJ0h76JzY2DoLNF+4Nk7vGtk8gQv8JWFikt6X-wfA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add results of early memtest to /proc/meminfo | expand |
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.
> 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
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c index 440960110..844bb7e17 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 + /* 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 */ + 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)); +#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..b206b2d9d 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 int early_memtest_done; /* How many memtest passes were 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..f8e9edebf 100644 --- a/mm/memtest.c +++ b/mm/memtest.c @@ -4,6 +4,9 @@ #include <linux/init.h> #include <linux/memblock.h> +int early_memtest_done = 0; +phys_addr_t early_memtest_bad_size = 0; + 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); }
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 <tomas.mudrunka@gmail.com> --- fs/proc/meminfo.c | 13 +++++++++++++ include/linux/memblock.h | 2 ++ mm/memtest.c | 5 +++++ 3 files changed, 20 insertions(+) static void __init memtest(u64 pattern, phys_addr_t start_phys, phys_addr_t size) @@ -78,6 +82,7 @@ static void __init do_one_pass(u64 pattern, phys_addr_t start, phys_addr_t end) memtest(pattern, this_start, this_end - this_start); } } + early_memtest_done++; } /* default is disabled */