Re: Problem with no mem_map arg to init functions change?

Re: Problem with no mem_map arg to init functions change?

[Ian Wienand]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

On Wed, Sep 01, 2004 at 08:15:10PM -0700, William Lee Irwin III wrote:
> Okay, you're diving into node_alloc_mem_map() when you shouldn't be.

OK, how about a patch like below for a start? node_alloc_mem_map()
calls alloc_bootmem_node() unconditionally, whereas we have already
setup a vmem map in arch/ia64/mm/contig.c:paging_init()

===== mm/page_alloc.c 1.234 vs edited =====
--- 1.234/mm/page_alloc.c       2004-08-27 16:59:59 +10:00
+++ edited/mm/page_alloc.c      2004-09-02 15:21:27 +10:00
@@ -1582,8 +1582,11 @@ void __init node_alloc_mem_map(struct pg
 {
        unsigned long size;

-       size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
-       pgdat->node_mem_map = alloc_bootmem_node(pgdat, size);
+       if (!pgdat->node_mem_map)
+       {
+               size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
+               pgdat->node_mem_map = alloc_bootmem_node(pgdat, size);
+       }
 #ifndef CONFIG_DISCONTIGMEM
        mem_map = contig_page_data.node_mem_map;
 #endif
===== arch/ia64/mm/contig.c 1.6 vs edited =====
--- 1.6/arch/ia64/mm/contig.c   2004-08-24 19:08:33 +10:00
+++ edited/arch/ia64/mm/contig.c        2004-09-02 15:33:35 +10:00
@@ -284,7 +284,6 @@ paging_init (void)
                free_area_init_node(0, &contig_page_data, zones_size,
                                    0, zholes_size);

-               mem_map = contig_page_data.node_mem_map;
                printk("Virtual mem_map starts at 0x%p\n", mem_map);
        }
 #else /* !CONFIG_VIRTUAL_MEM_MAP */

It's either that, or in arch/ia64/mm/contig.c:paging_init() finding
some way to set node_page_start_pfn (which will shortcut the test in
node_alloc_mem_map()) by walking the vmem_map?

With this applied, I no longer get the wrong address for the vmem_map
e.g. I get

Virtual mem_map starts at 0xa0007fffc7900000

on boot which is what I expect.  But it still stops with the same
problem :(

Bad page state at free_hot_cold_page (in process 'swapper', page a0007fffc7918d08)
flags:0x0000000000000000 mapping:0000000000000000 mapcount:1 count:0
Backtrace:

-i


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Problem with no mem_map arg to init functions change?

[William Lee Irwin III]

On Wed, Sep 01, 2004 at 08:15:10PM -0700, William Lee Irwin III wrote:
>> Okay, you're diving into node_alloc_mem_map() when you shouldn't be.

On Thu, Sep 02, 2004 at 03:36:59PM +1000, Ian Wienand wrote:
> OK, how about a patch like below for a start? node_alloc_mem_map()
> calls alloc_bootmem_node() unconditionally, whereas we have already
> setup a vmem map in arch/ia64/mm/contig.c:paging_init()

You don't thave CONFIG_DISCONTIGMEM set? Why are you bothering with
virtual mem_map if not? If the core helpers are insufficiently
lightweight in the CONFIG_DISCONTIGMEM=n case we are very well going
to have whatever sanction we need to repair it, as this is the common
case among end users of the most predominant hardware and architectures.


> === mm/page_alloc.c 1.234 vs edited ==> --- 1.234/mm/page_alloc.c       2004-08-27 16:59:59 +10:00
> +++ edited/mm/page_alloc.c      2004-09-02 15:21:27 +10:00
> @@ -1582,8 +1582,11 @@ void __init node_alloc_mem_map(struct pg
>  {
>         unsigned long size;
> 
> -       size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
> -       pgdat->node_mem_map = alloc_bootmem_node(pgdat, size);
> +       if (!pgdat->node_mem_map)
> +       {
> +               size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
> +               pgdat->node_mem_map = alloc_bootmem_node(pgdat, size);
> +       }
>  #ifndef CONFIG_DISCONTIGMEM
>         mem_map = contig_page_data.node_mem_map;
>  #endif
> === arch/ia64/mm/contig.c 1.6 vs edited ==> --- 1.6/arch/ia64/mm/contig.c   2004-08-24 19:08:33 +10:00
> +++ edited/arch/ia64/mm/contig.c        2004-09-02 15:33:35 +10:00
> @@ -284,7 +284,6 @@ paging_init (void)
>                 free_area_init_node(0, &contig_page_data, zones_size,
>                                     0, zholes_size);
> 
> -               mem_map = contig_page_data.node_mem_map;
>                 printk("Virtual mem_map starts at 0x%p\n", mem_map);
>         }
>  #else /* !CONFIG_VIRTUAL_MEM_MAP */

Only the second of these changes should be necessary; the first is to
all appearances lunacy; bootmem.c would panic() before returning NULL.
The big question here, is, of course, why on earth anyone thought it
was a good idea to set vmem_map to NULL immediately before calling
free_area_init_node() and thereby completely hose the pfn macros in so
doing right before callers are about to use them. Whoever's responsible
for this mess in arch/ia64/mm/contig.c deserves at least a token LART.


On Thu, Sep 02, 2004 at 03:36:59PM +1000, Ian Wienand wrote:
> It's either that, or in arch/ia64/mm/contig.c:paging_init() finding
> some way to set node_page_start_pfn (which will shortcut the test in
> node_alloc_mem_map()) by walking the vmem_map?
> With this applied, I no longer get the wrong address for the vmem_map
> e.g. I get
> Virtual mem_map starts at 0xa0007fffc7900000
> on boot which is what I expect.  But it still stops with the same
> problem :(
> Bad page state at free_hot_cold_page (in process 'swapper', page a0007fffc7918d08)
> flags:0x0000000000000000 mapping:0000000000000000 mapcount:1 count:0
> Backtrace:

Something is going wrong besides merely clobbbering the virtual mem_map,
as the changes above at least refer it to the correct virtual addresses
for the page structures. What you really have to address is making
pfn_to_page() produce correct results at this phase of bootstrapping.
Clobbering vmem_map immediately prior to calling free_area_init_node()
is now incorrect.


-- wli

Re: Problem with no mem_map arg to init functions change?

[Ian Wienand]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Wed, Sep 01, 2004 at 11:10:30PM -0700, William Lee Irwin III wrote:
> You don't thave CONFIG_DISCONTIGMEM set? Why are you bothering with
> virtual mem_map if not? If the core helpers are insufficiently
> lightweight in the CONFIG_DISCONTIGMEM=n case we are very well going
> to have whatever sanction we need to repair it, as this is the common
> case among end users of the most predominant hardware and architectures.

Just to reply to this: the only reason is that VIRTUAL_MEMMAP is set
in zx1_defconfig.  Maybe it shouldn't be and I might never have
noticed the problem ...

-i

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Problem with no mem_map arg to init functions change?

[Matthew Wilcox]

On Wed, Sep 01, 2004 at 11:10:30PM -0700, William Lee Irwin III wrote:
> You don't thave CONFIG_DISCONTIGMEM set? Why are you bothering with
> virtual mem_map if not? If the core helpers are insufficiently
> lightweight in the CONFIG_DISCONTIGMEM=n case we are very well going
> to have whatever sanction we need to repair it, as this is the common
> case among end users of the most predominant hardware and architectures.

Ah, basic lack of understanding of what VIRTUAL_MEM_MAP is used for
and why it exists.  It should be exclusive with DISCONTIGMEM as they
both solve the same problem, but in wildly different ways.  At the time
VIRTUAL_MEM_MAP was put in, the DISCONTIGMEM code was utterly broken
and nobody was interested in fixing it ("we have a version in the LSE
patch that doesn't suck as much" doesn't help).

Both should address the memory map on zx1:

0-1 GB
257-260GB
4-256GB

(in practice, the maximum memory you can put in any zx1 box at the moment
is 128GB because 4GB DIMMs aren't supported in the rx5670)

Without VIRTUAL_MEM_MAP or DISCONTIGMEM, a 2GB zx1 machine would have
a 13GB mem_map.  So DISCONTIGMEM does away with the global mem_map and
VIRTUAL_MEM_MAP constructs a mem_map in vmalloc space rather than the
kernel's fixed mapping.

If DISCONTIGMEM now works properly, I think VIRTUAL_MEM_MAP can disappear.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: Problem with no mem_map arg to init functions change?

[Jesse Barnes]

On Wednesday, September 1, 2004 11:10 pm, William Lee Irwin III wrote:
> On Wed, Sep 01, 2004 at 08:15:10PM -0700, William Lee Irwin III wrote:
> >> Okay, you're diving into node_alloc_mem_map() when you shouldn't be.
>
> On Thu, Sep 02, 2004 at 03:36:59PM +1000, Ian Wienand wrote:
> > OK, how about a patch like below for a start? node_alloc_mem_map()
> > calls alloc_bootmem_node() unconditionally, whereas we have already
> > setup a vmem map in arch/ia64/mm/contig.c:paging_init()
>
> You don't thave CONFIG_DISCONTIGMEM set? Why are you bothering with
> virtual mem_map if not?

The zx1 machines in particular have sparse memory layouts that need virtual 
memmap, but not CONFIG_DISCONTIGMEM.  Enabling CONFIG_DISCONTIGMEM should 
work around the bug though.

Jesse

Re: Problem with no mem_map arg to init functions change?

[Jesse Barnes]

On Thursday, September 2, 2004 8:05 am, Matthew Wilcox wrote:
> On Wed, Sep 01, 2004 at 11:10:30PM -0700, William Lee Irwin III wrote:
> > You don't thave CONFIG_DISCONTIGMEM set? Why are you bothering with
> > virtual mem_map if not? If the core helpers are insufficiently
> > lightweight in the CONFIG_DISCONTIGMEM=n case we are very well going
> > to have whatever sanction we need to repair it, as this is the common
> > case among end users of the most predominant hardware and architectures.
>
> Ah, basic lack of understanding of what VIRTUAL_MEM_MAP is used for
> and why it exists.  It should be exclusive with DISCONTIGMEM as they
> both solve the same problem, but in wildly different ways.

Well, on ia64 it's used *by* DISCONTIGMEM to deal with nodes that have sparse 
memory layouts.

> If DISCONTIGMEM now works properly, I think VIRTUAL_MEM_MAP can disappear.

Except that the former uses the latter, and that won't change without a bit 
more work.  Even then, CONFIG_NUMA and CONFIG_DISCONTIGMEM are somewhat 
intertwined at this point, so using the discontig code to deal with a zx1 
memory layout isn't a good match.

Jesse

Re: Problem with no mem_map arg to init functions change?

[Randolph Chung]

> > If DISCONTIGMEM now works properly, I think VIRTUAL_MEM_MAP can disappear.
> 
> Except that the former uses the latter, and that won't change without a bit 
> more work.  Even then, CONFIG_NUMA and CONFIG_DISCONTIGMEM are somewhat 
> intertwined at this point, so using the discontig code to deal with a zx1 
> memory layout isn't a good match.

fwiw we recently implemented CONFIG_DISCONTIGMEM for parisc-linux to better
support N-class (and pa8800, which uses zx1). You are right that CONFIG_NUMA
and CONFIG_DISCONTIGMEM are a bit intertwined, but it can be made to work
:)

For reference we are targetting these three memory layouts:
 * Astro: 0-3.75, 67.75-68, 4-64
 * zx1: 0-1, 257-260, 4-256
 * Stretch (N-class): 0-2, 4-32, 34-xxx

Astro and Stretch have been verified (up to 32GB, so 3 zones), zx1 
verification is still pending, but i don't see why it wouldn't work (famous 
last words? :)

randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: Problem with no mem_map arg to init functions change?

[Jesse Barnes]

On Thursday, September 2, 2004 9:10 am, Randolph Chung wrote:
> > > If DISCONTIGMEM now works properly, I think VIRTUAL_MEM_MAP can
> > > disappear.
> >
> > Except that the former uses the latter, and that won't change without a
> > bit more work.  Even then, CONFIG_NUMA and CONFIG_DISCONTIGMEM are
> > somewhat intertwined at this point, so using the discontig code to deal
> > with a zx1 memory layout isn't a good match.
>
> fwiw we recently implemented CONFIG_DISCONTIGMEM for parisc-linux to better
> support N-class (and pa8800, which uses zx1). You are right that
> CONFIG_NUMA and CONFIG_DISCONTIGMEM are a bit intertwined, but it can be
> made to work
>
> :)
>
> For reference we are targetting these three memory layouts:
>  * Astro: 0-3.75, 67.75-68, 4-64
>  * zx1: 0-1, 257-260, 4-256
>  * Stretch (N-class): 0-2, 4-32, 34-xxx
>
> Astro and Stretch have been verified (up to 32GB, so 3 zones), zx1
> verification is still pending, but i don't see why it wouldn't work (famous
> last words? :)

Are you creating the whole memmap and then freeing up the holes?  IIRC that's 
what's done for x86 NUMA boxes, but that won't work if the inital memmap is 
too big to fit in memory in the first place.

Jesse

Re: Problem with no mem_map arg to init functions change?

[Randolph Chung]

> Are you creating the whole memmap and then freeing up the holes?  IIRC that's 
> what's done for x86 NUMA boxes, but that won't work if the inital memmap is 
> too big to fit in memory in the first place.

this is what we do:

        for (i = 0; i < npmem_ranges; i++) {
                unsigned long start_pfn;
                unsigned long npages;

                start_pfn = pmem_ranges[i].start_pfn;
                npages = pmem_ranges[i].pages;

                bootmap_size = init_bootmem_node(NODE_DATA(i),
                                                bootmap_pfn,
                                                start_pfn,
                                                (start_pfn + npages) );
                free_bootmem_node(NODE_DATA(i),
                                  (start_pfn << PAGE_SHIFT),
                                  (npages << PAGE_SHIFT) );
                bootmap_pfn += (bootmap_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
                if ((start_pfn + npages) > max_pfn)
                        max_pfn = start_pfn + npages;
        }

we don't do anything explicit with the holes when DISCONTIGMEM is
enabled. when it's not enabled we are able to work with small holes by
using the technique you mentioned above.

randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: Problem with no mem_map arg to init functions change?

[William Lee Irwin III]

On Thu, Sep 02, 2004 at 04:05:56PM +0100, Matthew Wilcox wrote:
> Ah, basic lack of understanding of what VIRTUAL_MEM_MAP is used for
> and why it exists.  It should be exclusive with DISCONTIGMEM as they
> both solve the same problem, but in wildly different ways.  At the time
> VIRTUAL_MEM_MAP was put in, the DISCONTIGMEM code was utterly broken
> and nobody was interested in fixing it ("we have a version in the LSE
> patch that doesn't suck as much" doesn't help).
> Both should address the memory map on zx1:
> 0-1 GB
> 257-260GB
> 4-256GB
> (in practice, the maximum memory you can put in any zx1 box at the moment
> is 128GB because 4GB DIMMs aren't supported in the rx5670)
> Without VIRTUAL_MEM_MAP or DISCONTIGMEM, a 2GB zx1 machine would have
> a 13GB mem_map.  So DISCONTIGMEM does away with the global mem_map and
> VIRTUAL_MEM_MAP constructs a mem_map in vmalloc space rather than the
> kernel's fixed mapping.
> If DISCONTIGMEM now works properly, I think VIRTUAL_MEM_MAP can disappear.

My understanding of virtual mem_map was that it was used to remove some
kind of excessive address calculation overhead from CONFIG_DISCONTIGMEM.

This usage is valid too, of course. In this case we either need to adjust
include/asm-ia64/page.h for virtual mem_map or contig_page_data.mem_map
needs to be initialized with the virtual mem_map's base prior to calling
free_area_init_node(), and for that matter, Ian Wienand appears to fall
into the category of systems without a large gap because
find_largest_hole().  is returning something less than LARGE_GAP.

There are a lot of things going wrong at once here.


-- wli

Re: Problem with no mem_map arg to init functions change?

[Ian Wienand]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Thu, Sep 02, 2004 at 11:42:43AM -0700, William Lee Irwin III wrote:
> This usage is valid too, of course. In this case we either need to adjust
> include/asm-ia64/page.h for virtual mem_map

Thanks to all the explanations I think I'm starting to understand all
these options; I'll try to document things a little more fully in our
WiKi soon (http://www.gelato.unsw.edu.au/IA64wiki if you've never seen
it).

I applied the following trivial patch and things seem to work now.
This way, pfn_to_page(0) returns 0xa0007fff... (the virtual mem map)
which I belive to be correct.

===== include/asm-ia64/page.h 1.26 vs edited =====
--- 1.26/include/asm-ia64/page.h        2004-07-27 15:26:50 +10:00
+++ edited/include/asm-ia64/page.h      2004-09-03 12:04:37 +10:00
@@ -84,6 +84,13 @@
 #endif

 #ifndef CONFIG_DISCONTIGMEM
+# ifdef CONFIG_VIRTUAL_MEM_MAP
+extern struct page *vmem_map;
+#  define pfn_valid(pfn)       (((pfn) < max_mapnr) && ia64_pfn_valid(pfn))
+#  define page_to_pfn(page)    ((unsigned long) (page - vmem_map))
+#  define pfn_to_page(pfn)     (vmem_map + (pfn))
+# endif
+#else /* !CONFIG_VIRTUAL_MEM_MAP */
 #define pfn_valid(pfn)         (((pfn) < max_mapnr) && ia64_pfn_valid(pfn))
 #define page_to_pfn(page)      ((unsigned long) (page - mem_map))
 #define pfn_to_page(pfn)       (mem_map + (pfn))

-i

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Problem with no mem_map arg to init functions change?

[William Lee Irwin III]

On Fri, Sep 03, 2004 at 12:19:04PM +1000, Ian Wienand wrote:
> Thanks to all the explanations I think I'm starting to understand all
> these options; I'll try to document things a little more fully in our
> WiKi soon (http://www.gelato.unsw.edu.au/IA64wiki if you've never seen
> it).
> I applied the following trivial patch and things seem to work now.
> This way, pfn_to_page(0) returns 0xa0007fff... (the virtual mem map)
> which I belive to be correct.

Okay, this is a clear regression caused by a core patch breaking arches.
I think we should push for rapid merging with arch maintainer approval.
The following patch, which I took no part in writing, but only advised
Ian Wienand about what could be going wrong, is under consideration. His
announcement is included verbatim following my signature.

As I participated in the arch sweeps for the mem_map initialization
patches and participated in the review of the patch and so on, please
add Signed-off-by: William Irwin <wli@holomorphy.com>


-- wli

On Thu, Sep 02, 2004 at 11:42:43AM -0700, William Lee Irwin III wrote:
> This usage is valid too, of course. In this case we either need to adjust
> include/asm-ia64/page.h for virtual mem_map

Thanks to all the explanations I think I'm starting to understand all
these options; I'll try to document things a little more fully in our
WiKi soon (http://www.gelato.unsw.edu.au/IA64wiki if you've never seen
it).

I applied the following trivial patch and things seem to work now.
This way, pfn_to_page(0) returns 0xa0007fff... (the virtual mem map)
which I belive to be correct.

=== include/asm-ia64/page.h 1.26 vs edited ==--- 1.26/include/asm-ia64/page.h        2004-07-27 15:26:50 +10:00
+++ edited/include/asm-ia64/page.h      2004-09-03 12:04:37 +10:00
@@ -84,6 +84,13 @@
 #endif

 #ifndef CONFIG_DISCONTIGMEM
+# ifdef CONFIG_VIRTUAL_MEM_MAP
+extern struct page *vmem_map;
+#  define pfn_valid(pfn)       (((pfn) < max_mapnr) && ia64_pfn_valid(pfn))
+#  define page_to_pfn(page)    ((unsigned long) (page - vmem_map))
+#  define pfn_to_page(pfn)     (vmem_map + (pfn))
+# endif
+#else /* !CONFIG_VIRTUAL_MEM_MAP */
 #define pfn_valid(pfn)         (((pfn) < max_mapnr) && ia64_pfn_valid(pfn))
 #define page_to_pfn(page)      ((unsigned long) (page - mem_map))
 #define pfn_to_page(pfn)       (mem_map + (pfn))

-i

RE: Problem with no mem_map arg to init functions change?

[Luck, Tony]

>Thanks to all the explanations I think I'm starting to understand all
>these options; I'll try to document things a little more fully in our
>WiKi soon (http://www.gelato.unsw.edu.au/IA64wiki if you've never seen
>it).
>
>I applied the following trivial patch and things seem to work now.
>This way, pfn_to_page(0) returns 0xa0007fff... (the virtual mem map)
>which I belive to be correct.

Ian (and Bill),

Thanks for all the work you both put into this ... patch accepted
and in my bk trees for Andrew and Linus to pick up.

-Tony