On Tue, Nov 20, 2012 at 06:00:40PM +0200, Kirill A. Shutemov wrote:

On Fri, Nov 16, 2012 at 12:10:39PM -0800, David Rientjes wrote:

quoted

On Fri, 16 Nov 2012, Kirill A. Shutemov wrote:

quoted

quoted

quoted

quoted

quoted

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index d767a7c..05490b3 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c

@@ -1259,6 +1259,8 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 		pmd_t entry;
 		entry = pmdp_get_and_clear(mm, addr, pmd);
 		entry = pmd_modify(entry, newprot);
+		if (is_huge_zero_pmd(entry))
+			entry = pmd_wrprotect(entry);
 		set_pmd_at(mm, addr, pmd, entry);
 		spin_unlock(&vma->vm_mm->page_table_lock);
 		ret = 1;

Nack, this should be handled in pmd_modify().

I disagree. It means we will have to enable hzp per arch. Bad idea.

pmd_modify() only exists for those architectures with thp support already, 
so you've already implicitly enabled for all the necessary architectures 
with your patchset.

Now we have huge zero page fully implemented inside mm/huge_memory.c. Push
this logic to pmd_modify() means we expose hzp implementation details to
arch code. Looks ugly for me.

So you are suggesting that anybody who ever does pmd_modify() in the 
future is responsible for knowing about the zero page and to protect 
against giving it write permission in the calling code??

Looks like we don't need the patch at all.

IIUC, if you ask for PROT_WRITE vm_get_page_prot() will translate it to
_PAGE_COPY or similar and you'll only get the page writable on pagefault.

Could anybody confirm that it's correct?

-- 
 Kirill A. Shutemov

Andrew, please drop the patch or replace it with the patch below, if you
wish.

From 048e3d4c97202cfecab55ead2a816421dce4b382 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 7 Aug 2012 06:09:56 -0700
Subject: [PATCH] thp: change_huge_pmd(): make sure we don't try to make a
 page writable

mprotect core never tries to make page writable using change_huge_pmd().
Let's add an assert that the assumption is true. It's important to be
sure we will not make huge zero page writable.

Signed-off-by: Kirill A. Shutemov <redacted>
---
 mm/huge_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index f5589c0..5fba83b 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c

@@ -1245,6 +1245,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 		pmd_t entry;
 		entry = pmdp_get_and_clear(mm, addr, pmd);
 		entry = pmd_modify(entry, newprot);
+		BUG_ON(pmd_write(entry));
 		set_pmd_at(mm, addr, pmd, entry);
 		spin_unlock(&vma->vm_mm->page_table_lock);
 		ret = 1;

-- 
 Kirill A. Shutemov