On Mon, Nov 23, 2015 at 12:04 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > The following oops was observed when mmap() with MAP_POPULATE > pre-faulted pmd mappings of a DAX file. follow_trans_huge_pmd() > expects that a target address has a struct page. > > BUG: unable to handle kernel paging request at ffffea0012220000 > follow_trans_huge_pmd+0xba/0x390 > follow_page_mask+0x33d/0x420 > __get_user_pages+0xdc/0x800 > populate_vma_page_range+0xb5/0xe0 > __mm_populate+0xc5/0x150 > vm_mmap_pgoff+0xd5/0xe0 > SyS_mmap_pgoff+0x1c1/0x290 > SyS_mmap+0x1b/0x30 > > Fix it by making the PMD pre-fault handling consistent with PTE. > After pre-faulted in faultin_page(), follow_page_mask() calls > follow_trans_huge_pmd(), which is changed to call follow_pfn_pmd() > for VM_PFNMAP or VM_MIXEDMAP. follow_pfn_pmd() handles FOLL_TOUCH > and returns with -EEXIST. > > Reported-by: Mauricio Porto <mauricio.porto(a)hpe.com&gt; > Signed-off-by: Toshi Kani <toshi.kani(a)hpe.com&gt; > Cc: Andrew Morton <akpm(a)linux-foundation.org&gt; > Cc: Kirill A. Shutemov <kirill.shutemov(a)linux.intel.com&gt; > Cc: Matthew Wilcox <willy(a)linux.intel.com&gt; > Cc: Dan Williams <dan.j.williams(a)intel.com&gt; > Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com&gt; > --- Hey Toshi, I ended up fixing this differently with follow_pmd_devmap() introduced in this series: https://lists.01.org/pipermail/linux-nvdimm/2015-November/003033.html Does the latest libnvdimm-pending branch [1] pass your test case?

On Tue, Dec 1, 2015 at 6:19 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > On Mon, 2015-11-30 at 14:08 -0800, Dan Williams wrote: > > On Mon, Nov 23, 2015 at 12:04 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > > The following oops was observed when mmap() with MAP_POPULATE > > > pre-faulted pmd mappings of a DAX file. follow_trans_huge_pmd() > > > expects that a target address has a struct page. > > > > > > BUG: unable to handle kernel paging request at ffffea0012220000 > > > follow_trans_huge_pmd+0xba/0x390 > > > follow_page_mask+0x33d/0x420 > > > __get_user_pages+0xdc/0x800 > > > populate_vma_page_range+0xb5/0xe0 > > > __mm_populate+0xc5/0x150 > > > vm_mmap_pgoff+0xd5/0xe0 > > > SyS_mmap_pgoff+0x1c1/0x290 > > > SyS_mmap+0x1b/0x30 > > > > > > Fix it by making the PMD pre-fault handling consistent with PTE. > > > After pre-faulted in faultin_page(), follow_page_mask() calls > > > follow_trans_huge_pmd(), which is changed to call follow_pfn_pmd() > > > for VM_PFNMAP or VM_MIXEDMAP. follow_pfn_pmd() handles FOLL_TOUCH > > > and returns with -EEXIST. > > > > > > Reported-by: Mauricio Porto <mauricio.porto(a)hpe.com&gt; > > > Signed-off-by: Toshi Kani <toshi.kani(a)hpe.com&gt; > > > Cc: Andrew Morton <akpm(a)linux-foundation.org&gt; > > > Cc: Kirill A. Shutemov <kirill.shutemov(a)linux.intel.com&gt; > > > Cc: Matthew Wilcox <willy(a)linux.intel.com&gt; > > > Cc: Dan Williams <dan.j.williams(a)intel.com&gt; > > > Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com&gt; > > > --- > > > > Hey Toshi, > > > > I ended up fixing this differently with follow_pmd_devmap() introduced > > in this series: > > > > https://lists.01.org/pipermail/linux-nvdimm/2015-November/003033.html > > > > Does the latest libnvdimm-pending branch [1] pass your test case? > > Hi Dan, > > I ran several test cases, and they all hit the case "pfn not in memmap" in > __dax_pmd_fault() during mmap(MAP_POPULATE). Looking at the dax.pfn, > PFN_DEV is > set but PFN_MAP is not. I have not looked into why, but I thought I let you > know first. I've also seen the test thread got hung up at the end sometime. That PFN_MAP flag will not be set by default for NFIT-defined persistent memory. See pmem_should_map_pages() for pmem namespaces that will have it set by default, currently only e820 type-12 memory ranges. NFIT-defined persistent memory can have a memmap array dynamically allocated by setting up a pfn device (similar to setting up a btt). We don't map it by default because the NFIT may describe hundreds of gigabytes of persistent and the overhead of the memmap may be too large to locate the memmap in ram.

On Wed, Dec 2, 2015 at 9:43 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > Oh, I see. I will setup the memmap array and run the tests again. > > But, why does the PMD mapping depend on the memmap array? We have observed > major performance improvement with PMD. This feature should always be enabled > with DAX regardless of the option to allocate the memmap array. > Several factors drove this decision, I'm open to considering alternatives but here's the reasoning: 1/ DAX pmd mappings caused crashes in the get_user_pages path leading to commit e82c9ed41e8 "dax: disable pmd mappings". The reason pte mappings don't crash and instead trigger -EFAULT is due to the _PAGE_SPECIAL pte bit. 2/ To enable get_user_pages for DAX, in both the page and huge-page case, we need a new pte bit _PAGE_DEVMAP. 3/ Given the pte bits are hard to come I'm assuming we won't get two, i.e. both _PAGE_DEVMAP and a new _PAGE_SPECIAL for pmds. Even if we could get a _PAGE_SPECIAL for pmds I'm not in favor of pursuing it.

On Wed, 2015-12-02 at 10:06 -0800, Dan Williams wrote: > On Wed, Dec 2, 2015 at 9:01 AM, Dan Williams <dan.j.williams(a)intel.com&gt; wrote: > > On Wed, Dec 2, 2015 at 9:43 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > > Oh, I see. I will setup the memmap array and run the tests again. > > > > > > But, why does the PMD mapping depend on the memmap array? We have > > > observed major performance improvement with PMD. This feature should > > > always be enabled with DAX regardless of the option to allocate the memmap > > > array. > > > > > > > Several factors drove this decision, I'm open to considering > > alternatives but here's the reasoning: > > > > 1/ DAX pmd mappings caused crashes in the get_user_pages path leading > > to commit e82c9ed41e8 "dax: disable pmd mappings". The reason pte > > mappings don't crash and instead trigger -EFAULT is due to the > > _PAGE_SPECIAL pte bit. > > > > 2/ To enable get_user_pages for DAX, in both the page and huge-page > > case, we need a new pte bit _PAGE_DEVMAP. > > > > 3/ Given the pte bits are hard to come I'm assuming we won't get two, > > i.e. both _PAGE_DEVMAP and a new _PAGE_SPECIAL for pmds. Even if we > > could get a _PAGE_SPECIAL for pmds I'm not in favor of pursuing it. > > Actually, Dave says they aren't that hard to come by for pmds, so we > could go add _PMD_SPECIAL if we really wanted to support the limited > page-less DAX-pmd case. > > But I'm still of the opinion that we run away from the page-less case > until it can be made a full class citizen with O_DIRECT for pfn > support. I may be missing something, but per vm_normal_page(), I think _PAGE_SPECIAL can be substituted by the following check when we do not have the memmap. if ((vma->vm_flags & VM_PFNMAP) || ((vma->vm_flags & VM_MIXEDMAP) && (!pfn_valid(pfn)))) { This is what I did in this patch for follow_trans_huge_pmd(), although I missed the pfn_valid() check.

On Wed, 2015-12-02 at 11:00 -0800, Dan Williams wrote: > On Wed, Dec 2, 2015 at 11:26 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > On Wed, 2015-12-02 at 10:06 -0800, Dan Williams wrote: > > > On Wed, Dec 2, 2015 at 9:01 AM, Dan Williams <dan.j.williams(a)intel.com&gt; > > > wrote: > > > > On Wed, Dec 2, 2015 at 9:43 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > > > > Oh, I see. I will setup the memmap array and run the tests again. > > > > > > > > > > But, why does the PMD mapping depend on the memmap array? We have > > > > > observed major performance improvement with PMD. This feature > > > > > should always be enabled with DAX regardless of the option to > > > > > allocate the memmap array. > > > > > > > > > > > > > Several factors drove this decision, I'm open to considering > > > > alternatives but here's the reasoning: > > > > > > > > 1/ DAX pmd mappings caused crashes in the get_user_pages path leading > > > > to commit e82c9ed41e8 "dax: disable pmd mappings". The reason pte > > > > mappings don't crash and instead trigger -EFAULT is due to the > > > > _PAGE_SPECIAL pte bit. > > > > > > > > 2/ To enable get_user_pages for DAX, in both the page and huge-page > > > > case, we need a new pte bit _PAGE_DEVMAP. > > > > > > > > 3/ Given the pte bits are hard to come I'm assuming we won't get two, > > > > i.e. both _PAGE_DEVMAP and a new _PAGE_SPECIAL for pmds. Even if we > > > > could get a _PAGE_SPECIAL for pmds I'm not in favor of pursuing it. > > > > > > Actually, Dave says they aren't that hard to come by for pmds, so we > > > could go add _PMD_SPECIAL if we really wanted to support the limited > > > page-less DAX-pmd case. > > > > > > But I'm still of the opinion that we run away from the page-less case > > > until it can be made a full class citizen with O_DIRECT for pfn > > > support. > > > > I may be missing something, but per vm_normal_page(), I think > > _PAGE_SPECIAL can be substituted by the following check when we do not > > have the memmap. > > > > if ((vma->vm_flags & VM_PFNMAP) || > > ((vma->vm_flags & VM_MIXEDMAP) && (!pfn_valid(pfn)))) { > > > > This is what I did in this patch for follow_trans_huge_pmd(), although I > > missed the pfn_valid() check. > > That works for __get_user_pages but not __get_user_pages_fast where we > don't have access to the vma. __get_user_page_fast already refers current->mm, so we should be able to get the vma, and pass it down to gup_pud_range().

On Wed, Dec 2, 2015 at 12:12 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > On Wed, 2015-12-02 at 13:02 -0700, Toshi Kani wrote: > > On Wed, 2015-12-02 at 11:00 -0800, Dan Williams wrote: > > > On Wed, Dec 2, 2015 at 11:26 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > > > On Wed, 2015-12-02 at 10:06 -0800, Dan Williams wrote: > > > > > On Wed, Dec 2, 2015 at 9:01 AM, Dan Williams < > > > > > dan.j.williams(a)intel.com&gt; > > > > > wrote: > > > > > > On Wed, Dec 2, 2015 at 9:43 AM, Toshi Kani <toshi.kani(a)hpe.com&gt; > > > > > > wrote: > > > > > > > Oh, I see. I will setup the memmap array and run the tests > > > > > > > again. > > > > > > > > > > > > > > But, why does the PMD mapping depend on the memmap array? We > > > > > > > have observed major performance improvement with PMD. This > > > > > > > feature should always be enabled with DAX regardless of the > > > > > > > option to allocate the memmap array. > > > > > > > > > > > > > > > > > > > Several factors drove this decision, I'm open to considering > > > > > > alternatives but here's the reasoning: > > > > > > > > > > > > 1/ DAX pmd mappings caused crashes in the get_user_pages path > > > > > > leading to commit e82c9ed41e8 "dax: disable pmd mappings". The > > > > > > reason pte mappings don't crash and instead trigger -EFAULT is due > > > > > > to the _PAGE_SPECIAL pte bit. > > > > > > > > > > > > 2/ To enable get_user_pages for DAX, in both the page and huge > > > > > > -page case, we need a new pte bit _PAGE_DEVMAP. > > > > > > > > > > > > 3/ Given the pte bits are hard to come I'm assuming we won't get > > > > > > two, i.e. both _PAGE_DEVMAP and a new _PAGE_SPECIAL for pmds. > > > > > > Even if we could get a _PAGE_SPECIAL for pmds I'm not in favor of > > > > > > pursuing it. > > > > > > > > > > Actually, Dave says they aren't that hard to come by for pmds, so we > > > > > could go add _PMD_SPECIAL if we really wanted to support the limited > > > > > page-less DAX-pmd case. > > > > > > > > > > But I'm still of the opinion that we run away from the page-less > > > > > case until it can be made a full class citizen with O_DIRECT for pfn > > > > > support. > > > > > > > > I may be missing something, but per vm_normal_page(), I think > > > > _PAGE_SPECIAL can be substituted by the following check when we do not > > > > have the memmap. > > > > > > > > if ((vma->vm_flags & VM_PFNMAP) || > > > > ((vma->vm_flags & VM_MIXEDMAP) && (!pfn_valid(pfn)))) { > > > > > > > > This is what I did in this patch for follow_trans_huge_pmd(), although > > > > I missed the pfn_valid() check. > > > > > > That works for __get_user_pages but not __get_user_pages_fast where we > > > don't have access to the vma. > > > > __get_user_page_fast already refers current->mm, so we should be able to > > get the vma, and pass it down to gup_pud_range(). > > Alternatively, we can obtain the vma from current->mm in gup_huge_pmd() when > the !pfn_valid() condition is met, so that we do not add the code to the > main path of __get_user_pages_fast. The whole point of __get_user_page_fast() is to avoid the overhead of taking the mm semaphore to access the vma. _PAGE_SPECIAL simply tells __get_user_pages_fast that it needs to fallback to the __get_user_pages slow path.

On Wed, Dec 2, 2015 at 1:37 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > On Wed, 2015-12-02 at 11:57 -0800, Dan Williams wrote: [..] > > The whole point of __get_user_page_fast() is to avoid the overhead of > > taking the mm semaphore to access the vma. _PAGE_SPECIAL simply tells > > __get_user_pages_fast that it needs to fallback to the > > __get_user_pages slow path. > > I see. Then, I think gup_huge_pmd() can simply return 0 when !pfn_valid(), > instead of VM_BUG_ON. Is pfn_valid() a reliable check? It seems to be based on a max_pfn per node... what happens when pmem is located below that point. I haven't been able to convince myself that we won't get false positives, but maybe I'm missing something.

On Wed, Dec 2, 2015 at 1:55 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > On Wed, 2015-12-02 at 12:54 -0800, Dan Williams wrote: > > On Wed, Dec 2, 2015 at 1:37 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; > > wrote: > > > On Wed, 2015-12-02 at 11:57 -0800, Dan Williams wrote: > > [..] > > > > The whole point of __get_user_page_fast() is to avoid the > > > > overhead of taking the mm semaphore to access the vma. > > > > _PAGE_SPECIAL simply tells > > > > __get_user_pages_fast that it needs to fallback to the > > > > __get_user_pages slow path. > > > > > > I see. Then, I think gup_huge_pmd() can simply return 0 when > > > !pfn_valid(), instead of VM_BUG_ON. > > > > Is pfn_valid() a reliable check? It seems to be based on a max_pfn > > per node... what happens when pmem is located below that point. I > > haven't been able to convince myself that we won't get false > > positives, but maybe I'm missing something. > > I believe we use the version of pfn_valid() in linux/mmzone.h. Talking this over with Dave we came to the conclusion that it would be safer to be explicit about the pmd not being mapped. He points out that unless a platform can guarantee that persistent memory is always section aligned we might get false positive pfn_valid() indications. Given the get_user_pages_fast() path is arch specific we can simply have an arch specific pmd bit and not worry about generically enabling a "pmd special" bit for now.

On 12/02/2015 12:54 PM, Dan Williams wrote: > On Wed, Dec 2, 2015 at 1:37 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: >> > On Wed, 2015-12-02 at 11:57 -0800, Dan Williams wrote: > [..] >>> >> The whole point of __get_user_page_fast() is to avoid the overhead of >>> >> taking the mm semaphore to access the vma. _PAGE_SPECIAL simply tells >>> >> __get_user_pages_fast that it needs to fallback to the >>> >> __get_user_pages slow path. >> > >> > I see. Then, I think gup_huge_pmd() can simply return 0 when !pfn_valid(), >> > instead of VM_BUG_ON. > Is pfn_valid() a reliable check? It seems to be based on a max_pfn > per node... what happens when pmem is located below that point. I > haven't been able to convince myself that we won't get false > positives, but maybe I'm missing something. With sparsemem at least, it makes sure that you're looking at a valid _section_. See the pfn_valid() at ~include/linux/mmzone.h:1222.

On Tue, 2015-12-01 at 19:45 -0800, Dan Williams wrote: > On Tue, Dec 1, 2015 at 6:19 PM, Toshi Kani <toshi.kani(a)hpe.com&gt; wrote: > > On Mon, 2015-11-30 at 14:08 -0800, Dan Williams wrote:

> > > > > > Hey Toshi, > > > > > > I ended up fixing this differently with follow_pmd_devmap() introduced > > > in this series: > > > > > > https://lists.01.org/pipermail/linux-nvdimm/2015-November/003033.html > > > > > > Does the latest libnvdimm-pending branch [1] pass your test case? > > > > Hi Dan, > > > > I ran several test cases, and they all hit the case "pfn not in memmap" in > > __dax_pmd_fault() during mmap(MAP_POPULATE). Looking at the dax.pfn, > > PFN_DEV is set but PFN_MAP is not. I have not looked into why, but I > > thought I let you know first. I've also seen the test thread got hung up > > at the end sometime. > > That PFN_MAP flag will not be set by default for NFIT-defined > persistent memory. See pmem_should_map_pages() for pmem namespaces > that will have it set by default, currently only e820 type-12 memory > ranges. > > NFIT-defined persistent memory can have a memmap array dynamically > allocated by setting up a pfn device (similar to setting up a btt). > We don't map it by default because the NFIT may describe hundreds of > gigabytes of persistent and the overhead of the memmap may be too > large to locate the memmap in ram. Oh, I see. I will setup the memmap array and run the tests again.