Re: [Lhms-devel] Re: [Lhns-devel] Merging Nonlinear and Numa style memory hotplug

[Yasunori Goto <ygoto@us.fujitsu.com>]

Dave-san.

Probably, all of your advices are right.
I was confused between my emulation environment and true NUMA machine.
I will modify them. Thanks a lot.

BTW, I have a question about nonlinear patch.
It is about difference between phys_section[] and mem_section[]
I suppose that phys_section[] looks like no-meaning now.
If it isn't necessary, __va() and __pa() translation can be more simple.
What is the purpose of phys_section[]. Is it for ppc64?

Bye.

> Some more comments on the first patch:
> 
> +#ifdef CONFIG_HOTPLUG_MEMORY_OF_NODE
> +               if (node_online(nid)) {
> +                       allocate_pgdat(nid);
> +                       printk ("node %d will remap to vaddr %08lx\n", nid,
> +                               (ulong) node_remap_start_vaddr[nid]);
> +               }else
> +                       NODE_DATA(nid)=NULL;
> +#else
>                 allocate_pgdat(nid);
>                 printk ("node %d will remap to vaddr %08lx - %08lx\n", nid,
>                         (ulong) node_remap_start_vaddr[nid],
>                         (ulong) pfn_to_kaddr(highstart_pfn
>                             - node_remap_offset[nid] + node_remap_size[nid]));
> +#endif
> 
> I don't think this chunk is very necessary.  The 'NODE_DATA(nid)=NULL;'
> is superfluous because the node_data[] is zeroed at boot:
> 
> NUMA:
> #define NODE_DATA(nid) (node_data[nid])
> non-NUMA:
> #define NODE_DATA(nid) (&contig_page_data)
> 
> Why not just make it:
> 
> +               if (!node_online(nid))
> +			continue;
> 
> That should at least get rid of the ifdef.
> 
> -       bootmap_size = init_bootmem_node(NODE_DATA(0), min_low_pfn, 0, system_max_low_pfn);
> +       bootmap_size = init_bootmem_node(NODE_DATA(0), min_low_pfn, 0,
> +           (system_max_low_pfn > node_end_pfn[0]) ?
> +           node_end_pfn[0] : system_max_low_pfn);
> 
> -       register_bootmem_low_pages(system_max_low_pfn);
> +       register_bootmem_low_pages((system_max_low_pfn > node_end_pfn[0]) ?
> +           node_end_pfn[0] : system_max_low_pfn);
> 
> How about using a temp variable here instead of those nasty conditionals?
> 
> +
> +#ifdef CONFIG_HOTPLUG_MEMORY_OF_NODE
> +               if (node_online(nid)){
> +                       if (nid)
> +                               memset(NODE_DATA(nid), 0, sizeof(pg_data_t));
> +                       NODE_DATA(nid)->pgdat_next = pgdat_list;
> +                       pgdat_list = NODE_DATA(nid);
> +                       NODE_DATA(nid)->enabled = 1;
> +               }
> +#else
>                 if (nid)
>                         memset(NODE_DATA(nid), 0, sizeof(pg_data_t));
>                 NODE_DATA(nid)->pgdat_next = pgdat_list;
>                 pgdat_list = NODE_DATA(nid);
> +#endif
> 
> I'd just take the ifdef out.  Wouldn't this work instead?
> 
> -               if (nid)
> -                       memset(NODE_DATA(nid), 0, sizeof(pg_data_t));
> -               NODE_DATA(nid)->pgdat_next = pgdat_list;
> -               pgdat_list = NODE_DATA(nid);
> +               if (node_online(nid)){
> +                       if (nid)
> +                               memset(NODE_DATA(nid), 0, sizeof(pg_data_t));
> +                       NODE_DATA(nid)->pgdat_next = pgdat_list;
> +                       pgdat_list = NODE_DATA(nid);
> +                       NODE_DATA(nid)->enabled = 1;
> +               }
> 
> +void set_max_mapnr_init(void)
> +{
> ...
> +       struct page *hsp=0;
> 
> Should just be 'struct page *hsp = NULL;'
> 
> +       for(i = 0; i < numnodes; i++) {
> +               if (!NODE_DATA(i))
> +                       continue;
> +               pgdat = NODE_DATA(i);
> +               size = pgdat->node_zones[ZONE_HIGHMEM].present_pages;
> +               if (!size)
> +                       continue;
> +               hsp = pgdat->node_zones[ZONE_HIGHMEM].zone_mem_map;
> +               if (hsp)
> +                       break;
> +       }
> 
> Doesn't this just find the lowest-numbered node's highmem?  Are you sure
> that no NUMA systems have memory at lower physical addresses on
> higher-numbered nodes?  I'm not sure that this is true.
> 
> +       if (hsp)
> +               highmem_start_page = hsp;
> +       else
> +               highmem_start_page = (struct page *)-1;
> 
> By not just BUG() here?  Do you check for 'highmem_start_page == -1' somewhere?
> 
> @@ -478,12 +482,35 @@ void __init mem_init(void)
>         totalram_pages += __free_all_bootmem();
> 
>         reservedpages = 0;
> +
> +#ifdef CONFIG_HOTPLUG_MEMORY_OF_NODE
> +       for (nid = 0; nid < numnodes; nid++){
> +               int start, end;
> +
> +               if ( !node_online(nid))
> +                       continue;
> +               if ( node_start_pfn[nid] >= max_low_pfn )
> +                       break;
> +
> +               start = node_start_pfn[nid];
> +               end = ( node_end_pfn[nid] < max_low_pfn) ?
> +                       node_end_pfn[nid] : max_low_pfn;
> +
> +               for ( tmp = start; tmp < end; tmp++)
> +                       /*
> +                        * Only count reserved RAM pages
> +                        */
> +                       if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
> +                               reservedpages++;
> +       }
> +#else
> 
> Again, I don't see what this loop is used for.  You appear to be trying
> to detect which nodes have lowmem.  Is there currently any x86 NUMA
> architecture that has lowmem on any node but node 0?
> 
> 
> 
> -- Dave
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email sponsored by Black Hat Briefings & Training.
> Attend Black Hat Briefings & Training, Las Vegas July 24-29 - 
> digital self defense, top technical experts, no vendor pitches, 
> unmatched networking opportunities. Visit www.blackhat.com
> _______________________________________________
> Lhns-devel mailing list
> Lhns-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lhns-devel

-- 
Yasunori Goto <ygoto at us.fujitsu.com>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>