Re: [PATCH V3 1/6] powerpc/mm/book3s: Update pmd_present to look at _PAGE_PRESENT bit

On 9/21/18 11:25 AM, Christophe LEROY wrote:

Le 20/09/2018 à 20:09, Aneesh Kumar K.V a écrit :

quoted

diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index d71c7777669c..aee04b209b51 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c

@@ -188,11 +188,10 @@ void set_pte_at(struct mm_struct *mm, unsigned 

long addr, pte_t *ptep,
          pte_t pte)
  {
      /*
-     * When handling numa faults, we already have the pte marked
-     * _PAGE_PRESENT, but we can be sure that it is not in hpte.
-     * Hence we can use set_pte_at for them.
+     * Make sure hardware valid bit is not set. We don't do
+     * tlb flush for this update.
       */
-    VM_WARN_ON(pte_present(*ptep) && !pte_protnone(*ptep));
+    VM_WARN_ON(pte_val(*ptep) & _PAGE_PRESENT);

Why not using pte_present() anymore ?

Also, you are removing the pte_protnone() check, won't it change the 
behaviour ?

If we can't use pte_present(), can we create a new helper for that 
(allthough _PAGE_PRESENT exists on all platforms).

Christophe

This patch update a page table clear to clear _PAGE_PRESENT and mark it 
invalid via _PAGE_INVALID. The pte_present now looks at both the flag. 
That is we want these transient clear of pte to be considered as present 
pte even if _PAGE_PRESENT is cleared. What we are catching by the debug 
BUG_ON in these function is we are not using them to set a pte where the 
old entry has a hadware valid bit set. This is because we don't do any 
tlb flush with set_pte_at.


So the reason for pte_present -> pte_val() & _PAGE_PRESENT is because we 
swtiched the clear to clear _PAGE_PRESENT and set _PAGE_INVALID and 
pte_present now check both.

The reason for the removal of pte_protnone is because we dropped that 
set_pte_at usage from core autonuma code long time back.

Now Considering we are calling this from mm/pgtable.c With your approach 
of not using pte flags directly in core code we could switch this to 
pte_hw_valid(). May be we can do that as an addon patch?

-aneesh