On Wed, 8 Jun 2011 12:15:11 +0200
Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> On Wed, Jun 08, 2011 at 05:45:05PM +0900, KAMEZAWA Hiroyuki wrote:
> > @@ -196,7 +195,11 @@ static int __meminit init_section_page_cgroup(unsigned long pfn)
> > pc = base + index;
> > init_page_cgroup(pc, nr);
> > }
> > -
> > + /*
> > + * Even if passed 'pfn' is not aligned to section, we need to align
> > + * it to section boundary because of SPARSEMEM pfn calculation.
> > + */
> > + pfn = pfn & ~(PAGES_PER_SECTION - 1);
>
> PAGE_SECTION_MASK?
>
will use it.
> > section->page_cgroup = base - pfn;
> > total_usage += table_size;
> > return 0;
> > @@ -228,7 +231,7 @@ int __meminit online_page_cgroup(unsigned long start_pfn,
> > for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
> > if (!pfn_present(pfn))
> > continue;
> > - fail = init_section_page_cgroup(pfn);
> > + fail = init_section_page_cgroup(pfn, nid);
>
> AFAICS, nid can be -1 in the hotplug callbacks when there is a new
> section added to a node that already has memory, and then the
> allocation will fall back to numa_node_id().
>
Ah, thank you for pointing out.
> So I think we either need to trust start_pfn has valid mem map backing
> it (ARM has no memory hotplug support) and use pfn_to_nid(start_pfn),
> or find another way to the right node, no?
>
memory hotplug itself does nid = page_to_nid(pfn_to_page(pfn))..
so we can assume memmap of start_pfn will be valid..
> > @@ -285,14 +288,36 @@ void __init page_cgroup_init(void)
> > {
> > unsigned long pfn;
> > int fail = 0;
> > + int nid;
> >
> > if (mem_cgroup_disabled())
> > return;
> >
> > - for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
> > - if (!pfn_present(pfn))
> > - continue;
> > - fail = init_section_page_cgroup(pfn);
> > + for_each_node_state(nid, N_HIGH_MEMORY) {
> > + unsigned long start_pfn, end_pfn;
> > +
> > + start_pfn = NODE_DATA(nid)->node_start_pfn;
> > + end_pfn = start_pfn + NODE_DATA(nid)->node_spanned_pages;
> > + /*
> > + * Because we cannot trust page->flags of page out of node
> > + * boundary, we skip pfn < start_pfn.
> > + */
> > + for (pfn = start_pfn;
> > + !fail && (pfn < end_pfn);
> > + pfn = ALIGN(pfn + 1, PAGES_PER_SECTION)) {
>
> If we don't bother to align the pfn on the first iteration, I don't
> think we should for subsequent iterations. init_section_page_cgroup()
> has to be able to cope anyway. How about
>
> pfn += PAGES_PER_SECTION
>
> instead?
>
I thought of that but it means (pfn < end_pfn) goes wrong.
If pfn is not aligned.
pfn-------->end_pfn----------->pfn+PAGES_PER_SECTION
pages in [pfn..end_pfn) will not be handled. But I'd like to think about
this in the next version.
Thank you for review.
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>