diff mbox series

[V5,4/4] kvm: add a check if pfn is from NVDIMM pmem.

Message ID 4e8c2e0facd46cfaf4ab79e19c9115958ab6f218.1536342881.git.yi.z.zhang@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Fix kvm misconceives NVDIMM pages as reserved mmio | expand

Commit Message

Zhang, Yi Sept. 7, 2018, 6:04 p.m. UTC
For device specific memory space, when we move these area of pfn to
memory zone, we will set the page reserved flag at that time, some of
these reserved for device mmio, and some of these are not, such as
NVDIMM pmem.

Now, we map these dev_dax or fs_dax pages to kvm for DIMM/NVDIMM
backend, since these pages are reserved, the check of
kvm_is_reserved_pfn() misconceives those pages as MMIO. Therefor, we
introduce 2 page map types, MEMORY_DEVICE_FS_DAX/MEMORY_DEVICE_DEV_DAX,
to identify these pages are from NVDIMM pmem and let kvm treat these
as normal pages.

Without this patch, many operations will be missed due to this
mistreatment to pmem pages, for example, a page may not have chance to
be unpinned for KVM guest(in kvm_release_pfn_clean), not able to be
marked as dirty/accessed(in kvm_set_pfn_dirty/accessed) etc.

Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
Acked-by: Pankaj Gupta <pagupta@redhat.com>
---
 virt/kvm/kvm_main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Dan Williams Sept. 19, 2018, 2:53 a.m. UTC | #1
On Fri, Sep 7, 2018 at 2:25 AM Zhang Yi <yi.z.zhang@linux.intel.com> wrote:
>
> For device specific memory space, when we move these area of pfn to
> memory zone, we will set the page reserved flag at that time, some of
> these reserved for device mmio, and some of these are not, such as
> NVDIMM pmem.
>
> Now, we map these dev_dax or fs_dax pages to kvm for DIMM/NVDIMM
> backend, since these pages are reserved, the check of
> kvm_is_reserved_pfn() misconceives those pages as MMIO. Therefor, we
> introduce 2 page map types, MEMORY_DEVICE_FS_DAX/MEMORY_DEVICE_DEV_DAX,
> to identify these pages are from NVDIMM pmem and let kvm treat these
> as normal pages.
>
> Without this patch, many operations will be missed due to this
> mistreatment to pmem pages, for example, a page may not have chance to
> be unpinned for KVM guest(in kvm_release_pfn_clean), not able to be
> marked as dirty/accessed(in kvm_set_pfn_dirty/accessed) etc.
>
> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> Acked-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c44c406..9c49634 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -147,8 +147,20 @@ __weak void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
>
>  bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
>  {
> -       if (pfn_valid(pfn))
> -               return PageReserved(pfn_to_page(pfn));
> +       struct page *page;
> +
> +       if (pfn_valid(pfn)) {
> +               page = pfn_to_page(pfn);
> +
> +               /*
> +                * For device specific memory space, there is a case
> +                * which we need pass MEMORY_DEVICE_FS[DEV]_DAX pages
> +                * to kvm, these pages marked reserved flag as it is a
> +                * zone device memory, we need to identify these pages
> +                * and let kvm treat these as normal pages
> +                */
> +               return PageReserved(page) && !is_dax_page(page);

Should we consider just not setting PageReserved for
devm_memremap_pages()? Perhaps kvm is not be the only component making
these assumptions about this flag?

Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?

This has less to do with "dax" pages and more to do with
devm_memremap_pages() established ranges. P2PDMA is another producer
of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
used in these kvm paths then I think this points to consider clearing
the Reserved flag.

That said I haven't audited all the locations that test PageReserved().

Sorry for not responding sooner I was on extended leave.
David Hildenbrand Sept. 19, 2018, 7:20 a.m. UTC | #2
Am 19.09.18 um 04:53 schrieb Dan Williams:
> On Fri, Sep 7, 2018 at 2:25 AM Zhang Yi <yi.z.zhang@linux.intel.com> wrote:
>>
>> For device specific memory space, when we move these area of pfn to
>> memory zone, we will set the page reserved flag at that time, some of
>> these reserved for device mmio, and some of these are not, such as
>> NVDIMM pmem.
>>
>> Now, we map these dev_dax or fs_dax pages to kvm for DIMM/NVDIMM
>> backend, since these pages are reserved, the check of
>> kvm_is_reserved_pfn() misconceives those pages as MMIO. Therefor, we
>> introduce 2 page map types, MEMORY_DEVICE_FS_DAX/MEMORY_DEVICE_DEV_DAX,
>> to identify these pages are from NVDIMM pmem and let kvm treat these
>> as normal pages.
>>
>> Without this patch, many operations will be missed due to this
>> mistreatment to pmem pages, for example, a page may not have chance to
>> be unpinned for KVM guest(in kvm_release_pfn_clean), not able to be
>> marked as dirty/accessed(in kvm_set_pfn_dirty/accessed) etc.
>>
>> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
>> Acked-by: Pankaj Gupta <pagupta@redhat.com>
>> ---
>>  virt/kvm/kvm_main.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index c44c406..9c49634 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -147,8 +147,20 @@ __weak void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
>>
>>  bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
>>  {
>> -       if (pfn_valid(pfn))
>> -               return PageReserved(pfn_to_page(pfn));
>> +       struct page *page;
>> +
>> +       if (pfn_valid(pfn)) {
>> +               page = pfn_to_page(pfn);
>> +
>> +               /*
>> +                * For device specific memory space, there is a case
>> +                * which we need pass MEMORY_DEVICE_FS[DEV]_DAX pages
>> +                * to kvm, these pages marked reserved flag as it is a
>> +                * zone device memory, we need to identify these pages
>> +                * and let kvm treat these as normal pages
>> +                */
>> +               return PageReserved(page) && !is_dax_page(page);
> 
> Should we consider just not setting PageReserved for
> devm_memremap_pages()? Perhaps kvm is not be the only component making
> these assumptions about this flag?

I was asking the exact same question in v3 or so.

I was recently going through all PageReserved users, trying to clean up
and document how it is used.

PG_reserved used to be a marker "not available for the page allocator".
This is only partially true and not really helpful I think. My current
understanding:

"
PG_reserved is set for special pages, struct pages of such pages should
in general not be touched except by their owner. Pages marked as
reserved include:
- Kernel image (including vDSO) and similar (e.g. BIOS, initrd)
- Pages allocated early during boot (bootmem, memblock)
- Zero pages
- Pages that have been associated with a zone but were not onlined
  (e.g. NVDIMM/pmem, online_page_callback used by XEN)
- Pages to exclude from the hibernation image (e.g. loaded kexec images)
- MCA (memory error) pages on ia64
- Offline pages
Some architectures don't allow to ioremap RAM pages that are not marked
as reserved. Allocated pages might have to be set reserved to allow for
that - if there is a good reason to enforce this. Consequently,
PG_reserved part of a user space table might be the indicator for the
zero page, pmem or MMIO pages.
"

Swapping code does not care about PageReserved at all as far as I
remember. This seems to be fine as it only looks at the way pages have
been mapped into user space.

I don't really see a good reason to set pmem pages as reserved. One
question would be, how/if to exclude them from the hibernation image.
But that could also be solved differently (we would have to double check
how they are handled in hibernation code).


A similar user of PageReserved to look at is:

drivers/vfio/vfio_iommu_type1.c:is_invalid_reserved_pfn()

It will not mark pages dirty if they are reserved. Similar to KVM code.

> 
> Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?
> 
> This has less to do with "dax" pages and more to do with
> devm_memremap_pages() established ranges. P2PDMA is another producer
> of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
> used in these kvm paths then I think this points to consider clearing
> the Reserved flag.
> 
> That said I haven't audited all the locations that test PageReserved().
> 
> Sorry for not responding sooner I was on extended leave.
>
Dan Williams Sept. 20, 2018, 9:19 p.m. UTC | #3
On Thu, Sep 20, 2018 at 7:11 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>
> On 2018-09-19 at 09:20:25 +0200, David Hildenbrand wrote:
> > Am 19.09.18 um 04:53 schrieb Dan Williams:
> > >
> > > Should we consider just not setting PageReserved for
> > > devm_memremap_pages()? Perhaps kvm is not be the only component making
> > > these assumptions about this flag?
> >
> > I was asking the exact same question in v3 or so.
> >
> > I was recently going through all PageReserved users, trying to clean up
> > and document how it is used.
> >
> > PG_reserved used to be a marker "not available for the page allocator".
> > This is only partially true and not really helpful I think. My current
> > understanding:
> >
> > "
> > PG_reserved is set for special pages, struct pages of such pages should
> > in general not be touched except by their owner. Pages marked as
> > reserved include:
> > - Kernel image (including vDSO) and similar (e.g. BIOS, initrd)
> > - Pages allocated early during boot (bootmem, memblock)
> > - Zero pages
> > - Pages that have been associated with a zone but were not onlined
> >   (e.g. NVDIMM/pmem, online_page_callback used by XEN)
> > - Pages to exclude from the hibernation image (e.g. loaded kexec images)
> > - MCA (memory error) pages on ia64
> > - Offline pages
> > Some architectures don't allow to ioremap RAM pages that are not marked
> > as reserved. Allocated pages might have to be set reserved to allow for
> > that - if there is a good reason to enforce this. Consequently,
> > PG_reserved part of a user space table might be the indicator for the
> > zero page, pmem or MMIO pages.
> > "
> >
> > Swapping code does not care about PageReserved at all as far as I
> > remember. This seems to be fine as it only looks at the way pages have
> > been mapped into user space.
> >
> > I don't really see a good reason to set pmem pages as reserved. One
> > question would be, how/if to exclude them from the hibernation image.
> > But that could also be solved differently (we would have to double check
> > how they are handled in hibernation code).
> >
> >
> > A similar user of PageReserved to look at is:
> >
> > drivers/vfio/vfio_iommu_type1.c:is_invalid_reserved_pfn()
> >
> > It will not mark pages dirty if they are reserved. Similar to KVM code.
> Yes, kvm is not the only one user of the dax reserved page.
> >
> > >
> > > Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?
> > >
> > > This has less to do with "dax" pages and more to do with
> > > devm_memremap_pages() established ranges. P2PDMA is another producer
> > > of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
> > > used in these kvm paths then I think this points to consider clearing
> > > the Reserved flag.
>
> Thanks Dan/David's comments.
> for MEMORY_DEVICE_PUBLIC memory, since host driver could manager the
> memory resource to share to guest, Jerome says we could ignore it at
> this time.
>
> And p2pmem, it seems mapped in a PCI bar space which should most likely
> a mmio. I think kvm should treated as a reserved page.

Ok, but the question you left unanswered is whether it would be better
for devm_memremap_pages() to clear the PageReserved flag for
MEMORY_DEVICE_{FS,DEV}_DAX rather than introduce a local kvm-only hack
for what looks like a global problem.
Zhang, Yi Sept. 20, 2018, 10:49 p.m. UTC | #4
On 2018-09-19 at 09:20:25 +0200, David Hildenbrand wrote:
> Am 19.09.18 um 04:53 schrieb Dan Williams:
> > 
> > Should we consider just not setting PageReserved for
> > devm_memremap_pages()? Perhaps kvm is not be the only component making
> > these assumptions about this flag?
> 
> I was asking the exact same question in v3 or so.
> 
> I was recently going through all PageReserved users, trying to clean up
> and document how it is used.
> 
> PG_reserved used to be a marker "not available for the page allocator".
> This is only partially true and not really helpful I think. My current
> understanding:
> 
> "
> PG_reserved is set for special pages, struct pages of such pages should
> in general not be touched except by their owner. Pages marked as
> reserved include:
> - Kernel image (including vDSO) and similar (e.g. BIOS, initrd)
> - Pages allocated early during boot (bootmem, memblock)
> - Zero pages
> - Pages that have been associated with a zone but were not onlined
>   (e.g. NVDIMM/pmem, online_page_callback used by XEN)
> - Pages to exclude from the hibernation image (e.g. loaded kexec images)
> - MCA (memory error) pages on ia64
> - Offline pages
> Some architectures don't allow to ioremap RAM pages that are not marked
> as reserved. Allocated pages might have to be set reserved to allow for
> that - if there is a good reason to enforce this. Consequently,
> PG_reserved part of a user space table might be the indicator for the
> zero page, pmem or MMIO pages.
> "
> 
> Swapping code does not care about PageReserved at all as far as I
> remember. This seems to be fine as it only looks at the way pages have
> been mapped into user space.
> 
> I don't really see a good reason to set pmem pages as reserved. One
> question would be, how/if to exclude them from the hibernation image.
> But that could also be solved differently (we would have to double check
> how they are handled in hibernation code).
> 
> 
> A similar user of PageReserved to look at is:
> 
> drivers/vfio/vfio_iommu_type1.c:is_invalid_reserved_pfn()
> 
> It will not mark pages dirty if they are reserved. Similar to KVM code.
Yes, kvm is not the only one user of the dax reserved page. 
> 
> > 
> > Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?
> > 
> > This has less to do with "dax" pages and more to do with
> > devm_memremap_pages() established ranges. P2PDMA is another producer
> > of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
> > used in these kvm paths then I think this points to consider clearing
> > the Reserved flag.

Thanks Dan/David's comments.
for MEMORY_DEVICE_PUBLIC memory, since host driver could manager the
memory resource to share to guest, Jerome says we could ignore it at
this time.

And p2pmem, it seems mapped in a PCI bar space which should most likely
a mmio. I think kvm should treated as a reserved page.
> > 
> > That said I haven't audited all the locations that test PageReserved().
> > 
> > Sorry for not responding sooner I was on extended leave.
> > 
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
David Hildenbrand Sept. 21, 2018, 2:23 p.m. UTC | #5
On 22/09/2018 00:47, Yi Zhang wrote:
> On 2018-09-20 at 14:19:17 -0700, Dan Williams wrote:
>> On Thu, Sep 20, 2018 at 7:11 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>>>
>>> On 2018-09-19 at 09:20:25 +0200, David Hildenbrand wrote:
>>>> Am 19.09.18 um 04:53 schrieb Dan Williams:
>>>>>
>>>>> Should we consider just not setting PageReserved for
>>>>> devm_memremap_pages()? Perhaps kvm is not be the only component making
>>>>> these assumptions about this flag?
>>>>
>>>> I was asking the exact same question in v3 or so.
>>>>
>>>> I was recently going through all PageReserved users, trying to clean up
>>>> and document how it is used.
>>>>
>>>> PG_reserved used to be a marker "not available for the page allocator".
>>>> This is only partially true and not really helpful I think. My current
>>>> understanding:
>>>>
>>>> "
>>>> PG_reserved is set for special pages, struct pages of such pages should
>>>> in general not be touched except by their owner. Pages marked as
>>>> reserved include:
>>>> - Kernel image (including vDSO) and similar (e.g. BIOS, initrd)
>>>> - Pages allocated early during boot (bootmem, memblock)
>>>> - Zero pages
>>>> - Pages that have been associated with a zone but were not onlined
>>>>   (e.g. NVDIMM/pmem, online_page_callback used by XEN)
>>>> - Pages to exclude from the hibernation image (e.g. loaded kexec images)
>>>> - MCA (memory error) pages on ia64
>>>> - Offline pages
>>>> Some architectures don't allow to ioremap RAM pages that are not marked
>>>> as reserved. Allocated pages might have to be set reserved to allow for
>>>> that - if there is a good reason to enforce this. Consequently,
>>>> PG_reserved part of a user space table might be the indicator for the
>>>> zero page, pmem or MMIO pages.
>>>> "
>>>>
>>>> Swapping code does not care about PageReserved at all as far as I
>>>> remember. This seems to be fine as it only looks at the way pages have
>>>> been mapped into user space.
>>>>
>>>> I don't really see a good reason to set pmem pages as reserved. One
>>>> question would be, how/if to exclude them from the hibernation image.
>>>> But that could also be solved differently (we would have to double check
>>>> how they are handled in hibernation code).
>>>>
>>>>
>>>> A similar user of PageReserved to look at is:
>>>>
>>>> drivers/vfio/vfio_iommu_type1.c:is_invalid_reserved_pfn()
>>>>
>>>> It will not mark pages dirty if they are reserved. Similar to KVM code.
>>> Yes, kvm is not the only one user of the dax reserved page.
>>>>
>>>>>
>>>>> Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?
>>>>>
>>>>> This has less to do with "dax" pages and more to do with
>>>>> devm_memremap_pages() established ranges. P2PDMA is another producer
>>>>> of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
>>>>> used in these kvm paths then I think this points to consider clearing
>>>>> the Reserved flag.
>>>
>>> Thanks Dan/David's comments.
>>> for MEMORY_DEVICE_PUBLIC memory, since host driver could manager the
>>> memory resource to share to guest, Jerome says we could ignore it at
>>> this time.
>>>
>>> And p2pmem, it seems mapped in a PCI bar space which should most likely
>>> a mmio. I think kvm should treated as a reserved page.
>>
>> Ok, but the question you left unanswered is whether it would be better
>> for devm_memremap_pages() to clear the PageReserved flag for
>> MEMORY_DEVICE_{FS,DEV}_DAX rather than introduce a local kvm-only hack
>> for what looks like a global problem.
> 
> Remove the PageReserved flag sounds more reasonable. 
> And Could we still have a flag to identify it is a device private memory, or
> where these pages coming from?

We could use a page type for that or what you proposed. (as I said, we
might have to change hibernation code to skip the pages once we drop the
reserved flag).
Dan Williams Sept. 21, 2018, 6:17 p.m. UTC | #6
On Fri, Sep 21, 2018 at 7:24 AM David Hildenbrand <david@redhat.com> wrote:
[..]
> > Remove the PageReserved flag sounds more reasonable.
> > And Could we still have a flag to identify it is a device private memory, or
> > where these pages coming from?
>
> We could use a page type for that or what you proposed. (as I said, we
> might have to change hibernation code to skip the pages once we drop the
> reserved flag).

I think it would be reasonable to reject all ZONE_DEVICE pages in
saveable_page().
David Hildenbrand Sept. 21, 2018, 7:29 p.m. UTC | #7
On 21/09/2018 20:17, Dan Williams wrote:
> On Fri, Sep 21, 2018 at 7:24 AM David Hildenbrand <david@redhat.com> wrote:
> [..]
>>> Remove the PageReserved flag sounds more reasonable.
>>> And Could we still have a flag to identify it is a device private memory, or
>>> where these pages coming from?
>>
>> We could use a page type for that or what you proposed. (as I said, we
>> might have to change hibernation code to skip the pages once we drop the
>> reserved flag).
> 
> I think it would be reasonable to reject all ZONE_DEVICE pages in
> saveable_page().
> 

Indeed, that sounds like the easiest solution - guess that answer was
too easy for me to figure out :) .
Zhang, Yi Sept. 21, 2018, 10:47 p.m. UTC | #8
On 2018-09-20 at 14:19:17 -0700, Dan Williams wrote:
> On Thu, Sep 20, 2018 at 7:11 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
> >
> > On 2018-09-19 at 09:20:25 +0200, David Hildenbrand wrote:
> > > Am 19.09.18 um 04:53 schrieb Dan Williams:
> > > >
> > > > Should we consider just not setting PageReserved for
> > > > devm_memremap_pages()? Perhaps kvm is not be the only component making
> > > > these assumptions about this flag?
> > >
> > > I was asking the exact same question in v3 or so.
> > >
> > > I was recently going through all PageReserved users, trying to clean up
> > > and document how it is used.
> > >
> > > PG_reserved used to be a marker "not available for the page allocator".
> > > This is only partially true and not really helpful I think. My current
> > > understanding:
> > >
> > > "
> > > PG_reserved is set for special pages, struct pages of such pages should
> > > in general not be touched except by their owner. Pages marked as
> > > reserved include:
> > > - Kernel image (including vDSO) and similar (e.g. BIOS, initrd)
> > > - Pages allocated early during boot (bootmem, memblock)
> > > - Zero pages
> > > - Pages that have been associated with a zone but were not onlined
> > >   (e.g. NVDIMM/pmem, online_page_callback used by XEN)
> > > - Pages to exclude from the hibernation image (e.g. loaded kexec images)
> > > - MCA (memory error) pages on ia64
> > > - Offline pages
> > > Some architectures don't allow to ioremap RAM pages that are not marked
> > > as reserved. Allocated pages might have to be set reserved to allow for
> > > that - if there is a good reason to enforce this. Consequently,
> > > PG_reserved part of a user space table might be the indicator for the
> > > zero page, pmem or MMIO pages.
> > > "
> > >
> > > Swapping code does not care about PageReserved at all as far as I
> > > remember. This seems to be fine as it only looks at the way pages have
> > > been mapped into user space.
> > >
> > > I don't really see a good reason to set pmem pages as reserved. One
> > > question would be, how/if to exclude them from the hibernation image.
> > > But that could also be solved differently (we would have to double check
> > > how they are handled in hibernation code).
> > >
> > >
> > > A similar user of PageReserved to look at is:
> > >
> > > drivers/vfio/vfio_iommu_type1.c:is_invalid_reserved_pfn()
> > >
> > > It will not mark pages dirty if they are reserved. Similar to KVM code.
> > Yes, kvm is not the only one user of the dax reserved page.
> > >
> > > >
> > > > Why is MEMORY_DEVICE_PUBLIC memory specifically excluded?
> > > >
> > > > This has less to do with "dax" pages and more to do with
> > > > devm_memremap_pages() established ranges. P2PDMA is another producer
> > > > of these pages. If either MEMORY_DEVICE_PUBLIC or P2PDMA pages can be
> > > > used in these kvm paths then I think this points to consider clearing
> > > > the Reserved flag.
> >
> > Thanks Dan/David's comments.
> > for MEMORY_DEVICE_PUBLIC memory, since host driver could manager the
> > memory resource to share to guest, Jerome says we could ignore it at
> > this time.
> >
> > And p2pmem, it seems mapped in a PCI bar space which should most likely
> > a mmio. I think kvm should treated as a reserved page.
> 
> Ok, but the question you left unanswered is whether it would be better
> for devm_memremap_pages() to clear the PageReserved flag for
> MEMORY_DEVICE_{FS,DEV}_DAX rather than introduce a local kvm-only hack
> for what looks like a global problem.

Remove the PageReserved flag sounds more reasonable. 
And Could we still have a flag to identify it is a device private memory, or
where these pages coming from?
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
Barret Rhoden Oct. 19, 2018, 4:33 p.m. UTC | #9
On 2018-09-21 at 21:29 David Hildenbrand <david@redhat.com> wrote:
> On 21/09/2018 20:17, Dan Williams wrote:
> > On Fri, Sep 21, 2018 at 7:24 AM David Hildenbrand <david@redhat.com> wrote:
> > [..]  
> >>> Remove the PageReserved flag sounds more reasonable.
> >>> And Could we still have a flag to identify it is a device private memory, or
> >>> where these pages coming from?  
> >>
> >> We could use a page type for that or what you proposed. (as I said, we
> >> might have to change hibernation code to skip the pages once we drop the
> >> reserved flag).  
> > 
> > I think it would be reasonable to reject all ZONE_DEVICE pages in
> > saveable_page().
> >   
> 
> Indeed, that sounds like the easiest solution - guess that answer was
> too easy for me to figure out :) .
> 

Just to follow-up, is the plan to clear PageReserved for nvdimm pages
instead of the approach taken in this patch set?  Or should we special
case nvdimm/dax pages in kvm_is_reserved_pfn()?

Thanks,

Barret
Zhang, Yi Oct. 22, 2018, 8:47 a.m. UTC | #10
On 2018-10-19 at 12:33:48 -0400, Barret Rhoden wrote:
> On 2018-09-21 at 21:29 David Hildenbrand <david@redhat.com> wrote:
> > On 21/09/2018 20:17, Dan Williams wrote:
> > > On Fri, Sep 21, 2018 at 7:24 AM David Hildenbrand <david@redhat.com> wrote:
> > > [..]  
> > >>> Remove the PageReserved flag sounds more reasonable.
> > >>> And Could we still have a flag to identify it is a device private memory, or
> > >>> where these pages coming from?  
> > >>
> > >> We could use a page type for that or what you proposed. (as I said, we
> > >> might have to change hibernation code to skip the pages once we drop the
> > >> reserved flag).  
> > > 
> > > I think it would be reasonable to reject all ZONE_DEVICE pages in
> > > saveable_page().
> > >   
> > 
> > Indeed, that sounds like the easiest solution - guess that answer was
> > too easy for me to figure out :) .
> > 
> 
> Just to follow-up, is the plan to clear PageReserved for nvdimm pages
> instead of the approach taken in this patch set?  Or should we special
> case nvdimm/dax pages in kvm_is_reserved_pfn()?
Yes, we are going to remove the PageReserved flag for nvdimm pages.
Added Alex, attached the patch-set.
> 
> Thanks,
> 
> Barret
> 
> 
>
This patch modifies the set_page_links function to include the setting of
the reserved flag via a simple AND and OR operation. The motivation for
this is the fact that the existing __set_bit call still seems to have
effects on performance as replacing the call with the AND and OR can reduce
initialization time.

Looking over the assembly code before and after the change the main
difference between the two is that the reserved bit is stored in a value
that is generated outside of the main initialization loop and is then
written with the other flags field values in one write to the page->flags
value. Previously the generated value was written and then then a btsq
instruction was issued.

On my x86_64 test system with 3TB of persistent memory per node I saw the
persistent memory initialization time on average drop from 23.49s to
19.12s per node.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/linux/mm.h |    9 ++++++++-
 mm/page_alloc.c    |   29 +++++++++++++++++++----------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e2c9631af05..14d06d7d2986 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1171,11 +1171,18 @@ static inline void set_page_node(struct page *page, unsigned long node)
 	page->flags |= (node & NODES_MASK) << NODES_PGSHIFT;
 }
 
+static inline void set_page_reserved(struct page *page, bool reserved)
+{
+	page->flags &= ~(1ul << PG_reserved);
+	page->flags |= (unsigned long)(!!reserved) << PG_reserved;
+}
+
 static inline void set_page_links(struct page *page, enum zone_type zone,
-	unsigned long node, unsigned long pfn)
+	unsigned long node, unsigned long pfn, bool reserved)
 {
 	set_page_zone(page, zone);
 	set_page_node(page, node);
+	set_page_reserved(page, reserved);
 #ifdef SECTION_IN_PAGE_FLAGS
 	set_page_section(page, pfn_to_section_nr(pfn));
 #endif
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a0b81e0bef03..e7fee7a5f8a3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1179,7 +1179,7 @@ static void __meminit __init_single_page(struct page *page, unsigned long pfn,
 				unsigned long zone, int nid)
 {
 	mm_zero_struct_page(page);
-	set_page_links(page, zone, nid, pfn);
+	set_page_links(page, zone, nid, pfn, false);
 	init_page_count(page);
 	page_mapcount_reset(page);
 	page_cpupid_reset_last(page);
@@ -1195,7 +1195,8 @@ static void __meminit __init_single_page(struct page *page, unsigned long pfn,
 static void __meminit __init_pageblock(unsigned long start_pfn,
 				       unsigned long nr_pages,
 				       unsigned long zone, int nid,
-				       struct dev_pagemap *pgmap)
+				       struct dev_pagemap *pgmap,
+				       bool is_reserved)
 {
 	unsigned long nr_pgmask = pageblock_nr_pages - 1;
 	struct page *start_page = pfn_to_page(start_pfn);
@@ -1231,19 +1232,16 @@ static void __meminit __init_pageblock(unsigned long start_pfn,
 		 * call because of the fact that the pfn number is used to
 		 * get the section_nr and this function should not be
 		 * spanning more than a single section.
+		 *
+		 * We can use a non-atomic operation for setting the
+		 * PG_reserved flag as we are still initializing the pages.
 		 */
-		set_page_links(page, zone, nid, start_pfn);
+		set_page_links(page, zone, nid, start_pfn, is_reserved);
 		init_page_count(page);
 		page_mapcount_reset(page);
 		page_cpupid_reset_last(page);
 
 		/*
-		 * We can use the non-atomic __set_bit operation for setting
-		 * the flag as we are still initializing the pages.
-		 */
-		__SetPageReserved(page);
-
-		/*
 		 * ZONE_DEVICE pages union ->lru with a ->pgmap back
 		 * pointer and hmm_data.  It is a bug if a ZONE_DEVICE
 		 * page is ever freed or placed on a driver-private list.
@@ -5612,7 +5610,18 @@ static void __meminit __memmap_init_hotplug(unsigned long size, int nid,
 		pfn = max(ALIGN_DOWN(pfn - 1, pageblock_nr_pages), start_pfn);
 		stride -= pfn;
 
-		__init_pageblock(pfn, stride, zone, nid, pgmap);
+		/*
+		 * The last argument of __init_pageblock is a boolean
+		 * value indicating if the page will be marked as reserved.
+		 *
+		 * Mark page reserved as it will need to wait for onlining
+		 * phase for it to be fully associated with a zone.
+		 *
+		 * Under certain circumstances ZONE_DEVICE pages may not
+		 * need to be marked as reserved, however there is still
+		 * code that is depending on this being set for now.
+		 */
+		__init_pageblock(pfn, stride, zone, nid, pgmap, true);
 
 		cond_resched();
 	}
diff mbox series

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c44c406..9c49634 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -147,8 +147,20 @@  __weak void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 
 bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
 {
-	if (pfn_valid(pfn))
-		return PageReserved(pfn_to_page(pfn));
+	struct page *page;
+
+	if (pfn_valid(pfn)) {
+		page = pfn_to_page(pfn);
+
+		/*
+		 * For device specific memory space, there is a case
+		 * which we need pass MEMORY_DEVICE_FS[DEV]_DAX pages
+		 * to kvm, these pages marked reserved flag as it is a
+		 * zone device memory, we need to identify these pages
+		 * and let kvm treat these as normal pages
+		 */
+		return PageReserved(page) && !is_dax_page(page);
+	}
 
 	return true;
 }