On Mon, Dec 24, 2012 at 2:21 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Dec 24, 2012 at 10:53 AM, Christoffer Dall
> <cdall@xxxxxxxxxxxxxxx> wrote:
>>
>> I think I may have found an issue with the PageHead macro, which
>> returns true for tail compound pages when CONFIG_PAGEFLAGS_EXTENDED is
>> not defined.
>
> Hmm. Your patch *looks* obviously correct, in that it actually makes
> the code match the comment just above it. And making PageHead() test
> just the "compound" flag (and thus a tail-page would trigger it too)
> sounds wrong. But I join you in the "let's check the expected
> semantics with the people who use it" chorus.
>
> The fact that it fixes a problem on KVM/ARM is obviously another good sign.
>
> At the same time, I wonder why it hasn't shown up as a problem on
> x86-32. On x86-64 PAGEFLAGS_EXTENDED is always true, but afaik, it
> should be possible to trigger this on 32-bit architectures if you just
> have SPARSEMEM && !SPARSEMEM_VMEMMAP.
>
> And SPARSEMEM on x86-32 is enabled with NUMA or EXPERIMENTAL set. And
> afaik, x86-32 never has SPARSEMEM_VMEMMAP. So this should not be a
> very uncommon setup.
That's exactly why I was hesitant with just sending this out as a
patch. Then on the other hand, bugs from this problem *might* be so
subtle that it was simply not noticed.
>
> Added Andrea and Kirill to the Cc, since most of the *uses* of
> PageHead() in the generic VM code are attributed to either of them
> according to "git blame". Left the rest of the email quoted for the
> new participants.. Also, you seem to have used Christoph's old SGI
> email address that I don't think is in use any more.
Yep, just grabbed the e-mail from the commit that introduced the
problem, wasn't aware of the address change. Sorry about that.
Oh, and merry christmas :)
-Christoffer
>
> ---
>> I'm not sure however, if this indeed is the intended behavior and I'm
>> missing something overall. In any case, the below patch is a proposed
>> fix, which does fix a bug showing up on KVM/ARM with huge pages.
>>
>> Your input would be greatly appreciated.
>>
>> From: Christoffer Dall <cdall@xxxxxxxxxxxxxxx>
>> Date: Fri, 21 Dec 2012 13:03:50 -0500
>> Subject: [PATCH] mm: Fix PageHead when !CONFIG_PAGEFLAGS_EXTENDED
>>
>> Unfortunately with !CONFIG_PAGEFLAGS_EXTENDED, (!PageHead) is false, and
>> (PageHead) is true, for tail pages. If this is indeed the intended
>> behavior, which I doubt because it breaks cache cleaning on some ARM
>> systems, then the nomenclature is highly problematic.
>>
>> This patch makes sure PageHead is only true for head pages and PageTail
>> is only true for tail pages, and neither is true for non-compound pages.
>>
>> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxxxxxxx>
>> ---
>> include/linux/page-flags.h | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> index b5d1384..70473da 100644
>> --- a/include/linux/page-flags.h
>> +++ b/include/linux/page-flags.h
>> @@ -362,7 +362,7 @@ static inline void ClearPageCompound(struct page *page)
>> * pages on the LRU and/or pagecache.
>> */
>> TESTPAGEFLAG(Compound, compound)
>> -__PAGEFLAG(Head, compound)
>> +__SETPAGEFLAG(Head, compound) __CLEARPAGEFLAG(Head, compound)
>>
>> /*
>> * PG_reclaim is used in combination with PG_compound to mark the
>> @@ -374,8 +374,14 @@ __PAGEFLAG(Head, compound)
>> * PG_compound & PG_reclaim => Tail page
>> * PG_compound & ~PG_reclaim => Head page
>> */
>> +#define PG_head_mask ((1L << PG_compound))
>> #define PG_head_tail_mask ((1L << PG_compound) | (1L << PG_reclaim))
>>
>> +static inline int PageHead(struct page *page)
>> +{
>> + return ((page->flags & PG_head_tail_mask) == PG_head_mask);
>> +}
>> +
>> static inline int PageTail(struct page *page)
>> {
>> return ((page->flags & PG_head_tail_mask) == PG_head_tail_mask);
>> --
>> 1.7.9.5
>>
>>
>> Thanks!
>> -Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
