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

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

[Alexey Kardashevskiy]

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))
 		return NULL;
 
 	if (PageHighMem(page))
@@ -100,7 +100,7 @@ static int mm_iommu_move_page_from_cma(struct page *page)
 	LIST_HEAD(cma_migrate_pages);
 
 	/* Ignore huge pages for now */
-	if (PageHuge(page) || PageTransHuge(page) || PageCompound(page))
+	if (PageCompound(page) || PageHuge(page) || PageTransHuge(page))
 		return -EBUSY;
 
 	lru_add_drain();
-- 
2.11.0

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

[Michael Ellerman]

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.

So this is:

Fixes: 2e5bbb5461f1 ("KVM: PPC: Book3S HV: Migrate pinned pages out of CMA")

And therefore:

Cc: stable@vger.kernel.org # v4.9+

?

cheers

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

[Alexey Kardashevskiy]

On 28/03/17 21:45, Michael Ellerman 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.
> 
> So this is:
> 
> Fixes: 2e5bbb5461f1 ("KVM: PPC: Book3S HV: Migrate pinned pages out of CMA")
> 
> And therefore:
> 
> Cc: stable@vger.kernel.org # v4.9+
> 
> ?

May be, first I want to make sure this is enough fix, this is why I
(re)added Balbir in cc list.


-- 
Alexey

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

[Balbir Singh]

On Tue, 2017-03-28 at 16:25 +1100, Alexey Kardashevskiy wrote:
> 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>
> ---

The fix looks reasonable to me. I suspect the checks can be simplified
and we can support split and move of THP in the future.

For now, looks good

Acked-by: Balbir Singh <bsingharora@gmail.com>

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

[Aneesh Kumar K.V]

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. If we want to indicate that
we don't handle hugetlb and THP pages, we can write that as a comment ?



>  		return NULL;
>
>  	if (PageHighMem(page))
> @@ -100,7 +100,7 @@ static int mm_iommu_move_page_from_cma(struct page *page)
>  	LIST_HEAD(cma_migrate_pages);
>
>  	/* Ignore huge pages for now */
> -	if (PageHuge(page) || PageTransHuge(page) || PageCompound(page))
> +	if (PageCompound(page) || PageHuge(page) || PageTransHuge(page))
>  		return -EBUSY;
>
>  	lru_add_drain();
> -- 
> 2.11.0

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

[Alexey Kardashevskiy]

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)?


> If we want to indicate that
> we don't handle hugetlb and THP pages, we can write that as a comment ?
> 
> 
> 
>>  		return NULL;
>>
>>  	if (PageHighMem(page))
>> @@ -100,7 +100,7 @@ static int mm_iommu_move_page_from_cma(struct page *page)
>>  	LIST_HEAD(cma_migrate_pages);
>>
>>  	/* Ignore huge pages for now */
>> -	if (PageHuge(page) || PageTransHuge(page) || PageCompound(page))
>> +	if (PageCompound(page) || PageHuge(page) || PageTransHuge(page))
>>  		return -EBUSY;
>>
>>  	lru_add_drain();
>> -- 
>> 2.11.0
> 


-- 
Alexey

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

[Aneesh Kumar K.V]

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