Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Wed, Sep 26, 2012 at 03:38:41PM +0800, Fengguang Wu wrote: > On Wed, Sep 26, 2012 at 02:06:08AM -0400, Naoya Horiguchi wrote: > > On Wed, Sep 26, 2012 at 12:02:34AM -0400, Naoya Horiguchi wrote: > > ... > > > > > + * page is a thp, not a non-huge compound page. > > > > > + */ > > > > > + else if (PageTransCompound(page) && !PageSlab(page)) > > > > > u |= 1 << KPF_THP; > > > > > > > > Good catch! > > > > > > > > Will this report THP for the various drivers that do __GFP_COMP > > > > page allocations? > > > > > > I'm afraid it will. I think of checking PageLRU as an alternative, > > > but it needs compound_head() to report tail pages correctly. > > > In this context, pages are not pinned or locked, so it's unsafe to > > > use compound_head() because it can return a dangling pointer. > > > Maybe it's a thp's/hugetlbfs's (not kpageflags specific) problem, > > > so going forward with compound_head() expecting that it will be > > > fixed in the future work can be an option. > > > > It seems that compound_trans_head() solves this problem, so I'll > > simply use it. > > Naoya, in fact I didn't quite catch your concerns. Why not just test > > PageTransCompound(page) && PageLRU(page) If we simply check PageLRU, tail pages in thp only show KPF_COMPOUND_TAIL and we can't distinguish them from tail pages in non-huge compound pages. Moreover this behavior is not consistent with that of hugetlbfs tail pages where tail pages also have KPF_HUGE and are distinct from non-huge compound pages. I show the output of page-types: offset len flags ... 2d400 1 ___U_lAMa_bH__t (thp head) 2d401 1ff T__ (thp tail) # no KPF_THP ... 77000 1 ___U___Ma__H_G_ (hugetlbfs head) 77001 1ff TG_ (hugetlbfs tail) ... 11fb50 1 ___H___ (compound head) 11fb51 3 T__ (compound tail) ... 11fb58 1 ___S___H___ (slab head) 11fb59 7 T__ (slab tail) H: KPF_COMPOUND_HEAD T: KPF_COMPOUND_TAIL G: KPF_HUGEt: KPF_THP So I think it's better to set KPF_THP on thp tail pages. > The whole page flag report thing is inherently racy and it's fine to > report wrong values due to races. The "__GFP_COMP reported as THP", > however, should be avoided because it will make consistent wrong > reporting of page flags. Yes, I agree with this point. Thanks, Naoya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Wed, Sep 26, 2012 at 02:06:08AM -0400, Naoya Horiguchi wrote: > On Wed, Sep 26, 2012 at 12:02:34AM -0400, Naoya Horiguchi wrote: > ... > > > > +* page is a thp, not a non-huge compound page. > > > > +*/ > > > > + else if (PageTransCompound(page) && !PageSlab(page)) > > > > u |= 1 << KPF_THP; > > > > > > Good catch! > > > > > > Will this report THP for the various drivers that do __GFP_COMP > > > page allocations? > > > > I'm afraid it will. I think of checking PageLRU as an alternative, > > but it needs compound_head() to report tail pages correctly. > > In this context, pages are not pinned or locked, so it's unsafe to > > use compound_head() because it can return a dangling pointer. > > Maybe it's a thp's/hugetlbfs's (not kpageflags specific) problem, > > so going forward with compound_head() expecting that it will be > > fixed in the future work can be an option. > > It seems that compound_trans_head() solves this problem, so I'll > simply use it. Naoya, in fact I didn't quite catch your concerns. Why not just test PageTransCompound(page) && PageLRU(page) The whole page flag report thing is inherently racy and it's fine to report wrong values due to races. The "__GFP_COMP reported as THP", however, should be avoided because it will make consistent wrong reporting of page flags. Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Wed, Sep 26, 2012 at 12:02:34AM -0400, Naoya Horiguchi wrote: ... > > > + * page is a thp, not a non-huge compound page. > > > + */ > > > + else if (PageTransCompound(page) && !PageSlab(page)) > > > u |= 1 << KPF_THP; > > > > Good catch! > > > > Will this report THP for the various drivers that do __GFP_COMP > > page allocations? > > I'm afraid it will. I think of checking PageLRU as an alternative, > but it needs compound_head() to report tail pages correctly. > In this context, pages are not pinned or locked, so it's unsafe to > use compound_head() because it can return a dangling pointer. > Maybe it's a thp's/hugetlbfs's (not kpageflags specific) problem, > so going forward with compound_head() expecting that it will be > fixed in the future work can be an option. It seems that compound_trans_head() solves this problem, so I'll simply use it. Naoya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Wed, Sep 26, 2012 at 10:47:54AM +0800, Fengguang Wu wrote: > On Tue, Sep 25, 2012 at 10:06:08PM -0400, Naoya Horiguchi wrote: > > On Tue, Sep 25, 2012 at 05:20:48PM -0700, David Rientjes wrote: > > > On Tue, 25 Sep 2012, Naoya Horiguchi wrote: > > > > > > > KPF_THP can be set on non-huge compound pages like slab pages, because > > > > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > > > > and breaks user space applications which look for thp via > > > > /proc/kpageflags. > > > > Currently thp is constructed only on anonymous pages, so this patch > > > > makes > > > > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > > > > > > > Changelog in v2: > > > > - add a comment in code > > > > > > > > Signed-off-by: Naoya Horiguchi > > > > > > Wouldn't PageTransCompound(page) && !PageHuge(page) && !PageSlab(page) be > > > better for a future extension of thp support? > > > > Yes, this saves us an additional change when thp starts handling pagecaches. > > Andrew, can you replace the previous version in -mm tree with new one below? > > > > Thanks, > > Naoya > > --- > > From: Naoya Horiguchi > > Date: Tue, 25 Sep 2012 21:30:25 -0400 > > Subject: [PATCH v3] kpageflags: fix wrong KPF_THP on slab pages > > > > KPF_THP can be set on non-huge compound pages like slab pages, because > > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > > s/sees/checks/ > > > and breaks user space applications which look for thp via /proc/kpageflags. > > This patch rules out setting KPF_THP wrongly by additional PageSlab check. > > > > Changelog in v3: > > - check PageSlab instead of PageAnon > > - fix patch subject > > > > Changelog in v2: > > - add a comment in code > > > > Signed-off-by: Naoya Horiguchi > > --- > > fs/proc/page.c | 7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/fs/proc/page.c b/fs/proc/page.c > > index 7fcd0d6..e36d1f3 100644 > > --- a/fs/proc/page.c > > +++ b/fs/proc/page.c > > @@ -115,7 +115,12 @@ u64 stable_page_flags(struct page *page) > > u |= 1 << KPF_COMPOUND_TAIL; > > if (PageHuge(page)) > > u |= 1 << KPF_HUGE; > > - else if (PageTransCompound(page)) > > + /* > > +* PageTransCompound can be true for slab pages because it just sees > > s/sees/checks/ > > > +* PG_head/PG_head, so we need to check PageSlab to make sure the given > > PG_head/PG_head should be PG_head/PG_tail. Ah, sorry for my carelessness. > > +* page is a thp, not a non-huge compound page. > > +*/ > > + else if (PageTransCompound(page) && !PageSlab(page)) > > u |= 1 << KPF_THP; > > Good catch! > > Will this report THP for the various drivers that do __GFP_COMP > page allocations? I'm afraid it will. I think of checking PageLRU as an alternative, but it needs compound_head() to report tail pages correctly. In this context, pages are not pinned or locked, so it's unsafe to use compound_head() because it can return a dangling pointer. Maybe it's a thp's/hugetlbfs's (not kpageflags specific) problem, so going forward with compound_head() expecting that it will be fixed in the future work can be an option. Thanks, Naoya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, Sep 25, 2012 at 10:06:08PM -0400, Naoya Horiguchi wrote: > On Tue, Sep 25, 2012 at 05:20:48PM -0700, David Rientjes wrote: > > On Tue, 25 Sep 2012, Naoya Horiguchi wrote: > > > > > KPF_THP can be set on non-huge compound pages like slab pages, because > > > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > > > and breaks user space applications which look for thp via > > > /proc/kpageflags. > > > Currently thp is constructed only on anonymous pages, so this patch makes > > > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > > > > > Changelog in v2: > > > - add a comment in code > > > > > > Signed-off-by: Naoya Horiguchi > > > > Wouldn't PageTransCompound(page) && !PageHuge(page) && !PageSlab(page) be > > better for a future extension of thp support? > > Yes, this saves us an additional change when thp starts handling pagecaches. > Andrew, can you replace the previous version in -mm tree with new one below? > > Thanks, > Naoya > --- > From: Naoya Horiguchi > Date: Tue, 25 Sep 2012 21:30:25 -0400 > Subject: [PATCH v3] kpageflags: fix wrong KPF_THP on slab pages > > KPF_THP can be set on non-huge compound pages like slab pages, because > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug s/sees/checks/ > and breaks user space applications which look for thp via /proc/kpageflags. > This patch rules out setting KPF_THP wrongly by additional PageSlab check. > > Changelog in v3: > - check PageSlab instead of PageAnon > - fix patch subject > > Changelog in v2: > - add a comment in code > > Signed-off-by: Naoya Horiguchi > --- > fs/proc/page.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/proc/page.c b/fs/proc/page.c > index 7fcd0d6..e36d1f3 100644 > --- a/fs/proc/page.c > +++ b/fs/proc/page.c > @@ -115,7 +115,12 @@ u64 stable_page_flags(struct page *page) > u |= 1 << KPF_COMPOUND_TAIL; > if (PageHuge(page)) > u |= 1 << KPF_HUGE; > - else if (PageTransCompound(page)) > + /* > + * PageTransCompound can be true for slab pages because it just sees s/sees/checks/ > + * PG_head/PG_head, so we need to check PageSlab to make sure the given PG_head/PG_head should be PG_head/PG_tail. > + * page is a thp, not a non-huge compound page. > + */ > + else if (PageTransCompound(page) && !PageSlab(page)) > u |= 1 << KPF_THP; Good catch! Will this report THP for the various drivers that do __GFP_COMP page allocations? Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, 25 Sep 2012, Naoya Horiguchi wrote: > KPF_THP can be set on non-huge compound pages like slab pages, because > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > and breaks user space applications which look for thp via /proc/kpageflags. > This patch rules out setting KPF_THP wrongly by additional PageSlab check. > > Changelog in v3: > - check PageSlab instead of PageAnon > - fix patch subject > > Changelog in v2: > - add a comment in code > > Signed-off-by: Naoya Horiguchi Acked-by: David Rientjes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, Sep 25, 2012 at 05:20:48PM -0700, David Rientjes wrote: > On Tue, 25 Sep 2012, Naoya Horiguchi wrote: > > > KPF_THP can be set on non-huge compound pages like slab pages, because > > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > > and breaks user space applications which look for thp via /proc/kpageflags. > > Currently thp is constructed only on anonymous pages, so this patch makes > > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > > > Changelog in v2: > > - add a comment in code > > > > Signed-off-by: Naoya Horiguchi > > Wouldn't PageTransCompound(page) && !PageHuge(page) && !PageSlab(page) be > better for a future extension of thp support? Yes, this saves us an additional change when thp starts handling pagecaches. Andrew, can you replace the previous version in -mm tree with new one below? Thanks, Naoya --- From: Naoya Horiguchi Date: Tue, 25 Sep 2012 21:30:25 -0400 Subject: [PATCH v3] kpageflags: fix wrong KPF_THP on slab pages KPF_THP can be set on non-huge compound pages like slab pages, because PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug and breaks user space applications which look for thp via /proc/kpageflags. This patch rules out setting KPF_THP wrongly by additional PageSlab check. Changelog in v3: - check PageSlab instead of PageAnon - fix patch subject Changelog in v2: - add a comment in code Signed-off-by: Naoya Horiguchi --- fs/proc/page.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/proc/page.c b/fs/proc/page.c index 7fcd0d6..e36d1f3 100644 --- a/fs/proc/page.c +++ b/fs/proc/page.c @@ -115,7 +115,12 @@ u64 stable_page_flags(struct page *page) u |= 1 << KPF_COMPOUND_TAIL; if (PageHuge(page)) u |= 1 << KPF_HUGE; - else if (PageTransCompound(page)) + /* +* PageTransCompound can be true for slab pages because it just sees +* PG_head/PG_head, so we need to check PageSlab to make sure the given +* page is a thp, not a non-huge compound page. +*/ + else if (PageTransCompound(page) && !PageSlab(page)) u |= 1 << KPF_THP; /* -- 1.7.11.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, 25 Sep 2012, Naoya Horiguchi wrote: > KPF_THP can be set on non-huge compound pages like slab pages, because > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > and breaks user space applications which look for thp via /proc/kpageflags. > Currently thp is constructed only on anonymous pages, so this patch makes > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > Changelog in v2: > - add a comment in code > > Signed-off-by: Naoya Horiguchi Wouldn't PageTransCompound(page) && !PageHuge(page) && !PageSlab(page) be better for a future extension of thp support? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, Sep 25, 2012 at 1:05 PM, Naoya Horiguchi wrote: > On Tue, Sep 25, 2012 at 11:59:51AM -0400, KOSAKI Motohiro wrote: >> On Tue, Sep 25, 2012 at 9:56 AM, Naoya Horiguchi >> wrote: >> > KPF_THP can be set on non-huge compound pages like slab pages, because >> > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug >> > and breaks user space applications which look for thp via /proc/kpageflags. >> > Currently thp is constructed only on anonymous pages, so this patch makes >> > KPF_THP be set when both of PageAnon and PageTransCompound are true. >> >> Indeed. Please add some comment too. > > Sure. I send revised one. > > Thanks, > Naoya > --- > From: Naoya Horiguchi > Date: Mon, 24 Sep 2012 16:28:30 -0400 > Subject: [PATCH v2] pagemap: fix wrong KPF_THP on slab pages > > KPF_THP can be set on non-huge compound pages like slab pages, because > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > and breaks user space applications which look for thp via /proc/kpageflags. > Currently thp is constructed only on anonymous pages, so this patch makes > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > Changelog in v2: > - add a comment in code > > Signed-off-by: Naoya Horiguchi > --- > fs/proc/page.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/proc/page.c b/fs/proc/page.c > index 7fcd0d6..f7cd2f6c 100644 > --- a/fs/proc/page.c > +++ b/fs/proc/page.c > @@ -115,7 +115,12 @@ u64 stable_page_flags(struct page *page) > u |= 1 << KPF_COMPOUND_TAIL; > if (PageHuge(page)) > u |= 1 << KPF_HUGE; > - else if (PageTransCompound(page)) > + /* > +* Since THP is relevant only for anonymous pages so far, we check it > +* explicitly with PageAnon. Otherwise thp is confounded with non-huge > +* compound pages like slab pages. > +*/ > + else if (PageTransCompound(page) && PageAnon(page)) > u |= 1 << KPF_THP; Looks good to me. Acked-by: KOSAKI Motohiro -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, Sep 25, 2012 at 11:59:51AM -0400, KOSAKI Motohiro wrote: > On Tue, Sep 25, 2012 at 9:56 AM, Naoya Horiguchi > wrote: > > KPF_THP can be set on non-huge compound pages like slab pages, because > > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > > and breaks user space applications which look for thp via /proc/kpageflags. > > Currently thp is constructed only on anonymous pages, so this patch makes > > KPF_THP be set when both of PageAnon and PageTransCompound are true. > > Indeed. Please add some comment too. Sure. I send revised one. Thanks, Naoya --- From: Naoya Horiguchi Date: Mon, 24 Sep 2012 16:28:30 -0400 Subject: [PATCH v2] pagemap: fix wrong KPF_THP on slab pages KPF_THP can be set on non-huge compound pages like slab pages, because PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug and breaks user space applications which look for thp via /proc/kpageflags. Currently thp is constructed only on anonymous pages, so this patch makes KPF_THP be set when both of PageAnon and PageTransCompound are true. Changelog in v2: - add a comment in code Signed-off-by: Naoya Horiguchi --- fs/proc/page.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/proc/page.c b/fs/proc/page.c index 7fcd0d6..f7cd2f6c 100644 --- a/fs/proc/page.c +++ b/fs/proc/page.c @@ -115,7 +115,12 @@ u64 stable_page_flags(struct page *page) u |= 1 << KPF_COMPOUND_TAIL; if (PageHuge(page)) u |= 1 << KPF_HUGE; - else if (PageTransCompound(page)) + /* +* Since THP is relevant only for anonymous pages so far, we check it +* explicitly with PageAnon. Otherwise thp is confounded with non-huge +* compound pages like slab pages. +*/ + else if (PageTransCompound(page) && PageAnon(page)) u |= 1 << KPF_THP; /* -- 1.7.11.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pagemap: fix wrong KPF_THP on slab pages
On Tue, Sep 25, 2012 at 9:56 AM, Naoya Horiguchi wrote: > KPF_THP can be set on non-huge compound pages like slab pages, because > PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug > and breaks user space applications which look for thp via /proc/kpageflags. > Currently thp is constructed only on anonymous pages, so this patch makes > KPF_THP be set when both of PageAnon and PageTransCompound are true. Indeed. Please add some comment too. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] pagemap: fix wrong KPF_THP on slab pages
KPF_THP can be set on non-huge compound pages like slab pages, because PageTransCompound only sees PG_head and PG_tail. Obviously this is a bug and breaks user space applications which look for thp via /proc/kpageflags. Currently thp is constructed only on anonymous pages, so this patch makes KPF_THP be set when both of PageAnon and PageTransCompound are true. Signed-off-by: Naoya Horiguchi --- fs/proc/page.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git v3.6-rc6.orig/fs/proc/page.c v3.6-rc6/fs/proc/page.c index 7fcd0d6..613102d 100644 --- v3.6-rc6.orig/fs/proc/page.c +++ v3.6-rc6/fs/proc/page.c @@ -115,7 +115,7 @@ u64 stable_page_flags(struct page *page) u |= 1 << KPF_COMPOUND_TAIL; if (PageHuge(page)) u |= 1 << KPF_HUGE; - else if (PageTransCompound(page)) + else if (PageTransCompound(page) && PageAnon(page)) u |= 1 << KPF_THP; /* -- 1.7.11.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
