From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx190.postini.com [74.125.245.190]) by kanga.kvack.org (Postfix) with SMTP id 7CE986B0069 for ; Sun, 16 Sep 2012 06:14:51 -0400 (EDT) Received: by qady1 with SMTP id y1so1063241qad.14 for ; Sun, 16 Sep 2012 03:14:50 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20120914131428.1f530681.akpm@linux-foundation.org> References: <5052A7DF.4050301@gmail.com> <50530E39.5020100@jp.fujitsu.com> <20120914131428.1f530681.akpm@linux-foundation.org> Date: Sun, 16 Sep 2012 18:14:50 +0800 Message-ID: Subject: Re: [PATCH RESEND] memory hotplug: fix a double register section info bug From: xishi qiu Content-Type: multipart/alternative; boundary=0022158c0d1168e5a904c9ceed34 Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Yasuaki Ishimatsu , mgorman@suse.de, tony.luck@intel.com, Jiang Liu , qiuxishi@huawei.com, bessel.wang@huawei.com, wujianguo@huawei.com, paul.gortmaker@windriver.com, kamezawa.hiroyu@jp.fujitsu.com, kosaki.motohiro@jp.fujitsu.com, rientjes@google.com, Minchan Kim , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Wen Congyang --0022158c0d1168e5a904c9ceed34 Content-Type: text/plain; charset=UTF-8 On Sat, Sep 15, 2012 at 4:14 AM, Andrew Morton wrote: > On Fri, 14 Sep 2012 20:00:09 +0900 > Yasuaki Ishimatsu wrote: > > > > @@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct > pglist_data *pgdat) > > > end_pfn = pfn + pgdat->node_spanned_pages; > > > > > > /* register_section info */ > > > - for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) > > > - register_page_bootmem_info_section(pfn); > > > - > > > + for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { > > > + if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node)) > > > > I cannot judge whether your configuration is correct or not. > > Thus if it is correct, I want a comment of why the node check is > > needed. In usual configuration, a node does not span the other one. > > So it is natural that "pfn_to_nid(pfn) is same as "pgdat->node_id". > > Thus we may remove the node check in the future. > > yup. How does this look? > It looks fine to me. Some platforms can have this unusual configuration, not only in IA64. And register_page_bootmem_info_section() is only called by register_page_bootmem_info_node(), so we can delete pfn_valid() check in register_page_bootmem_info_section() Thanks Xishi Qiu > --- > a/mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix > +++ a/mm/memory_hotplug.c > @@ -185,6 +185,12 @@ void register_page_bootmem_info_node(str > > /* register_section info */ > for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { > + /* > + * Some platforms can assign the same pfn to multiple > nodes - on > + * node0 as well as nodeN. To avoid registering a pfn > against > + * multiple nodes we check that this pfn does not already > + * reside in some other node. > + */ > if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node)) > register_page_bootmem_info_section(pfn); > } > _ > > --0022158c0d1168e5a904c9ceed34 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Sat, Sep 15, 2012 at 4:14 AM, Andrew = Morton <akpm@linux-foundation.org> wrote:
On Fri, 14 Sep 2012 20:00:09 +0900
Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> wrote:

> > @@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct = pglist_data *pgdat)
> > =C2=A0 =C2=A0 end_pfn =3D pfn + pgdat->node_spanned_pages;
> >
> > =C2=A0 =C2=A0 /* register_section info */
> > - =C2=A0 for (; pfn < end_pfn; pfn +=3D PAGES_PER_SECTION)
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 register_page_bootmem_info_s= ection(pfn);
> > -
> > + =C2=A0 for (; pfn < end_pfn; pfn +=3D PAGES_PER_SECTION) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (pfn_valid(pfn) &&= ; (pfn_to_nid(pfn) =3D=3D node))
>
> I cannot judge whether your configuration is correct or not.
> Thus if it is correct, I want a comment of why the node check is
> needed. In usual configuration, a node does not span the other one. > So it is natural that "pfn_to_nid(pfn) is same as "pgdat->= ;node_id".
> Thus we may remove the node check in the future.

=C2=A0
yup. =C2=A0How does= this look?

It looks fine to me.=C2=A0S= ome platforms can have this unusual=C2=A0 configuration, not only in IA64.<= /div>
And=C2=A0register_page_bootmem_info_section() is only called by regist= er_page_bootmem_info_node(),
so we can delete pfn_valid() check i= n register_page_bootmem_info_section()
=C2=A0 =C2=A0
<= div>Thanks=C2=A0
Xishi Qiu


--- a/mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info= -bug-fix
+++ a/mm/memory_hotplug.c
@@ -185,6 +185,12 @@ void register_page_bootmem_info_node(str

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* register_section info */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 for (; pfn < end_pfn; pfn +=3D PAGES_PE= R_SECTION) {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Some platforms c= an assign the same pfn to multiple nodes - on
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* node0 as well as= nodeN. =C2=A0To avoid registering a pfn against
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* multiple nodes w= e check that this pfn does not already
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* reside in some o= ther node.
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (pfn_valid(= pfn) && (pfn_to_nid(pfn) =3D=3D node))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 register_page_bootmem_info_section(pfn);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
_


--0022158c0d1168e5a904c9ceed34-- -- 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: email@kvack.org