Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Jeff Moyer]

Dan Williams  writes:

> On Thu, Oct 22, 2015 at 1:48 PM, Jeff Moyer  wrote:
>> Dan Williams  writes:
>>
>>> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
>>> clear memory a page at a time to take advantage of non-temporal
>>> clear_page() implementations.  However, x86_64 does not use
>>> non-temporal instructions for clear_page(), and arch_clear_pmem() was
>>> always incurring the cost of __arch_wb_cache_pmem().
>>>
>>> Clean up the assumption that doing clear_pmem() a page at a time is more
>>> performant.
>>
>> Wouldn't another solution be to actually use non-temporal stores?
>
> Sure.
>
>> Why did you choose to punt?
>
> Just a priority call at this point.  Patches welcome of course ;-).

OK.  Patch is harmless.

Reviewed-by: Jeff Moyer 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Jeff Moyer]

Dan Williams  writes:

> On Thu, Oct 22, 2015 at 1:48 PM, Jeff Moyer  wrote:
>> Dan Williams  writes:
>>
>>> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
>>> clear memory a page at a time to take advantage of non-temporal
>>> clear_page() implementations.  However, x86_64 does not use
>>> non-temporal instructions for clear_page(), and arch_clear_pmem() was
>>> always incurring the cost of __arch_wb_cache_pmem().
>>>
>>> Clean up the assumption that doing clear_pmem() a page at a time is more
>>> performant.
>>
>> Wouldn't another solution be to actually use non-temporal stores?
>
> Sure.
>
>> Why did you choose to punt?
>
> Just a priority call at this point.  Patches welcome of course ;-).

OK.  Patch is harmless.

Reviewed-by: Jeff Moyer 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Ross Zwisler]

On Thu, Oct 22, 2015 at 01:10:21PM -0400, Dan Williams wrote:
> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
> clear memory a page at a time to take advantage of non-temporal
> clear_page() implementations.  However, x86_64 does not use
> non-temporal instructions for clear_page(), and arch_clear_pmem() was
> always incurring the cost of __arch_wb_cache_pmem().
> 
> Clean up the assumption that doing clear_pmem() a page at a time is more
> performant.
> 
> Cc: Ross Zwisler 
> Reported-by: Dave Hansen 
> Signed-off-by: Dan Williams 

Reviewed-by: Ross Zwisler 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Ross Zwisler]

On Thu, Oct 22, 2015 at 01:10:21PM -0400, Dan Williams wrote:
> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
> clear memory a page at a time to take advantage of non-temporal
> clear_page() implementations.  However, x86_64 does not use
> non-temporal instructions for clear_page(), and arch_clear_pmem() was
> always incurring the cost of __arch_wb_cache_pmem().
> 
> Clean up the assumption that doing clear_pmem() a page at a time is more
> performant.
> 
> Cc: Ross Zwisler 
> Reported-by: Dave Hansen 
> Signed-off-by: Dan Williams 

Reviewed-by: Ross Zwisler 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Dan Williams]

On Thu, Oct 22, 2015 at 1:48 PM, Jeff Moyer  wrote:
> Dan Williams  writes:
>
>> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
>> clear memory a page at a time to take advantage of non-temporal
>> clear_page() implementations.  However, x86_64 does not use
>> non-temporal instructions for clear_page(), and arch_clear_pmem() was
>> always incurring the cost of __arch_wb_cache_pmem().
>>
>> Clean up the assumption that doing clear_pmem() a page at a time is more
>> performant.
>
> Wouldn't another solution be to actually use non-temporal stores?

Sure.

> Why did you choose to punt?

Just a priority call at this point.  Patches welcome of course ;-).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Jeff Moyer]

Dan Williams  writes:

> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
> clear memory a page at a time to take advantage of non-temporal
> clear_page() implementations.  However, x86_64 does not use
> non-temporal instructions for clear_page(), and arch_clear_pmem() was
> always incurring the cost of __arch_wb_cache_pmem().
>
> Clean up the assumption that doing clear_pmem() a page at a time is more
> performant.

Wouldn't another solution be to actually use non-temporal stores?  Why
did you choose to punt?

Cheers,
Jeff

>
> Cc: Ross Zwisler 
> Reported-by: Dave Hansen 
> Signed-off-by: Dan Williams 
> ---
>  arch/x86/include/asm/pmem.h |7 +--
>  fs/dax.c|4 +---
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
> index d8ce3ec816ab..1544fabcd7f9 100644
> --- a/arch/x86/include/asm/pmem.h
> +++ b/arch/x86/include/asm/pmem.h
> @@ -132,12 +132,7 @@ static inline void arch_clear_pmem(void __pmem *addr, 
> size_t size)
>  {
>   void *vaddr = (void __force *)addr;
>  
> - /* TODO: implement the zeroing via non-temporal writes */
> - if (size == PAGE_SIZE && ((unsigned long)vaddr & ~PAGE_MASK) == 0)
> - clear_page(vaddr);
> - else
> - memset(vaddr, 0, size);
> -
> + memset(vaddr, 0, size);
>   __arch_wb_cache_pmem(vaddr, size);
>  }
>  
> diff --git a/fs/dax.c b/fs/dax.c
> index a86d3cc2b389..5dc33d788d50 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -623,9 +623,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned 
> long address,
>   goto fallback;
>  
>   if (buffer_unwritten() || buffer_new()) {
> - int i;
> - for (i = 0; i < PTRS_PER_PMD; i++)
> - clear_pmem(kaddr + i * PAGE_SIZE, PAGE_SIZE);
> + clear_pmem(kaddr, HPAGE_SIZE);
>   wmb_pmem();
>   count_vm_event(PGMAJFAULT);
>   mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
>
> ___
> Linux-nvdimm mailing list
> linux-nvd...@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Dan Williams]

Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
clear memory a page at a time to take advantage of non-temporal
clear_page() implementations.  However, x86_64 does not use
non-temporal instructions for clear_page(), and arch_clear_pmem() was
always incurring the cost of __arch_wb_cache_pmem().

Clean up the assumption that doing clear_pmem() a page at a time is more
performant.

Cc: Ross Zwisler 
Reported-by: Dave Hansen 
Signed-off-by: Dan Williams 
---
 arch/x86/include/asm/pmem.h |7 +--
 fs/dax.c|4 +---
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index d8ce3ec816ab..1544fabcd7f9 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -132,12 +132,7 @@ static inline void arch_clear_pmem(void __pmem *addr, 
size_t size)
 {
void *vaddr = (void __force *)addr;
 
-   /* TODO: implement the zeroing via non-temporal writes */
-   if (size == PAGE_SIZE && ((unsigned long)vaddr & ~PAGE_MASK) == 0)
-   clear_page(vaddr);
-   else
-   memset(vaddr, 0, size);
-
+   memset(vaddr, 0, size);
__arch_wb_cache_pmem(vaddr, size);
 }
 
diff --git a/fs/dax.c b/fs/dax.c
index a86d3cc2b389..5dc33d788d50 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -623,9 +623,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned 
long address,
goto fallback;
 
if (buffer_unwritten() || buffer_new()) {
-   int i;
-   for (i = 0; i < PTRS_PER_PMD; i++)
-   clear_pmem(kaddr + i * PAGE_SIZE, PAGE_SIZE);
+   clear_pmem(kaddr, HPAGE_SIZE);
wmb_pmem();
count_vm_event(PGMAJFAULT);
mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Dan Williams]

Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
clear memory a page at a time to take advantage of non-temporal
clear_page() implementations.  However, x86_64 does not use
non-temporal instructions for clear_page(), and arch_clear_pmem() was
always incurring the cost of __arch_wb_cache_pmem().

Clean up the assumption that doing clear_pmem() a page at a time is more
performant.

Cc: Ross Zwisler 
Reported-by: Dave Hansen 
Signed-off-by: Dan Williams 
---
 arch/x86/include/asm/pmem.h |7 +--
 fs/dax.c|4 +---
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index d8ce3ec816ab..1544fabcd7f9 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -132,12 +132,7 @@ static inline void arch_clear_pmem(void __pmem *addr, 
size_t size)
 {
void *vaddr = (void __force *)addr;
 
-   /* TODO: implement the zeroing via non-temporal writes */
-   if (size == PAGE_SIZE && ((unsigned long)vaddr & ~PAGE_MASK) == 0)
-   clear_page(vaddr);
-   else
-   memset(vaddr, 0, size);
-
+   memset(vaddr, 0, size);
__arch_wb_cache_pmem(vaddr, size);
 }
 
diff --git a/fs/dax.c b/fs/dax.c
index a86d3cc2b389..5dc33d788d50 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -623,9 +623,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned 
long address,
goto fallback;
 
if (buffer_unwritten() || buffer_new()) {
-   int i;
-   for (i = 0; i < PTRS_PER_PMD; i++)
-   clear_pmem(kaddr + i * PAGE_SIZE, PAGE_SIZE);
+   clear_pmem(kaddr, HPAGE_SIZE);
wmb_pmem();
count_vm_event(PGMAJFAULT);
mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Jeff Moyer]

Dan Williams  writes:

> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
> clear memory a page at a time to take advantage of non-temporal
> clear_page() implementations.  However, x86_64 does not use
> non-temporal instructions for clear_page(), and arch_clear_pmem() was
> always incurring the cost of __arch_wb_cache_pmem().
>
> Clean up the assumption that doing clear_pmem() a page at a time is more
> performant.

Wouldn't another solution be to actually use non-temporal stores?  Why
did you choose to punt?

Cheers,
Jeff

>
> Cc: Ross Zwisler 
> Reported-by: Dave Hansen 
> Signed-off-by: Dan Williams 
> ---
>  arch/x86/include/asm/pmem.h |7 +--
>  fs/dax.c|4 +---
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
> index d8ce3ec816ab..1544fabcd7f9 100644
> --- a/arch/x86/include/asm/pmem.h
> +++ b/arch/x86/include/asm/pmem.h
> @@ -132,12 +132,7 @@ static inline void arch_clear_pmem(void __pmem *addr, 
> size_t size)
>  {
>   void *vaddr = (void __force *)addr;
>  
> - /* TODO: implement the zeroing via non-temporal writes */
> - if (size == PAGE_SIZE && ((unsigned long)vaddr & ~PAGE_MASK) == 0)
> - clear_page(vaddr);
> - else
> - memset(vaddr, 0, size);
> -
> + memset(vaddr, 0, size);
>   __arch_wb_cache_pmem(vaddr, size);
>  }
>  
> diff --git a/fs/dax.c b/fs/dax.c
> index a86d3cc2b389..5dc33d788d50 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -623,9 +623,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned 
> long address,
>   goto fallback;
>  
>   if (buffer_unwritten() || buffer_new()) {
> - int i;
> - for (i = 0; i < PTRS_PER_PMD; i++)
> - clear_pmem(kaddr + i * PAGE_SIZE, PAGE_SIZE);
> + clear_pmem(kaddr, HPAGE_SIZE);
>   wmb_pmem();
>   count_vm_event(PGMAJFAULT);
>   mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
>
> ___
> Linux-nvdimm mailing list
> linux-nvd...@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/5] pmem, dax: clean up clear_pmem()

[Dan Williams]

On Thu, Oct 22, 2015 at 1:48 PM, Jeff Moyer  wrote:
> Dan Williams  writes:
>
>> Both, __dax_pmd_fault, and clear_pmem() were taking special steps to
>> clear memory a page at a time to take advantage of non-temporal
>> clear_page() implementations.  However, x86_64 does not use
>> non-temporal instructions for clear_page(), and arch_clear_pmem() was
>> always incurring the cost of __arch_wb_cache_pmem().
>>
>> Clean up the assumption that doing clear_pmem() a page at a time is more
>> performant.
>
> Wouldn't another solution be to actually use non-temporal stores?

Sure.

> Why did you choose to punt?

Just a priority call at this point.  Patches welcome of course ;-).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/