[PATCH 0/5] device-dax and huge-page dax fixes for 4.8-rc6

[PATCH 0/5] device-dax and huge-page dax fixes for 4.8-rc6

[Dan Williams]

Kai and Toshi reported poor performance with huge-page dax mappings and
while debugging a few more bugs were discovered in the device-dax driver
and mm.  The following fixes target 4.8-rc6 and are tagged for -stable:

- device-dax incorrectly translates the file offset to a physical
  resource address

- show_smap() crashes on huge-page dax mappings

- huge-page dax mappings are inadvertently being marked as
  _PAGE_CACHE_MODE_UC instead of _PAGE_CACHE_MODE_WB

I would like to take this set through nvdimm.git with acks from mm folks
as there is 4.9 device-dax development that depends on these changes.

---

Dan Williams (5):
      dax: fix mapping size check
      dax: fix offset to physical address translation
      mm: fix show_smap() for zone_device-pmd ranges
      mm: fix cache mode of dax pmd mappings
      mm: cleanup pfn_t usage in track_pfn_insert()


 arch/x86/mm/pat.c             |    4 ++--
 drivers/dax/dax.c             |   12 +++++++-----
 fs/proc/task_mmu.c            |    2 ++
 include/asm-generic/pgtable.h |    4 ++--
 mm/huge_memory.c              |    6 ++----
 mm/memory.c                   |    2 +-
 6 files changed, 16 insertions(+), 14 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/5] dax: fix mapping size check

[Dan Williams]

pgoff_to_phys() validates that both the starting address and the length
of the mapping against the resource list.  We need to check for a
mapping size of PMD_SIZE not PAGE_SIZE in the pmd fault path.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dax/dax.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index 803f3953b341..29f600f2c447 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -459,7 +459,7 @@ static int __dax_dev_pmd_fault(struct dax_dev *dax_dev,
 	}
 
 	pgoff = linear_page_index(vma, pmd_addr);
-	phys = pgoff_to_phys(dax_dev, pgoff, PAGE_SIZE);
+	phys = pgoff_to_phys(dax_dev, pgoff, PMD_SIZE);
 	if (phys == -1) {
 		dev_dbg(dev, "%s: phys_to_pgoff(%#lx) failed\n", __func__,
 				pgoff);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/5] dax: fix offset to physical address translation

[Dan Williams]

In pgoff_to_phys() 'pgoff' is already relative to base of the dax
device, so we only need to compare if the current offset is within the
current resource extent.  Otherwise, we are double accounting the
resource start offset when translating pgoff to a physical address.

Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dax/dax.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index 29f600f2c447..4653f84cabe7 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -357,16 +357,18 @@ static int check_vma(struct dax_dev *dax_dev, struct vm_area_struct *vma,
 static phys_addr_t pgoff_to_phys(struct dax_dev *dax_dev, pgoff_t pgoff,
 		unsigned long size)
 {
+	phys_addr_t phys, offset;
 	struct resource *res;
-	phys_addr_t phys;
 	int i;
 
+	offset = pgoff * PAGE_SIZE;
 	for (i = 0; i < dax_dev->num_resources; i++) {
 		res = &dax_dev->res[i];
-		phys = pgoff * PAGE_SIZE + res->start;
-		if (phys >= res->start && phys <= res->end)
+		if (offset < resource_size(res)) {
+			phys = offset + res->start;
 			break;
-		pgoff -= PHYS_PFN(resource_size(res));
+		}
+		offset -= resource_size(res);
 	}
 
 	if (i < dax_dev->num_resources) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/5] dax: fix offset to physical address translation

[Dan Williams]

On Tue, Sep 6, 2016 at 9:49 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> In pgoff_to_phys() 'pgoff' is already relative to base of the dax
> device, so we only need to compare if the current offset is within the
> current resource extent.  Otherwise, we are double accounting the
> resource start offset when translating pgoff to a physical address.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

On second look this results in the exact same translation, correct in
both cases.  This is also confirmed by a new ndctl unit test that does
data verification by writing through a /dev/pmem device and the
verifying via a /dev/dax device associated with the same namespace, so
I'm dropping this patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/5] mm: fix show_smap() for zone_device-pmd ranges

[Dan Williams]

Attempting to dump /proc/<pid>/smaps for a process with pmd dax mappings
currently results in the following VM_BUG_ONs:

 kernel BUG at mm/huge_memory.c:1105!
 task: ffff88045f16b140 task.stack: ffff88045be14000
 RIP: 0010:[<ffffffff81268f9b>]  [<ffffffff81268f9b>] follow_trans_huge_pmd+0x2cb/0x340
 [..]
 Call Trace:
  [<ffffffff81306030>] smaps_pte_range+0xa0/0x4b0
  [<ffffffff814c2755>] ? vsnprintf+0x255/0x4c0
  [<ffffffff8123c46e>] __walk_page_range+0x1fe/0x4d0
  [<ffffffff8123c8a2>] walk_page_vma+0x62/0x80
  [<ffffffff81307656>] show_smap+0xa6/0x2b0

 kernel BUG at fs/proc/task_mmu.c:585!
 RIP: 0010:[<ffffffff81306469>]  [<ffffffff81306469>] smaps_pte_range+0x499/0x4b0
 Call Trace:
  [<ffffffff814c2795>] ? vsnprintf+0x255/0x4c0
  [<ffffffff8123c46e>] __walk_page_range+0x1fe/0x4d0
  [<ffffffff8123c8a2>] walk_page_vma+0x62/0x80
  [<ffffffff81307696>] show_smap+0xa6/0x2b0

These locations are sanity checking page flags that must be set for an
anonymous transparent huge page, but are not set for the zone_device
pages associated with dax mappings.

Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/proc/task_mmu.c |    2 ++
 mm/huge_memory.c   |    4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 187d84ef9de9..f6fa99eca515 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -581,6 +581,8 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
 		mss->anonymous_thp += HPAGE_PMD_SIZE;
 	else if (PageSwapBacked(page))
 		mss->shmem_thp += HPAGE_PMD_SIZE;
+	else if (is_zone_device_page(page))
+		/* pass */;
 	else
 		VM_BUG_ON_PAGE(1, page);
 	smaps_account(mss, page, true, pmd_young(*pmd), pmd_dirty(*pmd));
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2db2112aa31e..a6abd76baa72 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1078,7 +1078,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
 		goto out;
 
 	page = pmd_page(*pmd);
-	VM_BUG_ON_PAGE(!PageHead(page), page);
+	VM_BUG_ON_PAGE(!PageHead(page) && !is_zone_device_page(page), page);
 	if (flags & FOLL_TOUCH)
 		touch_pmd(vma, addr, pmd);
 	if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
@@ -1116,7 +1116,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
 	}
 skip_mlock:
 	page += (addr & ~HPAGE_PMD_MASK) >> PAGE_SHIFT;
-	VM_BUG_ON_PAGE(!PageCompound(page), page);
+	VM_BUG_ON_PAGE(!PageCompound(page) && !is_zone_device_page(page), page);
 	if (flags & FOLL_GET)
 		get_page(page);
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/5] mm: fix show_smap() for zone_device-pmd ranges

[Andrew Morton]

On Tue, 06 Sep 2016 09:49:36 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> Attempting to dump /proc/<pid>/smaps for a process with pmd dax mappings
> currently results in the following VM_BUG_ONs:
> 
>  kernel BUG at mm/huge_memory.c:1105!
>  task: ffff88045f16b140 task.stack: ffff88045be14000
>  RIP: 0010:[<ffffffff81268f9b>]  [<ffffffff81268f9b>] follow_trans_huge_pmd+0x2cb/0x340
>  [..]
>  Call Trace:
>   [<ffffffff81306030>] smaps_pte_range+0xa0/0x4b0
>   [<ffffffff814c2755>] ? vsnprintf+0x255/0x4c0
>   [<ffffffff8123c46e>] __walk_page_range+0x1fe/0x4d0
>   [<ffffffff8123c8a2>] walk_page_vma+0x62/0x80
>   [<ffffffff81307656>] show_smap+0xa6/0x2b0
> 
>  kernel BUG at fs/proc/task_mmu.c:585!
>  RIP: 0010:[<ffffffff81306469>]  [<ffffffff81306469>] smaps_pte_range+0x499/0x4b0
>  Call Trace:
>   [<ffffffff814c2795>] ? vsnprintf+0x255/0x4c0
>   [<ffffffff8123c46e>] __walk_page_range+0x1fe/0x4d0
>   [<ffffffff8123c8a2>] walk_page_vma+0x62/0x80
>   [<ffffffff81307696>] show_smap+0xa6/0x2b0
> 
> These locations are sanity checking page flags that must be set for an
> anonymous transparent huge page, but are not set for the zone_device
> pages associated with dax mappings.

Acked-by: Andrew Morton <akpm@linux-foundation.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Dan Williams]

track_pfn_insert() is marking dax mappings as uncacheable.

It is used to keep mappings attributes consistent across a remapped range.
However, since dax regions are never registered via track_pfn_remap(), the
caching mode lookup for dax pfns always returns _PAGE_CACHE_MODE_UC.  We do not
use track_pfn_insert() in the dax-pte path, and we always want to use the
pgprot of the vma itself, so drop this call.

Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Matthew Wilcox <mawilcox@microsoft.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
Reported-by: Toshi Kani <toshi.kani@hpe.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 mm/huge_memory.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a6abd76baa72..338eff05c77a 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -676,8 +676,6 @@ int vmf_insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return VM_FAULT_SIGBUS;
-	if (track_pfn_insert(vma, &pgprot, pfn))
-		return VM_FAULT_SIGBUS;
 	insert_pfn_pmd(vma, addr, pmd, pfn, pgprot, write);
 	return VM_FAULT_NOPAGE;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Matthew Wilcox]

I have no objection to this patch going in for now.

Longer term, surely we want to track what mode the PFNs are mapped in?  There are various bizarre suppositions out there about how persistent memory should be mapped, and it's probably better if the kernel ignores what the user specifies and keeps everything sane.  I've read the dire warnings in the Intel architecture manual and I have no desire to deal with the inevitable bug reports on some hardware I don't own and requires twenty weeks of operation in order to observe the bug.

-----Original Message-----
From: Dan Williams [mailto:dan.j.williams@intel.com] 
Sent: Tuesday, September 6, 2016 12:50 PM
To: linux-nvdimm@lists.01.org
Cc: Toshi Kani <toshi.kani@hpe.com>; Matthew Wilcox <mawilcox@microsoft.com>; Nilesh Choudhury <nilesh.choudhury@oracle.com>; linux-kernel@vger.kernel.org; stable@vger.kernel.org; linux-mm@kvack.org; akpm@linux-foundation.org; Ross Zwisler <ross.zwisler@linux.intel.com>; Kirill A. Shutemov <kirill.shutemov@linux.intel.com>; Kai Zhang <kai.ka.zhang@oracle.com>
Subject: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

track_pfn_insert() is marking dax mappings as uncacheable.

It is used to keep mappings attributes consistent across a remapped range.
However, since dax regions are never registered via track_pfn_remap(), the caching mode lookup for dax pfns always returns _PAGE_CACHE_MODE_UC.  We do not use track_pfn_insert() in the dax-pte path, and we always want to use the pgprot of the vma itself, so drop this call.

Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Matthew Wilcox <mawilcox@microsoft.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
Reported-by: Toshi Kani <toshi.kani@hpe.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 mm/huge_memory.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c index a6abd76baa72..338eff05c77a 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -676,8 +676,6 @@ int vmf_insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return VM_FAULT_SIGBUS;
-	if (track_pfn_insert(vma, &pgprot, pfn))
-		return VM_FAULT_SIGBUS;
 	insert_pfn_pmd(vma, addr, pmd, pfn, pgprot, write);
 	return VM_FAULT_NOPAGE;
 }

Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Dan Williams]

On Tue, Sep 6, 2016 at 10:20 AM, Matthew Wilcox <mawilcox@microsoft.com> wrote:
> I have no objection to this patch going in for now.
>
> Longer term, surely we want to track what mode the PFNs are mapped in?  There are various bizarre suppositions out there about how persistent memory should be mapped, and it's probably better if the kernel ignores what the user specifies and keeps everything sane.  I've read the dire warnings in the Intel architecture manual and I have no desire to deal with the inevitable bug reports on some hardware I don't own and requires twenty weeks of operation in order to observe the bug.

Is there a way for userspace to establish mappings with different
cache modes, besides via /dev/mem?  That was the motivation for
CONFIG_IO_STRICT_DEVMEM.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Andrew Morton]

On Tue, 06 Sep 2016 09:49:41 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> track_pfn_insert() is marking dax mappings as uncacheable.
> 
> It is used to keep mappings attributes consistent across a remapped range.
> However, since dax regions are never registered via track_pfn_remap(), the
> caching mode lookup for dax pfns always returns _PAGE_CACHE_MODE_UC.  We do not
> use track_pfn_insert() in the dax-pte path, and we always want to use the
> pgprot of the vma itself, so drop this call.
> 
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
> Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
> Reported-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Changelog fails to explain the user-visible effects of the patch.  The
stable maintainer(s) will look at this and wonder "ytf was I sent
this".

After fixing that, 

Acked-by: Andrew Morton <akpm@linux-foundation.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Dan Williams]

On Tue, Sep 6, 2016 at 1:17 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 06 Sep 2016 09:49:41 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
>
>> track_pfn_insert() is marking dax mappings as uncacheable.
>>
>> It is used to keep mappings attributes consistent across a remapped range.
>> However, since dax regions are never registered via track_pfn_remap(), the
>> caching mode lookup for dax pfns always returns _PAGE_CACHE_MODE_UC.  We do not
>> use track_pfn_insert() in the dax-pte path, and we always want to use the
>> pgprot of the vma itself, so drop this call.
>>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Cc: Matthew Wilcox <mawilcox@microsoft.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
>> Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
>> Reported-by: Toshi Kani <toshi.kani@hpe.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Changelog fails to explain the user-visible effects of the patch.  The
> stable maintainer(s) will look at this and wonder "ytf was I sent
> this".

True, I'll change it to this:

track_pfn_insert() is marking dax mappings as uncacheable rendering
them impractical for application usage.  DAX-pte mappings are cached
and the goal of establishing DAX-pmd mappings is to attain more
performance, not dramatically less (3 orders of magnitude).

Deleting the call to track_pfn_insert() in vmf_insert_pfn_pmd() lets
the default pgprot (write-back cache enabled) from the vma be used for
the mapping which yields the expected performance improvement over
DAX-pte mappings.

track_pfn_insert() is meant to keep the cache mode for a given range
synchronized across different users of remap_pfn_range() and
vm_insert_pfn_prot().  DAX uses neither of those mapping methods, and
the pmem driver is already marking its memory ranges as write-back
cache enabled.  So, removing the call to track_pfn_insert() leaves the
kernel no worse off than the current situation where a user could map
the range via /dev/mem with an incompatible cache mode compared to the
driver.

> After fixing that,
>
> Acked-by: Andrew Morton <akpm@linux-foundation.org>

Thanks Andrew!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Kani, Toshimitsu]

On Tue, 2016-09-06 at 14:52 -0700, Dan Williams wrote:
> On Tue, Sep 6, 2016 at 1:17 PM, Andrew Morton <akpm@linux-foundation.
> org> wrote:
> > 
> > On Tue, 06 Sep 2016 09:49:41 -0700 Dan Williams <dan.j.williams@int
> > el.com> wrote:
> > 
> > > 
> > > track_pfn_insert() is marking dax mappings as uncacheable.
> > > 
> > > It is used to keep mappings attributes consistent across a
> > > remapped range. However, since dax regions are never registered
> > > via track_pfn_remap(), the caching mode lookup for dax pfns
> > > always returns _PAGE_CACHE_MODE_UC.  We do not use
> > > track_pfn_insert() in the dax-pte path, and we always want to use
> > > the pgprot of the vma itself, so drop this call.
> > > 
> > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > Cc: Matthew Wilcox <mawilcox@microsoft.com>
> > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
> > > Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
> > > Reported-by: Toshi Kani <toshi.kani@hpe.com>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > 
> > Changelog fails to explain the user-visible effects of the
> > patch.  The stable maintainer(s) will look at this and wonder "ytf
> > was I sent this".
> 
> True, I'll change it to this:
> 
> track_pfn_insert() is marking dax mappings as uncacheable rendering
> them impractical for application usage.  DAX-pte mappings are cached
> and the goal of establishing DAX-pmd mappings is to attain more
> performance, not dramatically less (3 orders of magnitude).
> 
> Deleting the call to track_pfn_insert() in vmf_insert_pfn_pmd() lets
> the default pgprot (write-back cache enabled) from the vma be used
> for the mapping which yields the expected performance improvement
> over DAX-pte mappings.
> 
> track_pfn_insert() is meant to keep the cache mode for a given range
> synchronized across different users of remap_pfn_range() and
> vm_insert_pfn_prot().  DAX uses neither of those mapping methods, and
> the pmem driver is already marking its memory ranges as write-back
> cache enabled.  So, removing the call to track_pfn_insert() leaves
> the kernel no worse off than the current situation where a user could
> map the range via /dev/mem with an incompatible cache mode compared
> to the driver.

I think devm_memremap_pages() should call reserve_memtype() on x86 to
keep it consistent with devm_memremap() on this regard.  We may need an
arch stub for reserve_memtype(), though.  Then, track_pfn_insert()
should have no issue in this case.

Thanks,
-Toshi

Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings

[Dan Williams]

On Wed, Sep 7, 2016 at 12:39 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Tue, 2016-09-06 at 14:52 -0700, Dan Williams wrote:
>> On Tue, Sep 6, 2016 at 1:17 PM, Andrew Morton <akpm@linux-foundation.
>> org> wrote:
>> >
>> > On Tue, 06 Sep 2016 09:49:41 -0700 Dan Williams <dan.j.williams@int
>> > el.com> wrote:
>> >
>> > >
>> > > track_pfn_insert() is marking dax mappings as uncacheable.
>> > >
>> > > It is used to keep mappings attributes consistent across a
>> > > remapped range. However, since dax regions are never registered
>> > > via track_pfn_remap(), the caching mode lookup for dax pfns
>> > > always returns _PAGE_CACHE_MODE_UC.  We do not use
>> > > track_pfn_insert() in the dax-pte path, and we always want to use
>> > > the pgprot of the vma itself, so drop this call.
>> > >
>> > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > > Cc: Matthew Wilcox <mawilcox@microsoft.com>
>> > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> > > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > > Cc: Nilesh Choudhury <nilesh.choudhury@oracle.com>
>> > > Reported-by: Kai Zhang <kai.ka.zhang@oracle.com>
>> > > Reported-by: Toshi Kani <toshi.kani@hpe.com>
>> > > Cc: <stable@vger.kernel.org>
>> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> >
>> > Changelog fails to explain the user-visible effects of the
>> > patch.  The stable maintainer(s) will look at this and wonder "ytf
>> > was I sent this".
>>
>> True, I'll change it to this:
>>
>> track_pfn_insert() is marking dax mappings as uncacheable rendering
>> them impractical for application usage.  DAX-pte mappings are cached
>> and the goal of establishing DAX-pmd mappings is to attain more
>> performance, not dramatically less (3 orders of magnitude).
>>
>> Deleting the call to track_pfn_insert() in vmf_insert_pfn_pmd() lets
>> the default pgprot (write-back cache enabled) from the vma be used
>> for the mapping which yields the expected performance improvement
>> over DAX-pte mappings.
>>
>> track_pfn_insert() is meant to keep the cache mode for a given range
>> synchronized across different users of remap_pfn_range() and
>> vm_insert_pfn_prot().  DAX uses neither of those mapping methods, and
>> the pmem driver is already marking its memory ranges as write-back
>> cache enabled.  So, removing the call to track_pfn_insert() leaves
>> the kernel no worse off than the current situation where a user could
>> map the range via /dev/mem with an incompatible cache mode compared
>> to the driver.
>
> I think devm_memremap_pages() should call reserve_memtype() on x86 to
> keep it consistent with devm_memremap() on this regard.  We may need an
> arch stub for reserve_memtype(), though.  Then, track_pfn_insert()
> should have no issue in this case.

Yes, indeed!  In fact I already have that re-write getting 0day
coverage before posting.  It occurred to me while re-writing the
changelog per Andrew's prompting.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 5/5] mm: cleanup pfn_t usage in track_pfn_insert()

[Dan Williams]

Now that track_pfn_insert() is no longer used in the DAX path, it no
longer needs to comprehend pfn_t values.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/mm/pat.c             |    4 ++--
 include/asm-generic/pgtable.h |    4 ++--
 mm/memory.c                   |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index ecb1b69c1651..e8aed3a30e29 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -971,7 +971,7 @@ int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
 }
 
 int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
-		     pfn_t pfn)
+		     unsigned long pfn)
 {
 	enum page_cache_mode pcm;
 
@@ -979,7 +979,7 @@ int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
 		return 0;
 
 	/* Set prot based on lookup */
-	pcm = lookup_memtype(pfn_t_to_phys(pfn));
+	pcm = lookup_memtype(PFN_PHYS(pfn));
 	*prot = __pgprot((pgprot_val(*prot) & (~_PAGE_CACHE_MASK)) |
 			 cachemode2protval(pcm));
 
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index d4458b6dbfb4..f9a4f708227d 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -559,7 +559,7 @@ static inline int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
  * by vm_insert_pfn().
  */
 static inline int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
-				   pfn_t pfn)
+				   unsigned long pfn)
 {
 	return 0;
 }
@@ -594,7 +594,7 @@ extern int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
 			   unsigned long pfn, unsigned long addr,
 			   unsigned long size);
 extern int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
-			    pfn_t pfn);
+			    unsigned long pfn);
 extern int track_pfn_copy(struct vm_area_struct *vma);
 extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
 			unsigned long size);
diff --git a/mm/memory.c b/mm/memory.c
index 83be99d9d8a1..5d4826a28e3f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1637,7 +1637,7 @@ int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
 
 	if (addr < vma->vm_start || addr >= vma->vm_end)
 		return -EFAULT;
-	if (track_pfn_insert(vma, &pgprot, __pfn_to_pfn_t(pfn, PFN_DEV)))
+	if (track_pfn_insert(vma, &pgprot, pfn))
 		return -EINVAL;
 
 	ret = insert_pfn(vma, addr, __pfn_to_pfn_t(pfn, PFN_DEV), pgprot);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: cleanup pfn_t usage in track_pfn_insert()

[Andrew Morton]

On Tue, 06 Sep 2016 09:49:47 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> Now that track_pfn_insert() is no longer used in the DAX path, it no
> longer needs to comprehend pfn_t values.

What's the benefit in this?  A pfn *should* have type pfn_t, shouldn't
it?   Confused.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: cleanup pfn_t usage in track_pfn_insert()

[Dan Williams]

On Tue, Sep 6, 2016 at 1:20 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 06 Sep 2016 09:49:47 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
>
>> Now that track_pfn_insert() is no longer used in the DAX path, it no
>> longer needs to comprehend pfn_t values.
>
> What's the benefit in this?  A pfn *should* have type pfn_t, shouldn't
> it?   Confused.

It should when there's extra information to consider.  I don't mind
leaving it as is, but all the other usages of pfn_t are considering or
passing through the PFN_DEV and PFN_MAP flags.  So, it's a courtesy to
the reader saying "you don't need to worry about pfn_t defined
behavior here, this is just a plain old physical address >>
PAGE_SHIFT"

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: cleanup pfn_t usage in track_pfn_insert()

[Anshuman Khandual]

On 09/06/2016 10:19 PM, Dan Williams wrote:
> Now that track_pfn_insert() is no longer used in the DAX path, it no
> longer needs to comprehend pfn_t values.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  arch/x86/mm/pat.c             |    4 ++--
>  include/asm-generic/pgtable.h |    4 ++--
>  mm/memory.c                   |    2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)

A small nit. Should not the arch/x86/mm/pat.c changes be separated out
into a different patch ? Kind of faced little bit problem separating out
generic core mm changes to that of arch specific mm changes when going
through the commits in retrospect.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: cleanup pfn_t usage in track_pfn_insert()

[Dan Williams]

On Tue, Sep 6, 2016 at 10:12 PM, Anshuman Khandual
<khandual@linux.vnet.ibm.com> wrote:
> On 09/06/2016 10:19 PM, Dan Williams wrote:
>> Now that track_pfn_insert() is no longer used in the DAX path, it no
>> longer needs to comprehend pfn_t values.
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  arch/x86/mm/pat.c             |    4 ++--
>>  include/asm-generic/pgtable.h |    4 ++--
>>  mm/memory.c                   |    2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> A small nit. Should not the arch/x86/mm/pat.c changes be separated out
> into a different patch ? Kind of faced little bit problem separating out
> generic core mm changes to that of arch specific mm changes when going
> through the commits in retrospect.

I'm going to drop this change.  Leaving it as is does no harm, and
users of pfn_t are likely to grow over time.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>