Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1477637 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] mm: fix cache mode of dax pmd mappings Dan Williams <dan.j.williams@intel.com> - 2016-09-06 19:00 +0200
    Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings Dan Williams <dan.j.williams@intel.com> - 2016-09-06 19:40 +0200
    RE: [PATCH 4/5] mm: fix cache mode of dax pmd mappings Matthew Wilcox <mawilcox@microsoft.com> - 2016-09-06 20:00 +0200
    Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings Andrew Morton <akpm@linux-foundation.org> - 2016-09-06 22:20 +0200
      Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings Dan Williams <dan.j.williams@intel.com> - 2016-09-07 00:00 +0200
        Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-09-07 21:40 +0200
          Re: [PATCH 4/5] mm: fix cache mode of dax pmd mappings Dan Williams <dan.j.williams@intel.com> - 2016-09-07 21:50 +0200

#1477637 — [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;
 }

[toc] | [next] | [standalone]

#1477699

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.

[toc] | [prev] | [next] | [standalone]

#1477707

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;
 }

[toc] | [prev] | [next] | [standalone]

#1477804

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>

[toc] | [prev] | [next] | [standalone]

#1477840

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!

[toc] | [prev] | [next] | [standalone]

#1478578

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

[toc] | [prev] | [next] | [standalone]

#1478580

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web