Re: [PATCH] LoongArch: Fix memsection size

From: Jianmin Lv
Date: Wed Oct 26 2022 - 06:51:25 EST




On 2022/10/26 下午5:21, WANG Xuerui wrote:
On 2022/10/26 15:56, Jianmin Lv wrote:
On LoongArch, the physical address space ranging from 0 to 0xfffffff is
always memory, which is in the low half of the memsection range from 0 to
0x1fffffff with 512M memsection size, and the high half will be a hole with
invalid memory pages.

The description is incorrect. For systems with less than 512MiB of memory for example, I believe not every address from 0x0 to 0x0fff_ffff is valid;

As far as I know, almost all designs use 0x0 to 0x0fff_ffff as memory even with 512M because it's difficult to set cpu to support a combination with memory and non-memory for 0x0 to 0x0fff_ffff.

and regarding the latter part of the sentence, what you mean by "invalid memory pages"...

If with 512M memsection size, for the memsection from 0 to 0x1fffffff, low half (0 to 0xfffffff) is valid memory, and high half(0x10000000 to 0x1fffffff ) is I/O registers (of chipsets and cpu), so high half is not memory, I think it's better to change *with invalid memory pages* to *which is not memory.*


This situation may cause some issues. For example, the range of 0x10000000
to 0x1fffffff is io registers, which will be considered as valid memory range
since which is in the memsection of range of 0 to 0x1fffffff. During S3

... turns out to be totally valid, only of the I/O kind. (This might be a case of Chinglish that is actually conveying the incorrect meaning to unaware readers.)


The whole range of the 512M memsection will be totally valid by pfn_valid(), and let me try reduce the example description. :)


sleep and resume, these io registers will be saved and restored as valid memory
page (pfn_valid() of common version considers that all pages in a memsection
are valid), which will cause exception, especially when restoring during resume.

We can use 256M size for memsection of LoongArch, or use the way as ARM64 to
walk through all memory memblock to check if a mem pfn is valid which maybe
lower performance. For simplicity, this patch just use the former way.

And the rest of the commit message is, unfortunately, a bit too verbose and hard to understand in general. I have to look at the actual change (luckily, a one-liner in this case) to confirm my understanding.

I think your intent is just to *avoid stepping into IO memory region during suspend/resume by reducing the section size order by one*.

Yes.


Try reducing the verbosity of the commit message in v2?

Ok, let me try. :)

I can't proofread and edit every commit due to limited time, so you have to practice and improve your writing skills after all. I'll review that piece of text afterwards. :)

Ok, thanks.


Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>

diff --git a/arch/loongarch/include/asm/sparsemem.h b/arch/loongarch/include/asm/sparsemem.h
index 3d18cdf1b069..05903b40a625 100644
--- a/arch/loongarch/include/asm/sparsemem.h
+++ b/arch/loongarch/include/asm/sparsemem.h
@@ -8,7 +8,7 @@
   * SECTION_SIZE_BITS        2^N: how big each section will be
   * MAX_PHYSMEM_BITS        2^N: how much memory we can have in that space
   */
-#define SECTION_SIZE_BITS    29 /* 2^29 = Largest Huge Page Size */
+#define SECTION_SIZE_BITS    28
  #define MAX_PHYSMEM_BITS    48
  #endif /* CONFIG_SPARSEMEM */

The change is trivial indeed but I'm not immediately giving the R-b.