On Thu, Nov 15, 2012 at 01:47:33PM -0800, David Rientjes wrote:

On Thu, 15 Nov 2012, Kirill A. Shutemov wrote:

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.

quoted

What's wrong with the check?

Anybody using pmd_modify() to set new protections in the future perhaps 
without knowledge of huge zero page can incorrectly make the huge zero 
page writable, which can never be allowed to happen.  It's better to make 
sure it can never happen with the usual interface to modify protections.

I haven't found where we check if the page is a 4k zero page, but it's
definitely not pte_modify().

-- 
 Kirill A. Shutemov