Re: [PATCH kernel] powerpc/iommu: Do not call PageTransHuge() on tail pages

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Wednesday 05 April 2017 08:29 AM, Alexey Kardashevskiy wrote:
> On 04/04/17 19:26, Aneesh Kumar K.V wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>
>>> The CMA pages migration code does not support compound pages at
>>> the moment so it performs few tests before proceeding to actual page
>>> migration.
>>>
>>> One of the tests - PageTransHuge() - has VM_BUG_ON_PAGE(PageTail()) as
>>> it should be called on head pages. Since we also test for PageCompound(),
>>> and it contains PageTail(), we can simply move PageCompound() in front
>>> of PageTransHuge() and therefore avoid possible VM_BUG_ON_PAGE.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>
>>> Some of actual POWER8 systems do crash on that BUG_ON.
>>> ---
>>>  arch/powerpc/mm/mmu_context_iommu.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
>>> index 497130c5c742..ba7fccf993b3 100644
>>> --- a/arch/powerpc/mm/mmu_context_iommu.c
>>> +++ b/arch/powerpc/mm/mmu_context_iommu.c
>>> @@ -81,7 +81,7 @@ struct page *new_iommu_non_cma_page(struct page *page, unsigned long private,
>>>  	gfp_t gfp_mask = GFP_USER;
>>>  	struct page *new_page;
>>>
>>> -	if (PageHuge(page) || PageTransHuge(page) || PageCompound(page))
>>> +	if (PageCompound(page) || PageHuge(page) || PageTransHuge(page))
>>
>>
>> A checked for compound page should be sufficient here, because a
>> Huge/TransHuge page is also marked compound.
>
>
> But PageCompound() calls PageTail() so PageTail() will be called on a trans
> page which is BUG_ON in PageTransHuge but it is not in PageCompound() -
> this inconsistency is bothering me. Does not this BUG_ON tell us that we
> should not be calling PageTail() on _any_ page?
>
> In other words, should I get a head page (via compound_head()) first and
> only then test the head page if it is thp/huge (as you suggested in a chat)?
>
>
>

I was suggesting to replace that if () condition with just

/* We don't handle hugetlb/THP pages yet */
if (PageCompund(page)) {

}

-aneesh