[PATCH 0/2] Enable s390/arc/sparc to use generic thp deposit/withdraw

[PATCH 0/2] Enable s390/arc/sparc to use generic thp deposit/withdraw

[Vineet Gupta]

Hi,

This came out my debugging THP on ARC. The generic deposit/withdraw routines
can be easily adapted to work with pgtable_t != struct page *.

Build/Run tested on ARC only.

Thx,
-Vineet

Vineet Gupta (2):
  mm,thp: refactor generic deposit/withdraw routines for wider usage
  ARC: mm: THP: use generic THP deposit/withdraw

 arch/arc/include/asm/hugepage.h |  8 --------
 arch/arc/mm/tlb.c               | 37 -------------------------------------
 mm/pgtable-generic.c            | 27 +++++++++++++++++----------
 3 files changed, 17 insertions(+), 55 deletions(-)

-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] mm,thp: refactor generic deposit/withdraw routines for wider usage

[Vineet Gupta]

Generic pgtable_trans_huge_deposit()/pgtable_trans_huge_withdraw()
assume pgtable_t to be struct page * which is not true for all arches.
Thus arc, s390, sparch end up with their own copies despite no special
hardware requirements (unlike powerpc).

It seems massaging the code a bit can make it reusbale.

 - Use explicit casts to (struct page *). For existing users, this
   should be semantically no-op for existing users

 - The only addition is zero'ing out of page->lru which for arc leaves
   a stray entry in pgtable_t cause mm spew when such pgtable is freed.

  | huge_memory: BUG: failure at
  | ../mm/huge_memory.c:1858/__split_huge_page_map()!
  | CPU: 0 PID: 901 Comm: bw_mem Not tainted 4.4.0-00015-g0569c1459cfa-dirty
  |
  | Stack Trace:
  |  arc_unwind_core.constprop.1+0x94/0x104
  |  split_huge_page_to_list+0x5c0/0x920
  |  __split_huge_page_pmd+0xc8/0x1b4
  |  vma_adjust_trans_huge+0x104/0x1c8
  |  vma_adjust+0xf8/0x6d8
  |  __split_vma.isra.40+0xf8/0x174
  |  do_munmap+0x360/0x428
  |  SyS_munmap+0x28/0x44

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-arch@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 mm/pgtable-generic.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index 75664ed7e3ab..c9f2f6f8c7bb 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -155,13 +155,17 @@ void pmdp_splitting_flush(struct vm_area_struct *vma, unsigned long address,
 void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
 				pgtable_t pgtable)
 {
+	struct page *new = (struct page *)pgtable;
+	struct page *head;
+
 	assert_spin_locked(pmd_lockptr(mm, pmdp));
 
 	/* FIFO */
-	if (!pmd_huge_pte(mm, pmdp))
-		INIT_LIST_HEAD(&pgtable->lru);
+	head = (struct page *)pmd_huge_pte(mm, pmdp);
+	if (!head)
+		INIT_LIST_HEAD(&new->lru);
 	else
-		list_add(&pgtable->lru, &pmd_huge_pte(mm, pmdp)->lru);
+		list_add(&new->lru, &head->lru);
 	pmd_huge_pte(mm, pmdp) = pgtable;
 }
 #endif
@@ -170,20 +174,23 @@ void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
 /* no "address" argument so destroys page coloring of some arch */
 pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
 {
-	pgtable_t pgtable;
+	struct page *page;
 
 	assert_spin_locked(pmd_lockptr(mm, pmdp));
 
+	page = (struct page *)pmd_huge_pte(mm, pmdp);
+
 	/* FIFO */
-	pgtable = pmd_huge_pte(mm, pmdp);
-	if (list_empty(&pgtable->lru))
+	if (list_empty(&page->lru))
 		pmd_huge_pte(mm, pmdp) = NULL;
 	else {
-		pmd_huge_pte(mm, pmdp) = list_entry(pgtable->lru.next,
-					      struct page, lru);
-		list_del(&pgtable->lru);
+		pmd_huge_pte(mm, pmdp) = (pgtable_t) list_entry(page->lru.next,
+							struct page, lru);
+		list_del(&page->lru);
 	}
-	return pgtable;
+
+	memset(&page->lru, 0, sizeof(page->lru));
+	return (pgtable_t)page;
 }
 #endif
 
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm,thp: refactor generic deposit/withdraw routines for wider usage

[Martin Schwidefsky]

On Thu, 11 Feb 2016 14:58:26 +0530
Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> Generic pgtable_trans_huge_deposit()/pgtable_trans_huge_withdraw()
> assume pgtable_t to be struct page * which is not true for all arches.
> Thus arc, s390, sparch end up with their own copies despite no special
> hardware requirements (unlike powerpc).

s390 does have a special hardware requirement. pgtable_t is an address
for a 2K block of memory. It is *not* equivalent to a struct page *
which refers to a 4K block of memory. That has been the whole point
to introduce pgtable_t.

> It seems massaging the code a bit can make it reusbale.

Imho the new code for asm-generic looks fine, as long as the override
with __HAVE_ARCH_PGTABLE_DEPOSIT/__HAVE_ARCH_PGTABLE_WITHDRAW continues
to work I do not mind.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm,thp: refactor generic deposit/withdraw routines for wider usage

[Vineet Gupta]

On Thursday 11 February 2016 03:52 PM, Martin Schwidefsky wrote:
> On Thu, 11 Feb 2016 14:58:26 +0530
> Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> 
>> Generic pgtable_trans_huge_deposit()/pgtable_trans_huge_withdraw()
>> assume pgtable_t to be struct page * which is not true for all arches.
>> Thus arc, s390, sparch end up with their own copies despite no special
>> hardware requirements (unlike powerpc).
> 
> s390 does have a special hardware requirement. pgtable_t is an address
> for a 2K block of memory. It is *not* equivalent to a struct page *
> which refers to a 4K block of memory. That has been the whole point
> to introduce pgtable_t.

Actually my reference to hardware requirement was more like powerpc style save a
hash value some where etc.

Now pgtable_t need not be struct page * even if the actual sizes are same - e.g.
in ARC port I kept pgtable_t as pte_t * simply to avoid a few page_address() calls
in mm code (you could argue that is was a micro-optimization, anyways..)

So given I know nothing about s390 MMU internals, I still think you can switch to
the update generic version despite 2K vs. 4K. Agree ?

>> It seems massaging the code a bit can make it reusbale.
> 
> Imho the new code for asm-generic looks fine, as long as the override
> with __HAVE_ARCH_PGTABLE_DEPOSIT/__HAVE_ARCH_PGTABLE_WITHDRAW continues
> to work I do not mind.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm,thp: refactor generic deposit/withdraw routines for wider usage

[Martin Schwidefsky]

On Thu, 11 Feb 2016 16:23:33 +0530
Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> On Thursday 11 February 2016 03:52 PM, Martin Schwidefsky wrote:
> > On Thu, 11 Feb 2016 14:58:26 +0530
> > Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> > 
> >> Generic pgtable_trans_huge_deposit()/pgtable_trans_huge_withdraw()
> >> assume pgtable_t to be struct page * which is not true for all arches.
> >> Thus arc, s390, sparch end up with their own copies despite no special
> >> hardware requirements (unlike powerpc).
> > 
> > s390 does have a special hardware requirement. pgtable_t is an address
> > for a 2K block of memory. It is *not* equivalent to a struct page *
> > which refers to a 4K block of memory. That has been the whole point
> > to introduce pgtable_t.
> 
> Actually my reference to hardware requirement was more like powerpc style save a
> hash value some where etc.
> 
> Now pgtable_t need not be struct page * even if the actual sizes are same - e.g.
> in ARC port I kept pgtable_t as pte_t * simply to avoid a few page_address() calls
> in mm code (you could argue that is was a micro-optimization, anyways..)
> 
> So given I know nothing about s390 MMU internals, I still think you can switch to
> the update generic version despite 2K vs. 4K. Agree ?

No, we can not. For s390 a page table is aligned on a 2K boundary and is
only half the size of a page (except for KVM but that is another story).
For s390 a pgtable_t is a pointer to the memory location with the 256 ptes
and not a struct page *.

The cast "struct page *new = (struct page*)pgtable;" in your first patch
is already broken, "new" points to the memory of the page table and
the list_head operations will clobber that memory. You try to fix it up
with the memset to zero in pgtable_trans_huge_withdraw but that does not
correct the pte entries for s390 as an invalid page-table entry is *not*
all zeros.

In short, please let s390 keep its own copy of deposit/withdraw.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm,thp: refactor generic deposit/withdraw routines for wider usage

[Vineet Gupta]

On Thursday 11 February 2016 04:50 PM, Martin Schwidefsky wrote:
> On Thu, 11 Feb 2016 16:23:33 +0530
> Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> 
>> On Thursday 11 February 2016 03:52 PM, Martin Schwidefsky wrote:
>>> On Thu, 11 Feb 2016 14:58:26 +0530
>>> Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>>>
>>>> Generic pgtable_trans_huge_deposit()/pgtable_trans_huge_withdraw()
>>>> assume pgtable_t to be struct page * which is not true for all arches.
>>>> Thus arc, s390, sparch end up with their own copies despite no special
>>>> hardware requirements (unlike powerpc).
>>>
>>> s390 does have a special hardware requirement. pgtable_t is an address
>>> for a 2K block of memory. It is *not* equivalent to a struct page *
>>> which refers to a 4K block of memory. That has been the whole point
>>> to introduce pgtable_t.
>>
>> Actually my reference to hardware requirement was more like powerpc style save a
>> hash value some where etc.
>>
>> Now pgtable_t need not be struct page * even if the actual sizes are same - e.g.
>> in ARC port I kept pgtable_t as pte_t * simply to avoid a few page_address() calls
>> in mm code (you could argue that is was a micro-optimization, anyways..)
>>
>> So given I know nothing about s390 MMU internals, I still think you can switch to
>> the update generic version despite 2K vs. 4K. Agree ?
> 
> No, we can not. For s390 a page table is aligned on a 2K boundary and is
> only half the size of a page (except for KVM but that is another story).
> For s390 a pgtable_t is a pointer to the memory location with the 256 ptes
> and not a struct page *.
> 
> The cast "struct page *new = (struct page*)pgtable;" in your first patch
> is already broken, "new" points to the memory of the page table and
> the list_head operations will clobber that memory.

The current s390 code does something similar using a different struct cast. It is
still writing in pgtable_t - although at a different location.

> You try to fix it up
> with the memset to zero in pgtable_trans_huge_withdraw but that does not
> correct the pte entries for s390 as an invalid page-table entry is *not*
> all zeros.

Right so that is the problem - just trying to understand.

> In short, please let s390 keep its own copy of deposit/withdraw.

You got it - I'm out of the way :-)

Thx,
-Vineet

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] ARC: mm: THP: use generic THP deposit/withdraw

[Vineet Gupta]

Generic code can now cope with pgtable_t != struct page *

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/hugepage.h |  8 --------
 arch/arc/mm/tlb.c               | 37 -------------------------------------
 2 files changed, 45 deletions(-)

diff --git a/arch/arc/include/asm/hugepage.h b/arch/arc/include/asm/hugepage.h
index c5094de86403..8653ed2f2ec5 100644
--- a/arch/arc/include/asm/hugepage.h
+++ b/arch/arc/include/asm/hugepage.h
@@ -66,14 +66,6 @@ extern void update_mmu_cache_pmd(struct vm_area_struct *vma, unsigned long addr,
 
 #define has_transparent_hugepage() 1
 
-/* Generic variants assume pgtable_t is struct page *, hence need for these */
-#define __HAVE_ARCH_PGTABLE_DEPOSIT
-extern void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
-				       pgtable_t pgtable);
-
-#define __HAVE_ARCH_PGTABLE_WITHDRAW
-extern pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp);
-
 #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE
 extern void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start,
 				unsigned long end);
diff --git a/arch/arc/mm/tlb.c b/arch/arc/mm/tlb.c
index 2e731c87011e..b300479b8ad3 100644
--- a/arch/arc/mm/tlb.c
+++ b/arch/arc/mm/tlb.c
@@ -663,43 +663,6 @@ void update_mmu_cache_pmd(struct vm_area_struct *vma, unsigned long addr,
 	update_mmu_cache(vma, addr, &pte);
 }
 
-void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
-				pgtable_t pgtable)
-{
-	struct list_head *lh = (struct list_head *) pgtable;
-
-	assert_spin_locked(&mm->page_table_lock);
-
-	/* FIFO */
-	if (!pmd_huge_pte(mm, pmdp))
-		INIT_LIST_HEAD(lh);
-	else
-		list_add(lh, (struct list_head *) pmd_huge_pte(mm, pmdp));
-	pmd_huge_pte(mm, pmdp) = pgtable;
-}
-
-pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
-{
-	struct list_head *lh;
-	pgtable_t pgtable;
-
-	assert_spin_locked(&mm->page_table_lock);
-
-	pgtable = pmd_huge_pte(mm, pmdp);
-	lh = (struct list_head *) pgtable;
-	if (list_empty(lh))
-		pmd_huge_pte(mm, pmdp) = NULL;
-	else {
-		pmd_huge_pte(mm, pmdp) = (pgtable_t) lh->next;
-		list_del(lh);
-	}
-
-	pte_val(pgtable[0]) = 0;
-	pte_val(pgtable[1]) = 0;
-
-	return pgtable;
-}
-
 void local_flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start,
 			       unsigned long end)
 {
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>