[PATCH 0/3] optimize for powerpc _PAGE_NUMA

[PATCH 0/3] optimize for powerpc _PAGE_NUMA

[Liu Ping Fan]

I saw Aneesh had implemented Numa faults on ppc64. Most of them have been reviewed-by.
So I just diff mine and his to form some trivial modification.

Based on Aneesh's series  "[PATCH -V2 0/5] powerpc: mm: Numa faults support for ppc64"

Liu Ping Fan (3):
  powerpc: mm: make _PAGE_NUMA take effect
  mm: export numa_migrate_prep()
  powerpc: mm: optimize for the correctly placed page

 arch/powerpc/mm/hash_utils_64.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/mm.h              |  2 ++
 2 files changed, 34 insertions(+)

-- 
1.8.1.4

[PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Liu Ping Fan]

To enable the do_numa_page(), we should not fix _PAGE_NUMA in
hash_page(), so bail out for the case of pte_numa().

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/mm/hash_utils_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index fb176e9..9bf1195 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -1033,7 +1033,7 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
 
 	/* Get PTE and page size from page tables */
 	ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
-	if (ptep == NULL || !pte_present(*ptep)) {
+	if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
 		DBG_LOW(" no PTE !\n");
 		rc = 1;
 		goto bail;
-- 
1.8.1.4

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Aneesh Kumar K.V]

Liu Ping Fan <kernelfans@gmail.com> writes:

> To enable the do_numa_page(), we should not fix _PAGE_NUMA in
> hash_page(), so bail out for the case of pte_numa().
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/hash_utils_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index fb176e9..9bf1195 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -1033,7 +1033,7 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>
>  	/* Get PTE and page size from page tables */
>  	ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
> -	if (ptep == NULL || !pte_present(*ptep)) {
> +	if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
>  		DBG_LOW(" no PTE !\n");
>  		rc = 1;
>  		goto bail;

why ? , All the hash routines do check for _PAGE_PRESENT via access
variable.

-aneesh

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Benjamin Herrenschmidt]

On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
> Liu Ping Fan <kernelfans@gmail.com> writes:
> 
> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
> > hash_page(), so bail out for the case of pte_numa().

For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
them on patchwork) so I'll reply to this one.

Overall, your statement that this is a faster path needs to be backed up
with numbers.

The code is complicated enough as it-is, such additional mess in the low
level hashing code requires a good justification, and also a
demonstration that it doesn't add overhead to the normal hash path.

Cheers,
Ben.

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Liu ping fan]

On Mon, Dec 9, 2013 at 8:31 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
>> Liu Ping Fan <kernelfans@gmail.com> writes:
>>
>> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
>> > hash_page(), so bail out for the case of pte_numa().
>
> For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
> them on patchwork) so I'll reply to this one.
>
> Overall, your statement that this is a faster path needs to be backed up
> with numbers.
>
> The code is complicated enough as it-is, such additional mess in the low
> level hashing code requires a good justification, and also a
> demonstration that it doesn't add overhead to the normal hash path.
>
For the test, is it ok to have an user application to copy page where
all page are PG_mlocked?

Thanks and regards,
Pingfan

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Benjamin Herrenschmidt]

On Mon, 2013-12-09 at 14:17 +0800, Liu ping fan wrote:
> On Mon, Dec 9, 2013 at 8:31 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
> >> Liu Ping Fan <kernelfans@gmail.com> writes:
> >>
> >> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
> >> > hash_page(), so bail out for the case of pte_numa().
> >
> > For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
> > them on patchwork) so I'll reply to this one.
> >
> > Overall, your statement that this is a faster path needs to be backed up
> > with numbers.
> >
> > The code is complicated enough as it-is, such additional mess in the low
> > level hashing code requires a good justification, and also a
> > demonstration that it doesn't add overhead to the normal hash path.
> >
> For the test, is it ok to have an user application to copy page where
> all page are PG_mlocked?

If that specific scenario is relevant in practice, then yes, though also
demonstrate the lack of regression with some more normal path such as a
kernel compile.

Cheers,
Ben.

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Liu ping fan]

On Thu, Dec 5, 2013 at 6:53 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Liu Ping Fan <kernelfans@gmail.com> writes:
>
>> To enable the do_numa_page(), we should not fix _PAGE_NUMA in
>> hash_page(), so bail out for the case of pte_numa().
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/mm/hash_utils_64.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
>> index fb176e9..9bf1195 100644
>> --- a/arch/powerpc/mm/hash_utils_64.c
>> +++ b/arch/powerpc/mm/hash_utils_64.c
>> @@ -1033,7 +1033,7 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>>
>>       /* Get PTE and page size from page tables */
>>       ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
>> -     if (ptep == NULL || !pte_present(*ptep)) {
>> +     if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
>>               DBG_LOW(" no PTE !\n");
>>               rc = 1;
>>               goto bail;
>
> why ? , All the hash routines do check for _PAGE_PRESENT via access
> variable.
>
Going through __hash_page_4K(4k on 4k HW), I do not find such check.
Am I wrong? Or I will send out a patch to fix that.

Thanks and regards,
Pingfan
> -aneesh
>

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Benjamin Herrenschmidt]

On Wed, 2013-12-11 at 16:50 +0800, Liu ping fan wrote:
> > why ? , All the hash routines do check for _PAGE_PRESENT via access
> > variable.
> >
> Going through __hash_page_4K(4k on 4k HW), I do not find such check.
> Am I wrong? Or I will send out a patch to fix that.

We pass a bitmask of flags to check which are tested by doing an "andc"
of the PTE on that mask and checking if anything is left...

Ben.

Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect

[Liu ping fan]

On Wed, Dec 11, 2013 at 5:50 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2013-12-11 at 16:50 +0800, Liu ping fan wrote:
>> > why ? , All the hash routines do check for _PAGE_PRESENT via access
>> > variable.
>> >
>> Going through __hash_page_4K(4k on 4k HW), I do not find such check.
>> Am I wrong? Or I will send out a patch to fix that.
>
> We pass a bitmask of flags to check which are tested by doing an "andc"
> of the PTE on that mask and checking if anything is left...
>
Oh, see it, thank you very much

Regards,
Pingfan

[PATCH 2/3] mm: export numa_migrate_prep()

[Liu Ping Fan]

powerpc will use it in fast path.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/linux/mm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5ab0e22..420fb77 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1092,6 +1092,8 @@ extern unsigned long change_protection(struct vm_area_struct *vma, unsigned long
 extern int mprotect_fixup(struct vm_area_struct *vma,
 			  struct vm_area_struct **pprev, unsigned long start,
 			  unsigned long end, unsigned long newflags);
+extern int numa_migrate_prep(struct page *page, struct vm_area_struct *vma,
+				unsigned long addr, int page_nid);
 
 /*
  * doesn't attempt to fault and will return short.
-- 
1.8.1.4

[PATCH 3/3] powerpc: mm: optimize for the correctly placed page

[Liu Ping Fan]

The period check of _PAGE_NUMA can probably trigger the check on
the correctly placed page. For this case, we can just insert hpte and
do fast exception return.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/mm/hash_utils_64.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 9bf1195..735678c 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -965,6 +965,10 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
 	const struct cpumask *tmp;
 	int rc, user_region = 0, local = 0;
 	int psize, ssize;
+	pte_t old, new;
+	struct vm_area_struct *vma;
+	int page_nid, target_nid;
+	struct page *test_page;
 
 	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
 		ea, access, trap);
@@ -1033,12 +1037,40 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
 
 	/* Get PTE and page size from page tables */
 	ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
-	if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
+	if (ptep == NULL || !pte_present(*ptep)) {
 		DBG_LOW(" no PTE !\n");
 		rc = 1;
 		goto bail;
 	}
 
+	old = pte_val(*ptep);
+	if (pte_numa(old)) {
+		/* If fail to lock, let do_page_fault() to handle it */
+		if (down_read_trylock(&mm->mmap_sem)) {
+			vma = find_vma(mm, ea);
+			up_read(&mm->mmap_sem);
+			test_page = pte_page(old);
+			page_nid = page_to_nid(test_page);
+			target_nid = numa_migrate_prep(test_page, vma, ea,
+						page_nid);
+			if (target_nid < 0) {
+				new = pte_mknonnuma(old);
+				/* If ptep is modified under us,
+				 * just retry the access
+				 */
+				if (unlikely(cmpxchg(ptep, old, new) != old)) {
+					put_page(test_page);
+					return 0;
+				}
+				put_page(test_page);
+			}
+		} else {
+				put_page(test_page);
+				rc = 1;
+				goto bail;
+		}
+	}
+
 	/* Add _PAGE_PRESENT to the required access perm */
 	access |= _PAGE_PRESENT;
 
-- 
1.8.1.4

Re: [PATCH 3/3] powerpc: mm: optimize for the correctly placed page

[Aneesh Kumar K.V]

Liu Ping Fan <kernelfans@gmail.com> writes:

> The period check of _PAGE_NUMA can probably trigger the check on
> the correctly placed page. For this case, we can just insert hpte and
> do fast exception return.

I still don't understand why we need to handle numa faults in hash
page ? Are you trying to optimize the code path ? If so can you explain
the benefits ? Some numbers showing it is helping  ?


>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/hash_utils_64.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 9bf1195..735678c 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -965,6 +965,10 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>  	const struct cpumask *tmp;
>  	int rc, user_region = 0, local = 0;
>  	int psize, ssize;
> +	pte_t old, new;
> +	struct vm_area_struct *vma;
> +	int page_nid, target_nid;
> +	struct page *test_page;
>
>  	DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
>  		ea, access, trap);
> @@ -1033,12 +1037,40 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>
>  	/* Get PTE and page size from page tables */
>  	ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
> -	if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
> +	if (ptep == NULL || !pte_present(*ptep)) {
>  		DBG_LOW(" no PTE !\n");
>  		rc = 1;
>  		goto bail;
>  	}
>
> +	old = pte_val(*ptep);
> +	if (pte_numa(old)) {
> +		/* If fail to lock, let do_page_fault() to handle it */
> +		if (down_read_trylock(&mm->mmap_sem)) {

hmm is that something we want to do in hash_page ?

> +			vma = find_vma(mm, ea);
> +			up_read(&mm->mmap_sem);
> +			test_page = pte_page(old);
> +			page_nid = page_to_nid(test_page);
> +			target_nid = numa_migrate_prep(test_page, vma, ea,
> +						page_nid);
> +			if (target_nid < 0) {
> +				new = pte_mknonnuma(old);
> +				/* If ptep is modified under us,
> +				 * just retry the access
> +				 */
> +				if (unlikely(cmpxchg(ptep, old, new) != old)) {
> +					put_page(test_page);
> +					return 0;
> +				}
> +				put_page(test_page);
> +			}
> +		} else {
> +				put_page(test_page);
> +				rc = 1;
> +				goto bail;
> +		}
> +	}
> +
>  	/* Add _PAGE_PRESENT to the required access perm */
>  	access |= _PAGE_PRESENT;
>

-aneesh

Re: [PATCH 3/3] powerpc: mm: optimize for the correctly placed page

[Liu ping fan]

On Thu, Dec 5, 2013 at 6:58 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Liu Ping Fan <kernelfans@gmail.com> writes:
>
>> The period check of _PAGE_NUMA can probably trigger the check on
>> the correctly placed page. For this case, we can just insert hpte and
>> do fast exception return.
>
> I still don't understand why we need to handle numa faults in hash
> page ? Are you trying to optimize the code path ? If so can you explain
> the benefits ? Some numbers showing it is helping  ?
>
When return from hash_page(), we will take fast_exc_return_irq, while
from do_page_fault(), we take ret_from_except.
As the "fast" implies that there are more complicated logic to sync
the interrupt states in ret_from_except, which cost much.
Do you think so?

>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/mm/hash_utils_64.c | 34 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
>> index 9bf1195..735678c 100644
>> --- a/arch/powerpc/mm/hash_utils_64.c
>> +++ b/arch/powerpc/mm/hash_utils_64.c
>> @@ -965,6 +965,10 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>>       const struct cpumask *tmp;
>>       int rc, user_region = 0, local = 0;
>>       int psize, ssize;
>> +     pte_t old, new;
>> +     struct vm_area_struct *vma;
>> +     int page_nid, target_nid;
>> +     struct page *test_page;
>>
>>       DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
>>               ea, access, trap);
>> @@ -1033,12 +1037,40 @@ int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
>>
>>       /* Get PTE and page size from page tables */
>>       ptep = find_linux_pte_or_hugepte(pgdir, ea, &hugeshift);
>> -     if (ptep == NULL || !pte_present(*ptep) || pte_numa(*ptep)) {
>> +     if (ptep == NULL || !pte_present(*ptep)) {
>>               DBG_LOW(" no PTE !\n");
>>               rc = 1;
>>               goto bail;
>>       }
>>
>> +     old = pte_val(*ptep);
>> +     if (pte_numa(old)) {
>> +             /* If fail to lock, let do_page_fault() to handle it */
>> +             if (down_read_trylock(&mm->mmap_sem)) {
>
> hmm is that something we want to do in hash_page ?
>
Yes, the function has no relation with hash. But I think it depends on
whether it is worth to optimize or not.

Thanks and regards,
Pingfan
>> +                     vma = find_vma(mm, ea);
>> +                     up_read(&mm->mmap_sem);
>> +                     test_page = pte_page(old);
>> +                     page_nid = page_to_nid(test_page);
>> +                     target_nid = numa_migrate_prep(test_page, vma, ea,
>> +                                             page_nid);
>> +                     if (target_nid < 0) {
>> +                             new = pte_mknonnuma(old);
>> +                             /* If ptep is modified under us,
>> +                              * just retry the access
>> +                              */
>> +                             if (unlikely(cmpxchg(ptep, old, new) != old)) {
>> +                                     put_page(test_page);
>> +                                     return 0;
>> +                             }
>> +                             put_page(test_page);
>> +                     }
>> +             } else {
>> +                             put_page(test_page);
>> +                             rc = 1;
>> +                             goto bail;
>> +             }
>> +     }
>> +
>>       /* Add _PAGE_PRESENT to the required access perm */
>>       access |= _PAGE_PRESENT;
>>
>
> -aneesh
>