| Message ID | 20170328052559.12912-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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>
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
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 >
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
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();
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(-)