diff mbox

dax: Fix use of zero page

Message ID 20180517183711.GE26718@bombadil.infradead.org (mailing list archive)
State New, archived
Headers show

Commit Message

Matthew Wilcox May 17, 2018, 6:37 p.m. UTC 
I plucked this patch from my XArray work.  It seems self-contained enough
that it could go into the DAX tree for merging this cycle.

From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
From: Matthew Wilcox <mawilcox@microsoft.com>
Date: Thu, 29 Mar 2018 22:41:18 -0400
Subject: [PATCH] dax: Fix use of zero page

Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
works on MIPS and s390.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/dax.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

Comments

Ross Zwisler May 17, 2018, 7:24 p.m. UTC | #1 
On Thu, May 17, 2018 at 11:37:11AM -0700, Matthew Wilcox wrote:
> 
> I plucked this patch from my XArray work.  It seems self-contained enough
> that it could go into the DAX tree for merging this cycle.
> 
> From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <mawilcox@microsoft.com>
> Date: Thu, 29 Mar 2018 22:41:18 -0400
> Subject: [PATCH] dax: Fix use of zero page
> 
> Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> works on MIPS and s390.
> 
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

Yep, this looks fine.

Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Ross Zwisler May 17, 2018, 7:29 p.m. UTC | #2 
On Thu, May 17, 2018 at 01:24:00PM -0600, Ross Zwisler wrote:
> On Thu, May 17, 2018 at 11:37:11AM -0700, Matthew Wilcox wrote:
> > 
> > I plucked this patch from my XArray work.  It seems self-contained enough
> > that it could go into the DAX tree for merging this cycle.
> > 
> > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > Date: Thu, 29 Mar 2018 22:41:18 -0400
> > Subject: [PATCH] dax: Fix use of zero page
> > 
> > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> > works on MIPS and s390.
> > 
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Yep, this looks fine.
> 
> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Huh, actually, it looks like this relies on patch 01/63 of your full Xarray
series where you s/RADIX_DAX_ZERO_PAGE/DAX_ZERO_PAGE/g.

Ditto for the 2nd patch you sent today.
Dan Williams May 17, 2018, 7:32 p.m. UTC | #3 
On Thu, May 17, 2018 at 11:37 AM, Matthew Wilcox <willy@infradead.org> wrote:
>
> I plucked this patch from my XArray work.  It seems self-contained enough
> that it could go into the DAX tree for merging this cycle.
>
> From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <mawilcox@microsoft.com>
> Date: Thu, 29 Mar 2018 22:41:18 -0400
> Subject: [PATCH] dax: Fix use of zero page
>
> Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> works on MIPS and s390.
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

I'm being thick and / or lazy, what's the user visible effect of this fix?
Matthew Wilcox May 17, 2018, 7:56 p.m. UTC | #4 
On Thu, May 17, 2018 at 12:32:07PM -0700, Dan Williams wrote:
> On Thu, May 17, 2018 at 11:37 AM, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > I plucked this patch from my XArray work.  It seems self-contained enough
> > that it could go into the DAX tree for merging this cycle.
> >
> > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > Date: Thu, 29 Mar 2018 22:41:18 -0400
> > Subject: [PATCH] dax: Fix use of zero page
> >
> > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> > works on MIPS and s390.
> >
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> I'm being thick and / or lazy, what's the user visible effect of this fix?

For s390 it appears to be a performance issue:

Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date:   Mon Oct 25 16:10:07 2010 +0200

    [S390] zero page cache synonyms
    
    If the zero page is mapped to virtual user space addresses that differ
    only in bit 2^12 or 2^13 we get L1 cache synonyms which can affect
    performance. Follow the mips model and use multiple zero pages to avoid
    the synonyms.

MIPS' use of multiple ZERO_PAGEs predates git history.  Given the
history of MIPS' caches behaving in incredibly weird ways, I'd assume
that getting this wrong results in miniature black holes forming and/or
the CPU calculating the largest prime number.
Matthew Wilcox May 17, 2018, 7:57 p.m. UTC | #5 
On Thu, May 17, 2018 at 01:29:10PM -0600, Ross Zwisler wrote:
> On Thu, May 17, 2018 at 01:24:00PM -0600, Ross Zwisler wrote:
> > On Thu, May 17, 2018 at 11:37:11AM -0700, Matthew Wilcox wrote:
> > > 
> > > I plucked this patch from my XArray work.  It seems self-contained enough
> > > that it could go into the DAX tree for merging this cycle.
> > > 
> > > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > Date: Thu, 29 Mar 2018 22:41:18 -0400
> > > Subject: [PATCH] dax: Fix use of zero page
> > > 
> > > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> > > works on MIPS and s390.
> > > 
> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > Yep, this looks fine.
> > 
> > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> Huh, actually, it looks like this relies on patch 01/63 of your full Xarray
> series where you s/RADIX_DAX_ZERO_PAGE/DAX_ZERO_PAGE/g.
> 
> Ditto for the 2nd patch you sent today.

Argh, thanks.  I can respin them against linux-next if you like.
Dan Williams May 17, 2018, 8:03 p.m. UTC | #6 
On Thu, May 17, 2018 at 12:56 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Thu, May 17, 2018 at 12:32:07PM -0700, Dan Williams wrote:
>> On Thu, May 17, 2018 at 11:37 AM, Matthew Wilcox <willy@infradead.org> wrote:
>> >
>> > I plucked this patch from my XArray work.  It seems self-contained enough
>> > that it could go into the DAX tree for merging this cycle.
>> >
>> > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
>> > From: Matthew Wilcox <mawilcox@microsoft.com>
>> > Date: Thu, 29 Mar 2018 22:41:18 -0400
>> > Subject: [PATCH] dax: Fix use of zero page
>> >
>> > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
>> > works on MIPS and s390.
>> >
>> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>>
>> I'm being thick and / or lazy, what's the user visible effect of this fix?
>
> For s390 it appears to be a performance issue:
>
> Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date:   Mon Oct 25 16:10:07 2010 +0200
>
>     [S390] zero page cache synonyms
>
>     If the zero page is mapped to virtual user space addresses that differ
>     only in bit 2^12 or 2^13 we get L1 cache synonyms which can affect
>     performance. Follow the mips model and use multiple zero pages to avoid
>     the synonyms.
>
> MIPS' use of multiple ZERO_PAGEs predates git history.  Given the
> history of MIPS' caches behaving in incredibly weird ways, I'd assume
> that getting this wrong results in miniature black holes forming and/or
> the CPU calculating the largest prime number.

Unless I am missing something I think this sounds like 4.18-rc1
material with a cc: stable. Last I heard no one is really using
dccssblk + dax, and MIPS has no way to describe pmem outside of
memmap= which is only a development tool.
Ross Zwisler May 17, 2018, 8:48 p.m. UTC | #7 
On Thu, May 17, 2018 at 01:03:48PM -0700, Dan Williams wrote:
> On Thu, May 17, 2018 at 12:56 PM, Matthew Wilcox <willy@infradead.org> wrote:
> > On Thu, May 17, 2018 at 12:32:07PM -0700, Dan Williams wrote:
> >> On Thu, May 17, 2018 at 11:37 AM, Matthew Wilcox <willy@infradead.org> wrote:
> >> >
> >> > I plucked this patch from my XArray work.  It seems self-contained enough
> >> > that it could go into the DAX tree for merging this cycle.
> >> >
> >> > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> >> > From: Matthew Wilcox <mawilcox@microsoft.com>
> >> > Date: Thu, 29 Mar 2018 22:41:18 -0400
> >> > Subject: [PATCH] dax: Fix use of zero page
> >> >
> >> > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> >> > works on MIPS and s390.
> >> >
> >> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> >>
> >> I'm being thick and / or lazy, what's the user visible effect of this fix?
> >
> > For s390 it appears to be a performance issue:
> >
> > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Date:   Mon Oct 25 16:10:07 2010 +0200
> >
> >     [S390] zero page cache synonyms
> >
> >     If the zero page is mapped to virtual user space addresses that differ
> >     only in bit 2^12 or 2^13 we get L1 cache synonyms which can affect
> >     performance. Follow the mips model and use multiple zero pages to avoid
> >     the synonyms.
> >
> > MIPS' use of multiple ZERO_PAGEs predates git history.  Given the
> > history of MIPS' caches behaving in incredibly weird ways, I'd assume
> > that getting this wrong results in miniature black holes forming and/or
> > the CPU calculating the largest prime number.
> 
> Unless I am missing something I think this sounds like 4.18-rc1
> material with a cc: stable. Last I heard no one is really using
> dccssblk + dax, and MIPS has no way to describe pmem outside of
> memmap= which is only a development tool.

Yea, I agree that this is v4.18 material.
Ross Zwisler May 17, 2018, 8:48 p.m. UTC | #8 
On Thu, May 17, 2018 at 12:57:39PM -0700, Matthew Wilcox wrote:
> On Thu, May 17, 2018 at 01:29:10PM -0600, Ross Zwisler wrote:
> > On Thu, May 17, 2018 at 01:24:00PM -0600, Ross Zwisler wrote:
> > > On Thu, May 17, 2018 at 11:37:11AM -0700, Matthew Wilcox wrote:
> > > > 
> > > > I plucked this patch from my XArray work.  It seems self-contained enough
> > > > that it could go into the DAX tree for merging this cycle.
> > > > 
> > > > From 8cb56f4ba36af38814ca7b8ba030a66384e59a21 Mon Sep 17 00:00:00 2001
> > > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > > Date: Thu, 29 Mar 2018 22:41:18 -0400
> > > > Subject: [PATCH] dax: Fix use of zero page
> > > > 
> > > > Use my_zero_pfn instead of ZERO_PAGE, and pass the vaddr to it so it
> > > > works on MIPS and s390.
> > > > 
> > > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > Yep, this looks fine.
> > > 
> > > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > 
> > Huh, actually, it looks like this relies on patch 01/63 of your full Xarray
> > series where you s/RADIX_DAX_ZERO_PAGE/DAX_ZERO_PAGE/g.
> > 
> > Ditto for the 2nd patch you sent today.
> 
> Argh, thanks.  I can respin them against linux-next if you like.

Yep, that would be awesome, thanks.
diff mbox

Patch

diff --git a/fs/dax.c b/fs/dax.c
index 6a26626f20f3..f643e8fc34ee 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1006,17 +1006,9 @@  static vm_fault_t dax_load_hole(struct address_space *mapping, void *entry,
 	struct inode *inode = mapping->host;
 	unsigned long vaddr = vmf->address;
 	vm_fault_t ret = VM_FAULT_NOPAGE;
-	struct page *zero_page;
 	void *entry2;
-	pfn_t pfn;
-
-	zero_page = ZERO_PAGE(0);
-	if (unlikely(!zero_page)) {
-		ret = VM_FAULT_OOM;
-		goto out;
-	}
+	pfn_t pfn = pfn_to_pfn_t(my_zero_pfn(vaddr));
 
-	pfn = page_to_pfn_t(zero_page);
 	entry2 = dax_insert_mapping_entry(mapping, vmf, entry, pfn,
 			DAX_ZERO_PAGE, false);
 	if (IS_ERR(entry2)) {