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

Message ID 1455182907-15445-2-git-send-email-vgupta@synopsys.com
State New
Headers show

Commit Message

Vineet Gupta Feb. 11, 2016, 9:28 a.m. UTC
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(-)

Comments

Martin Schwidefsky Feb. 11, 2016, 10:22 a.m. UTC | #1
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.
Vineet Gupta Feb. 11, 2016, 10:53 a.m. UTC | #2
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.
Martin Schwidefsky Feb. 11, 2016, 11:20 a.m. UTC | #3
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.
Vineet Gupta Feb. 11, 2016, 12:29 p.m. UTC | #4
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

Patch
diff mbox

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