Tight check of pfn_valid on sparsemem

Kukjin, Could you test below patch?

I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
section 0 section 1 section 2
0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

include/linux/mmzone.h +2 -0

index b4d109e..6c2147a 100644
@@ -979,6 +979,8 @@ struct mem_section {
        struct page_cgroup *page_cgroup;
        unsigned long pad;
 #endif
+       unsigned long start_pfn;
+       unsigned long end_pfn;
 };
 
 #ifdef CONFIG_SPARSEMEM_EXTREME

include/linux/mmzone.h +2 -0

+{
+       return ((section && (section->section_mem_map &

SECTION_HAS_MEM_MAP)) &&

include/linux/mmzone.h +3 -0

+               (section->start_pfn <= pfn && pfn < section->end_pfn));
+}
+
 static inline int valid_section_nr(unsigned long nr)
 {
        return valid_section(__nr_to_section(nr));

mm/sparse.c +0 -0

 unsigned long end)
                if (!ms->section_mem_map)
                        ms->section_mem_map =

sparse_encode_early_nid(nid) |

SECTION_MARKED_PRESENT;

mm/sparse.c +2 -1

+               ms->start_pfn = start;
+               ms->end_pfn = end;
        }
 }
 
 
 
--

Kind regards,
Minchan Kim

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
section 0 section 1 section 2
0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

include/linux/mmzone.h +0 -0

index b4d109e..6c2147a 100644

Quoted 31 lines · On Tue, 13 Jul 2010 00:53:48 +0900 Minchan Kim <[email protected]> wrote:

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
     section 0             section 1              section 2 0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

include/linux/mmzone.h struct mem_section { · line 985

Quoted 12 lines

include/linux/mmzone.h +2 -0

index b4d109e..6c2147a 100644
@@ -979,6 +979,8 @@ struct mem_section {
         struct page_cgroup *page_cgroup;
         unsigned long pad;
  #endif
+       unsigned long start_pfn;
+       unsigned long end_pfn;
  };
 

I have 2 concerns.
 1. This makes mem_section twice. Wasting too much memory and not good for cache. But yes, you can put this under some CONFIG which has small number of mem_section[].

 2. This can't be help for a case where a section has multiple small holes.

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Quoted 34 lines · KAMEZAWA Hiroyuki

Quoted 31 lines · On Tue, 13 Jul 2010 00:53:48 +0900 Minchan Kim <[email protected]> wrote:

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
A A A section 0 A A A A A A section 1 A A A A A A A section 2 0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

Quoted 17 lines

Quoted 8 lines

include/linux/mmzone.h +0 -0

index b4d109e..6c2147a 100644
@@ -979,6 +979,8 @@ struct mem_section {
 A  A  A  A  struct page_cgroup *page_cgroup;
 A  A  A  A  unsigned long pad;

A #endif

Quoted 4 lines

+ A  A  A  unsigned long start_pfn;
+ A  A  A  unsigned long end_pfn;

A };

I have 2 concerns.
A 1. This makes mem_section twice. Wasting too much memory and not good for cache. A A But yes, you can put this under some CONFIG which has small number of mem_section[].

I think memory usage isn't a big deal. but for cache, we can move fields into just after section_mem_map.

Quoted 1 line

A 2. This can't be help for a case where a section has multiple small holes.

I agree. But this(not punched hole but not filled section problem) isn't such case. But it would be better to handle it altogether. :)

Quoted 1 line

Quoted 5 lines

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Look at free_unused_memmap. We don't unmap pte of hole memmap. Is __get_use effective, still?

Quoted 54 lines · On Tue, 13 Jul 2010 13:11:14 +0900 Minchan Kim <[email protected]> wrote:

Quoted 51 lines · KAMEZAWA Hiroyuki

Quoted 36 lines · On Tue, 13 Jul 2010 00:53:48 +0900 Minchan Kim <[email protected]> wrote:

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
     section 0             section 1              section 2 0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

include/linux/mmzone.h +0 -0

index b4d109e..6c2147a 100644

include/linux/mmzone.h struct mem_section { · line 985

Quoted 7 lines

include/linux/mmzone.h +2 -0

@@ -979,6 +979,8 @@ struct mem_section {
         struct page_cgroup *page_cgroup;
         unsigned long pad;
  #endif
+       unsigned long start_pfn;
+       unsigned long end_pfn;
  };
 

I have 2 concerns.
 1. This makes mem_section twice. Wasting too much memory and not good for cache. But yes, you can put this under some CONFIG which has small number of mem_section[].

Quoted 3 lines

I think memory usage isn't a big deal. but for cache, we can move fields into just after section_mem_map.

I don't think so. This addtional field can eat up the amount of memory you saved by unmap.

Quoted 13 lines

Quoted 2 lines

 2. This can't be help for a case where a section has multiple small holes.

I agree. But this(not punched hole but not filled section problem) isn't such case. But it would be better to handle it altogether. :)

Quoted 6 lines

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Quoted 3 lines

Look at free_unused_memmap. We don't unmap pte of hole memmap. Is __get_use effective, still?

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

A 2. This can't be help for a case where a section has multiple small holes.

I agree. But this(not punched hole but not filled section problem) isn't such case. But it would be better to handle it altogether. :)

Quoted 6 lines

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Quoted 5 lines

Quoted 3 lines

Look at free_unused_memmap. We don't unmap pte of hole memmap. Is __get_use effective, still?

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

I mean following as.
For example, there is a struct page in on 0x20000000.

int pfn_valid_mapped(unsigned long pfn)

{
       struct page *page = pfn_to_page(pfn); /* hole page is 0x2000000 */
       char *lastbyte = (char *)(page+1)-1;  /* lastbyte is 0x2000001f */
       char byte;

       /* We pass this test since free_unused_memmap doesn't unmap pte */
       if(__get_user(byte, page) != 0)				
               return 0;

Quoted 27 lines · On Tue, 13 Jul 2010 15:04:00 +0900 Minchan Kim <[email protected]> wrote:

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

 2. This can't be help for a case where a section has multiple small holes.

I agree. But this(not punched hole but not filled section problem) isn't such case. But it would be better to handle it altogether. :)

Quoted 6 lines

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Quoted 5 lines

Quoted 3 lines

Look at free_unused_memmap. We don't unmap pte of hole memmap. Is __get_use effective, still?

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

I mean following as.
For example, there is a struct page in on 0x20000000.

int pfn_valid_mapped(unsigned long pfn)
{
       struct page *page = pfn_to_page(pfn); /* hole page is 0x2000000 */ char *lastbyte = (char *)(page+1)-1;  /* lastbyte is 0x2000001f */ char byte;

Quoted 3 lines

       /* We pass this test since free_unused_memmap doesn't unmap pte */ if(__get_user(byte, page) != 0)
               return 0;

why ? When the page size is 4096 byte.

     0x1ffff000 - 0x1ffffffff
     0x20000000 - 0x200000fff are on the same page. And memory is mapped per page.

What we access by above __get_user() is a byte at [0x20000000, 0x20000001)

and it's unmapped if 0x20000000 is unmapped.

Thanks,
-Kame

Quoted 30 lines · KAMEZAWA Hiroyuki

Quoted 27 lines · On Tue, 13 Jul 2010 15:04:00 +0900 Minchan Kim <[email protected]> wrote:

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

A 2. This can't be help for a case where a section has multiple small holes.

I agree. But this(not punched hole but not filled section problem) isn't such case. But it would be better to handle it altogether. :)

Quoted 6 lines

Then, my proposal for HOLES_IN_MEMMAP sparsemem is below.
==
Some architectures unmap memmap[] for memory holes even with SPARSEMEM. To handle that, pfn_valid() should check there are really memmap or not. For that purpose, __get_user() can be used.

Quoted 5 lines

Quoted 3 lines

Look at free_unused_memmap. We don't unmap pte of hole memmap. Is __get_use effective, still?

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

I mean following as.
For example, there is a struct page in on 0x20000000.

int pfn_valid_mapped(unsigned long pfn)
{

A  A  A  A struct page *page = pfn_to_page(pfn); /* hole page is 0x2000000 */
A  A  A  A char *lastbyte = (char *)(page+1)-1; A /* lastbyte is 0x2000001f */
A  A  A  A char byte;

Quoted 8 lines

Quoted 3 lines

A  A  A  A /* We pass this test since free_unused_memmap doesn't unmap pte */
A  A  A  A if(__get_user(byte, page) != 0)
A  A  A  A  A  A  A  A return 0;

why ? When the page size is 4096 byte.

A  A  A 0x1ffff000 - 0x1ffffffff
A  A  A 0x20000000 - 0x200000fff are on the same page. And memory is mapped per page.

sizeof(struct page) is 32 byte.
So lastbyte is address of struct page + 32 byte - 1.

Quoted 1 line

What we access by above __get_user() is a byte at [0x20000000, 0x20000001)

Right.

Quoted 1 line

and it's unmapped if 0x20000000 is unmapped.

free_unused_memmap doesn't unmap pte although it returns the page to free list of buddy.

Quoted 2 lines

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

Quoted 2 lines · Minchan Kim

Quoted 2 lines

__get_user() works with TLB and page table, the vaddr is really mapped or not. If you got SEGV, __get_user() returns -EFAULT. It works per page granule.

Not in kernel space. It works on 1MB sections there.

Testing whether a page is mapped by __get_user is a hugely expensive way to test whether a PFN is valid.

It'd be cheaper to use our flatmem implementation of pfn_valid() instead.

Anyway, sparsemem is designed to be aligned to SECTION_SIZE of memmap. Please avoid adding new Spaghetti code without proper configs. Thanks,

Quoted 3 lines · On Tue, 13 Jul 2010 16:34:17 +0900 KAMEZAWA Hiroyuki…

Anyway, sparsemem is designed to be aligned to SECTION_SIZE of memmap. Please avoid adding new Spaghetti code without proper configs. Thanks,

Ok, I realized I misunderstand all. Arm doesn't unmap memmap but reuse the page for memmap without modifing ptes. My routine only works when ARM uses sparsemem_vmemmap. But yes, it isn't.

Hmm...How about using pfn_valid() for FLATMEM or avoid using SPARSEMEM ?
If you want conrols lower than SPARSEMEM, FLATMEM works better because ARM unmaps memmap.

How about stop using SPARSEMEM ? What's the benefit ? It just eats up memory for mem_section[].

Quoted 2 lines · KAMEZAWA Hiroyuki

How about stop using SPARSEMEM ? What's the benefit ? It just eats up memory for mem_section[].

The problem with that approach is that sometimes the mem_map array doesn't fit into any memory banks.

We've gone around the loop of using flatmem with holes punched in it, to using discontigmem, and now to using sparsemem. It seems none of these solutions does what we need for ARM. I guess that's the price we pay for not having memory architected to be at any particular place in the physical memory map.

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
section 0 section 1 section 2
0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Quoted 14 lines · Minchan Kim

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
section 0 section 1 section 2
0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

Quoted 8 lines

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.
So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Look at the declaration of struct mem_section for a second. It is meant to partition address space uniformly into backed and unbacked areas.

It comes with implicit size and offset information by means of SECTION_SIZE_BITS and the section's index in the section array.

Now you are not okay with the _granularity_ but propose to change _the model_ by introducing a subsection within each section and at the same time make the concept of a section completely meaningless: its size becomes arbitrary and its associated mem_map and flags will apply to the subsection only.

My question is: if the sections are not fine-grained enough, why not just make them?

The biggest possible section size to describe the memory population on this machine accurately is 16M. Why not set SECTION_SIZE_BITS to 24?

3 is not a big deal than 2 about memory usage.
If the system use memory space fully(MAX_PHYSMEM_BITS 31), it just consumes 1024(128 * 8) byte. So now I think best solution is 2.

Russell. What do you think about it?

Quoted 5 lines · Minchan Kim

3 is not a big deal than 2 about memory usage.
If the system use memory space fully(MAX_PHYSMEM_BITS 31), it just consumes 1024(128 * 8) byte. So now I think best solution is 2.

Russell. What do you think about it?

I'm not Russell, but I'll tell you what I think. :)

Make the sections 16MB. You suggestion to add the start/end pfns

_doubles_ the size of the structure, and its size overhead. We have systems with a pretty tremendous amount of memory with 16MB sections.

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. That would work OK if, for instance, you only needed to check a couple of memory sections in the system. It'll start to suck if you made the lists very long.

-- Dave

Quoted 7 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. That would work OK if, for instance, you only needed to check a couple of memory sections in the system. It'll start to suck if you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

Quoted 10 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 7 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in.  We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected.  That would work OK if, for instance, you only needed to check a couple of memory sections in the system.  It'll start to suck if you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

register address of mem_section to memmap itself's page struct's pg->private field. This means the page is used for memmap of the section.
Otherwise, the page is used for other purpose and memmap has a hole.

Hi, Kame.

Quoted 19 lines · KAMEZAWA Hiroyuki

Quoted 10 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 7 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. A We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. A That would work OK if, for instance, you only needed to check a couple of memory sections in the system. A It'll start to suck if you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

Quoted 3 lines

register address of mem_section to memmap itself's page struct's pg->private field. This means the page is used for memmap of the section.
Otherwise, the page is used for other purpose and memmap has a hole.

It's a very good idea. :)
But can this handle case that a page on memmap pages have struct page descriptor of hole?
I mean one page can include 128 page descriptor(4096 / 32).

In there, 64 page descriptor is valid but remain 64 page descriptor is on hole. In this case, free_memmap doesn't free the page.

Quoted 23 lines · On Wed, 14 Jul 2010 15:44:41 +0900 Minchan Kim <[email protected]> wrote:

Hi, Kame.

Quoted 19 lines · KAMEZAWA Hiroyuki

Quoted 10 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 7 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in.  We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected.  That would work OK if, for instance, you only needed to check a couple of memory sections in the system.  It'll start to suck if you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

Quoted 8 lines

Quoted 3 lines

register address of mem_section to memmap itself's page struct's pg->private field. This means the page is used for memmap of the section.
Otherwise, the page is used for other purpose and memmap has a hole.

It's a very good idea. :)
But can this handle case that a page on memmap pages have struct page descriptor of hole?
I mean one page can include 128 page descriptor(4096 / 32).

yes.

Quoted 2 lines

In there, 64 page descriptor is valid but remain 64 page descriptor is on hole. In this case, free_memmap doesn't free the page.

yes. but in that case, there are valid page decriptor for 64pages of holes. pfn_valid() should return true but PG_reserved is set.
(This is usual behavior.)

My intention is that

 - When all 128 page descriptors are unused, free_memmap() will free it. In that case, clear page->private of a page for freed page descriptors.

 - When some of page descriptors are used, free_memmap() can't free it and page->private points to &mem_section. We may have memmap for memory hole but pfn_valid() is a function to check there is memmap or not. The bahavior of pfn_valid() is valid.
  Anyway, you can't free only half of page.

Quoted 45 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Wed, 14 Jul 2010 15:44:41 +0900 Minchan Kim <[email protected]> wrote:

Hi, Kame.

Quoted 19 lines · KAMEZAWA Hiroyuki

Quoted 10 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 7 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. A We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. A That would work OK if, for instance, you only needed to check a couple of memory sections in the system. A It'll start to suck if you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

Quoted 8 lines

Quoted 3 lines

register address of mem_section to memmap itself's page struct's pg->private field. This means the page is used for memmap of the section.
Otherwise, the page is used for other purpose and memmap has a hole.

It's a very good idea. :)
But can this handle case that a page on memmap pages have struct page descriptor of hole?
I mean one page can include 128 page descriptor(4096 / 32).

yes.

Quoted 2 lines

In there, 64 page descriptor is valid but remain 64 page descriptor is on hole. In this case, free_memmap doesn't free the page.

yes. but in that case, there are valid page decriptor for 64pages of holes. pfn_valid() should return true but PG_reserved is set.
(This is usual behavior.)

My intention is that

Quoted 8 lines

A - When all 128 page descriptors are unused, free_memmap() will free it. A In that case, clear page->private of a page for freed page descriptors.

A - When some of page descriptors are used, free_memmap() can't free it

A  and page->private points to &mem_section. We may have memmap for memory
A  hole but pfn_valid() is a function to check there is memmap or not.
A  The bahavior of pfn_valid() is valid.
A  Anyway, you can't free only half of page.

Okay. I missed PageReserved.
Your idea seems to be good. :)

I looked at pagetypeinfo_showblockcount_print.
It doesn't check PageReserved. Instead of it, it does ugly memmap_valid_within. Can't we remove it and change it with PageReserved?

Quoted 6 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 6 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. That would work OK if, for instance, you only needed to check a couple of memory sections in the system. It'll start to suck

Quoted 4 lines

Quoted 1 line

you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

register address of mem_section to memmap itself's page struct's

field.
This means the page is used for memmap of the section. Otherwise, the page is used for other purpose and memmap has a hole.

---
 arch/arm/mm/init.c     |   11 ++++++++++-
 include/linux/mmzone.h |   19 ++++++++++++++++++-
 mm/sparse.c            |   37
+++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)

Index: mmotm-2.6.35-0701/include/linux/mmzone.h
=============================================================
======

include/linux/mmzone.h +18 -1

@@ -1047,11 +1047,28 @@ static inline struct mem_section *__pfn_
 	return __nr_to_section(pfn_to_section_nr(pfn));
 }
 
+#ifdef CONFIG_SPARSEMEM_HAS_PIT
+void mark_memmap_pit(unsigned long start, unsigned long end, bool valid);
+static inline int page_valid(struct mem_section *ms, unsigned long pfn)
+{
+	struct page *page = pfn_to_page(pfn);
+	struct page *__pg = virt_to_page(page);
+	return __pg->private == ms;
+}
+#else
+static inline int page_valid(struct mem_section *ms, unsigned long pfn)
+{
+	return 1;
+}
+#endif
+
 static inline int pfn_valid(unsigned long pfn)
 {
+	struct mem_section *ms;
 	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
 		return 0;
-	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
+	ms = __nr_to_section(pfn_to_section_nr(pfn));
+	return valid_section(ms) && page_valid(ms, pfn);
 }
 
 static inline int pfn_present(unsigned long pfn)

Index: mmotm-2.6.35-0701/mm/sparse.c
=============================================================
======

mm/sparse.c +37 -0

@@ -615,6 +615,43 @@ void __init sparse_init(void)
 	free_bootmem(__pa(usemap_map), size);
 }
 
+#ifdef CONFIT_SPARSEMEM_HAS_PIT
+/*
+ * Fill memmap's pg->private with a pointer to mem_section.
+ * pfn_valid() will check this later. (see include/linux/mmzone.h)
+ * The caller should call
+ * 	mark_memmap_pit(start, end, true) # for all allocated mem_map
+ * 	and, after that,
+ * 	mark_memmap_pit(start, end, false) # for all pits in mem_map.
+ * please see usage in ARM.
+ */
+void mark_memmap_pit(unsigned long start, unsigned long end, bool valid)
+{
+	struct mem_section *ms;
+	unsigned long pos, next;
+	struct page *pg;
+	void *memmap, *end;
+	unsigned long mapsize = sizeof(struct page) * PAGES_PER_SECTION;
+
+	for (pos = start;
+	     pos < end; pos = next) {
+		next = (pos + PAGES_PER_SECTION) &
 PAGE_SECTION_MASK;
+		ms = __pfn_to_section(pos);
+		if (!valid_section(ms))
+			continue;
+		for (memmap = pfn_to_page(pfn), end = pfn_to_page(next-1);
+		     memmap != end + 1;
+		     memmap += PAGE_SIZE) {
+			pg = virt_to_page(memmap);
+			if (valid)
+				pg->private = ms;
+			else
+				pg->private = NULL;
+		}
+	}
+}
+#endif
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 #ifdef CONFIG_SPARSEMEM_VMEMMAP

static inline struct page *kmalloc_section_memmap(unsigned long pnum, int

Index: mmotm-2.6.35-0701/arch/arm/mm/init.c
=============================================================
======

arch/arm/mm/init.c +10 -1

@@ -234,6 +234,13 @@ static void __init arm_bootmem_free(stru
 	arch_adjust_zones(zone_size, zhole_size);
 
 	free_area_init_node(0, zone_size, min, zhole_size);
+
+#ifdef CONFIG_SPARSEMEM
+	for_each_bank(i, mi) {
+		mark_memmap_pit(bank_start_pfn(mi->bank[i]),
+				bank_end_pfn(mi->bank[i]), true);
+	}
+#endif
 }
 
 #ifndef CONFIG_SPARSEMEM

@@ -386,8 +393,10 @@ free_memmap(unsigned long start_pfn, uns
 	 * If there are free pages between these,
 	 * free the section of the memmap array.
 	 */
-	if (pg < pgend)
+	if (pg < pgend) {
+		mark_memap_pit(pg >> PAGE_SHIFT, pgend >> PAGE_SHIFT,
 false);
 		free_bootmem(pg, pgend - pg);
+	}
 }
 

/*

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Wed, 14 Jul 2010 01:44:23 +0900 Minchan Kim <[email protected]> wrote:

Quoted 6 lines

If you _really_ can't make the section size smaller, and the vast majority of the sections are fully populated, you could hack something in. We could, for instance, have a global list that's mostly readonly which tells you which sections need to be have their sizes closely inspected. That would work OK if, for instance, you only needed to check a couple of memory sections in the system. It'll start to suck

if

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

you made the lists very long.

Thanks for advise. As I say, I hope Russell accept 16M section.

Hi,

Thanks for your inputs.

Quoted 5 lines

It seems what I needed was good sleep....
How about this if 16M section is not acceptable ?

== NOT TESTED AT ALL, EVEN NOT COMPILED ==

Yeah...

Couldn't build with s5pv210_defconfig when used mmotm tree, And couldn't apply your patch against latest mainline 35-rc5.

Could you please remake your patch against mainline 35-rc5? Or...please let me know how I can test on my board(smdkv210).

Kukjin, Could you test below patch?
I don't have any sparsemem system. Sorry.

-- CUT DOWN HERE --

Kukjin reported oops happen while he change min_free_kbytes
http://www.spinics.net/lists/arm-kernel/msg92894.html
It happen by memory map on sparsemem.

The system has a memory map following as.
section 0 section 1 section 2
0x20000000-0x25000000, 0x40000000-0x50000000, 0x50000000-0x58000000 SECTION_SIZE_BITS 28(256M)

It means section 0 is an incompletely filled section. Nontheless, current pfn_valid of sparsemem checks pfn loosely.

It checks only mem_section's validation.

So in above case, pfn on 0x25000000 can pass pfn_valid's validation check. It's not what we want.

The Following patch adds check valid pfn range check on pfn_valid of sparsemem.

Signed-off-by Minchan Kim <[email protected]>

Reported-by Kukjin Kim <[email protected]>

P.S)
It is just RFC. If we agree with this, I will make the patch on mmotm.

--

include/linux/mmzone.h +0 -0

index b4d109e..6c2147a 100644

include/linux/mmzone.h +7 -1

 #ifdef CONFIG_SPARSEMEM_EXTREME

@@ -1039,6 +1041,12 @@ static inline int valid_section(struct mem_section *section)
        return (section && (section->section_mem_map & SECTION_HAS_MEM_MAP));
 }
 
+static inline int valid_section_pfn(struct mem_section *section, unsigned long pfn)
+{
+       return ((section && (section->section_mem_map & SECTION_HAS_MEM_MAP)) &&
+               (section->start_pfn <= pfn && pfn < section->end_pfn));
+}
+
 static inline int valid_section_nr(unsigned long nr)
 {
        return valid_section(__nr_to_section(nr));

@@ -1053,7 +1061,7 @@ static inline int pfn_valid(unsigned long pfn)
 {
        if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
                return 0;
-       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
+       return valid_section_pfn(__nr_to_section(pfn_to_section_nr(pfn)), pfn);
 }
 

mm/sparse.c +0 -0

index 95ac219..bde9090 100644

I prefer Kamezawa's suggestion of mapping on a ZERO_PAGE-like page full of PageReserved struct pages because it would have better performance and be more in line with maintaining the assumptions of the memory model. If we go in this direction, I would strongly prefer it was an ARM-only thing.

Quoted 5 lines · Mel Gorman

I prefer Kamezawa's suggestion of mapping on a ZERO_PAGE-like page full of PageReserved struct pages because it would have better performance and be more in line with maintaining the assumptions of the memory model. If we go in this direction, I would strongly prefer it was an ARM-only thing.

As I've said, this is not possible without doing some serious page manipulation.

Plus the pages that where there become unusable as they don't correspond with a PFN or obey phys_to_virt(). So there's absolutely no point to this.

Now, why do we free the holes in the mem_map - because these holes can be extremely large. Every 512K of hole equates to one page of mem_map array.

Balance that against memory placed at 0xc0000000 physical on
some platforms, and with PHYSMEM_BITS at 32 and SECTION_SIZE_BITS at 19 - well, you do the maths. The result is certainly not pretty.