[v2,0/5] Allocate memmap from hotadded memory
mbox series

Message ID 20190625075227.15193-1-osalvador@suse.de
Headers show
Series
  • Allocate memmap from hotadded memory
Related show

Message

Oscar Salvador June 25, 2019, 7:52 a.m. UTC
Hi,

It has been while since I sent previous version [1].

In this version I added some feedback I got back then, like letting
the caller decide whether he wants allocating per memory block or
per memory range (patch#2), and having the chance to disable vmemmap when
users want to expose all hotpluggable memory to userspace (patch#5).

[Testing]

While I could test last version on powerpc, and Huawei's fellows helped me out
testing it on arm64, this time I could only test it on x86_64.
The codebase is quite the same, so I would not expect surprises.

 - x86_64: small and large memblocks (128MB, 1G and 2G)
 - Kernel module that adds memory spanning multiple memblocks
   and remove that memory in a different granularity.

So far, only acpi memory hotplug uses the new flag.
The other callers can be changed depending on their needs.

Of course, more testing and feedback is appreciated.

[Coverletter]

This is another step to make memory hotplug more usable. The primary
goal of this patchset is to reduce memory overhead of the hot-added
memory (at least for SPARSEMEM_VMEMMAP memory model). The current way we use
to populate memmap (struct page array) has two main drawbacks:

a) it consumes an additional memory until the hotadded memory itself is
   onlined and
b) memmap might end up on a different numa node which is especially true
   for movable_node configuration.

a) it is a problem especially for memory hotplug based memory "ballooning"
   solutions when the delay between physical memory hotplug and the
   onlining can lead to OOM and that led to introduction of hacks like auto
   onlining (see 31bc3858ea3e ("memory-hotplug: add automatic onlining
   policy for the newly added memory")).

b) can have performance drawbacks.

Another minor case is that I have seen hot-add operations failing on archs
because they were running out of order-x pages.
E.g On powerpc, in certain configurations, we use order-8 pages,
and given 64KB base pagesize, that is 16MB.
If we run out of those, we just fail the operation and we cannot add
more memory.
We could fallback to base pages as x86_64 does, but we can do better.

One way to mitigate all these issues is to simply allocate memmap array
(which is the largest memory footprint of the physical memory hotplug)
from the hot-added memory itself. SPARSEMEM_VMEMMAP memory model allows
us to map any pfn range so the memory doesn't need to be online to be
usable for the array. See patch 3 for more details.
This feature is only usable when CONFIG_SPARSEMEM_VMEMMAP is set.

[Overall design]:

Implementation wise we reuse vmem_altmap infrastructure to override
the default allocator used by vmemap_populate. Once the memmap is
allocated we need a way to mark altmap pfns used for the allocation.
If MHP_MEMMAP_{DEVICE,MEMBLOCK} flag was passed, we set up the layout of the
altmap structure at the beginning of __add_pages(), and then we call
mark_vmemmap_pages().

The flags are either MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK, and only differ
in the way they allocate vmemmap pages within the memory blocks.

MHP_MEMMAP_MEMBLOCK:
        - With this flag, we will allocate vmemmap pages in each memory block.
          This means that if we hot-add a range that spans multiple memory blocks,
          we will use the beginning of each memory block for the vmemmap pages.
          This strategy is good for cases where the caller wants the flexiblity
          to hot-remove memory in a different granularity than when it was added.

MHP_MEMMAP_DEVICE:
        - With this flag, we will store all vmemmap pages at the beginning of
          hot-added memory.

So it is a tradeoff of flexiblity vs contigous memory.
More info on the above can be found in patch#2.

Depending on which flag is passed (MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK),
mark_vmemmap_pages() gets called at a different stage.
With MHP_MEMMAP_MEMBLOCK, we call it once we have populated the sections
fitting in a single memblock, while with MHP_MEMMAP_DEVICE we wait until all
sections have been populated.

mark_vmemmap_pages() marks the pages as vmemmap and sets some metadata:

The current layout of the Vmemmap pages are:

        [Head->refcount] : Nr sections used by this altmap
        [Head->private]  : Nr of vmemmap pages
        [Tail->freelist] : Pointer to the head page

This is done to easy the computation we need in some places.
E.g:

Example 1)
We hot-add 1GB on x86_64 (memory block 128MB) using
MHP_MEMMAP_DEVICE:

head->_refcount = 8 sections
head->private = 4096 vmemmap pages
tail's->freelist = head

Example 2)
We hot-add 1GB on x86_64 using MHP_MEMMAP_MEMBLOCK:

[at the beginning of each memblock]
head->_refcount = 1 section
head->private = 512 vmemmap pages
tail's->freelist = head

We have the refcount because when using MHP_MEMMAP_DEVICE, we need to know
how much do we have to defer the call to vmemmap_free().
The thing is that the first pages of the hot-added range are used to create
the memmap mapping, so we cannot remove those first, otherwise we would blow up
when accessing the other pages.

What we do is that since when we hot-remove a memory-range, sections are being
removed sequentially, we wait until we hit the last section, and then we free
the hole range to vmemmap_free backwards.
We know that it is the last section because in every pass we
decrease head->_refcount, and when it reaches 0, we got our last section.

We also have to be careful about those pages during online and offline
operations. They are simply skipped, so online will keep them
reserved and so unusable for any other purpose and offline ignores them
so they do not block the offline operation.

One thing worth mention is that vmemmap pages residing in movable memory is not a
show-stopper for that memory to be offlined/migrated away.
Vmemmap pages are just ignored in that case and they stick around until sections
referred by those vmemmap pages are hot-removed.

[1] https://patchwork.kernel.org/cover/10875017/

Oscar Salvador (5):
  drivers/base/memory: Remove unneeded check in
    remove_memory_block_devices
  mm,memory_hotplug: Introduce MHP_VMEMMAP_FLAGS
  mm,memory_hotplug: Introduce Vmemmap page helpers
  mm,memory_hotplug: allocate memmap from the added memory range for
    sparse-vmemmap
  mm,memory_hotplug: Allow userspace to enable/disable vmemmap

 arch/arm64/mm/mmu.c            |   5 +-
 arch/powerpc/mm/init_64.c      |   7 ++
 arch/s390/mm/init.c            |   6 ++
 arch/x86/mm/init_64.c          |  10 +++
 drivers/acpi/acpi_memhotplug.c |   2 +-
 drivers/base/memory.c          |  41 +++++++++--
 drivers/dax/kmem.c             |   2 +-
 drivers/hv/hv_balloon.c        |   2 +-
 drivers/s390/char/sclp_cmd.c   |   2 +-
 drivers/xen/balloon.c          |   2 +-
 include/linux/memory_hotplug.h |  31 ++++++++-
 include/linux/memremap.h       |   2 +-
 include/linux/page-flags.h     |  34 +++++++++
 mm/compaction.c                |   7 ++
 mm/memory_hotplug.c            | 152 ++++++++++++++++++++++++++++++++++-------
 mm/page_alloc.c                |  22 +++++-
 mm/page_isolation.c            |  14 +++-
 mm/sparse.c                    |  93 +++++++++++++++++++++++++
 mm/util.c                      |   2 +
 19 files changed, 394 insertions(+), 42 deletions(-)

Comments

David Hildenbrand June 25, 2019, 8:25 a.m. UTC | #1
On 25.06.19 09:52, Oscar Salvador wrote:
> Hi,
> 
> It has been while since I sent previous version [1].
> 
> In this version I added some feedback I got back then, like letting
> the caller decide whether he wants allocating per memory block or
> per memory range (patch#2), and having the chance to disable vmemmap when
> users want to expose all hotpluggable memory to userspace (patch#5).
> 
> [Testing]
> 
> While I could test last version on powerpc, and Huawei's fellows helped me out
> testing it on arm64, this time I could only test it on x86_64.
> The codebase is quite the same, so I would not expect surprises.
> 
>  - x86_64: small and large memblocks (128MB, 1G and 2G)
>  - Kernel module that adds memory spanning multiple memblocks
>    and remove that memory in a different granularity.
> 
> So far, only acpi memory hotplug uses the new flag.
> The other callers can be changed depending on their needs.
> 
> Of course, more testing and feedback is appreciated.
> 
> [Coverletter]
> 
> This is another step to make memory hotplug more usable. The primary
> goal of this patchset is to reduce memory overhead of the hot-added
> memory (at least for SPARSEMEM_VMEMMAP memory model). The current way we use
> to populate memmap (struct page array) has two main drawbacks:

Mental note: How will it be handled if a caller specifies "Allocate
memmap from hotadded memory", but we are running under SPARSEMEM where
we can't do this.

> 
> a) it consumes an additional memory until the hotadded memory itself is
>    onlined and
> b) memmap might end up on a different numa node which is especially true
>    for movable_node configuration.
> 
> a) it is a problem especially for memory hotplug based memory "ballooning"
>    solutions when the delay between physical memory hotplug and the
>    onlining can lead to OOM and that led to introduction of hacks like auto
>    onlining (see 31bc3858ea3e ("memory-hotplug: add automatic onlining
>    policy for the newly added memory")).
> 
> b) can have performance drawbacks.
> 
> Another minor case is that I have seen hot-add operations failing on archs
> because they were running out of order-x pages.
> E.g On powerpc, in certain configurations, we use order-8 pages,
> and given 64KB base pagesize, that is 16MB.
> If we run out of those, we just fail the operation and we cannot add
> more memory.

At least for SPARSEMEM, we fallback to vmalloc() to work around this
issue. I haven't looked into the populate_section_memmap() internals
yet. Can you point me at the code that performs this allocation?

> We could fallback to base pages as x86_64 does, but we can do better.
> 
> One way to mitigate all these issues is to simply allocate memmap array
> (which is the largest memory footprint of the physical memory hotplug)
> from the hot-added memory itself. SPARSEMEM_VMEMMAP memory model allows
> us to map any pfn range so the memory doesn't need to be online to be
> usable for the array. See patch 3 for more details.
> This feature is only usable when CONFIG_SPARSEMEM_VMEMMAP is set.
> 
> [Overall design]:
> 
> Implementation wise we reuse vmem_altmap infrastructure to override
> the default allocator used by vmemap_populate. Once the memmap is
> allocated we need a way to mark altmap pfns used for the allocation.
> If MHP_MEMMAP_{DEVICE,MEMBLOCK} flag was passed, we set up the layout of the
> altmap structure at the beginning of __add_pages(), and then we call
> mark_vmemmap_pages().
> 
> The flags are either MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK, and only differ
> in the way they allocate vmemmap pages within the memory blocks.
> 
> MHP_MEMMAP_MEMBLOCK:
>         - With this flag, we will allocate vmemmap pages in each memory block.
>           This means that if we hot-add a range that spans multiple memory blocks,
>           we will use the beginning of each memory block for the vmemmap pages.
>           This strategy is good for cases where the caller wants the flexiblity
>           to hot-remove memory in a different granularity than when it was added.
> 
> MHP_MEMMAP_DEVICE:
>         - With this flag, we will store all vmemmap pages at the beginning of
>           hot-added memory.
> 
> So it is a tradeoff of flexiblity vs contigous memory.
> More info on the above can be found in patch#2.
> 
> Depending on which flag is passed (MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK),
> mark_vmemmap_pages() gets called at a different stage.
> With MHP_MEMMAP_MEMBLOCK, we call it once we have populated the sections
> fitting in a single memblock, while with MHP_MEMMAP_DEVICE we wait until all
> sections have been populated.
> 
> mark_vmemmap_pages() marks the pages as vmemmap and sets some metadata:
> 
> The current layout of the Vmemmap pages are:
> 
>         [Head->refcount] : Nr sections used by this altmap
>         [Head->private]  : Nr of vmemmap pages
>         [Tail->freelist] : Pointer to the head page
> 
> This is done to easy the computation we need in some places.
> E.g:
> 
> Example 1)
> We hot-add 1GB on x86_64 (memory block 128MB) using
> MHP_MEMMAP_DEVICE:
> 
> head->_refcount = 8 sections
> head->private = 4096 vmemmap pages
> tail's->freelist = head
> 
> Example 2)
> We hot-add 1GB on x86_64 using MHP_MEMMAP_MEMBLOCK:
> 
> [at the beginning of each memblock]
> head->_refcount = 1 section
> head->private = 512 vmemmap pages
> tail's->freelist = head
> 
> We have the refcount because when using MHP_MEMMAP_DEVICE, we need to know
> how much do we have to defer the call to vmemmap_free().
> The thing is that the first pages of the hot-added range are used to create
> the memmap mapping, so we cannot remove those first, otherwise we would blow up
> when accessing the other pages.

So, assuming we add_memory(1GB, MHP_MEMMAP_DEVICE) and then
remove_memory(128MB) of the added memory, this will work?

add_memory(8GB, MHP_MEMMAP_DEVICE)

For 8GB, we will need exactly 128MB of memmap if I did the math right.
So exactly one section. This section will still be marked as being
online (although not pages on it are actually online)?

> 
> What we do is that since when we hot-remove a memory-range, sections are being
> removed sequentially, we wait until we hit the last section, and then we free
> the hole range to vmemmap_free backwards.
> We know that it is the last section because in every pass we
> decrease head->_refcount, and when it reaches 0, we got our last section.
> 
> We also have to be careful about those pages during online and offline
> operations. They are simply skipped, so online will keep them
> reserved and so unusable for any other purpose and offline ignores them
> so they do not block the offline operation.

I assume that they will still be dumped normally by user space. (as they
are described by a "memory resource" and not PG_Offline)

> 
> One thing worth mention is that vmemmap pages residing in movable memory is not a
> show-stopper for that memory to be offlined/migrated away.
> Vmemmap pages are just ignored in that case and they stick around until sections
> referred by those vmemmap pages are hot-removed.
> 
> [1] https://patchwork.kernel.org/cover/10875017/
> 
> Oscar Salvador (5):
>   drivers/base/memory: Remove unneeded check in
>     remove_memory_block_devices
>   mm,memory_hotplug: Introduce MHP_VMEMMAP_FLAGS
>   mm,memory_hotplug: Introduce Vmemmap page helpers
>   mm,memory_hotplug: allocate memmap from the added memory range for
>     sparse-vmemmap
>   mm,memory_hotplug: Allow userspace to enable/disable vmemmap
> 
>  arch/arm64/mm/mmu.c            |   5 +-
>  arch/powerpc/mm/init_64.c      |   7 ++
>  arch/s390/mm/init.c            |   6 ++
>  arch/x86/mm/init_64.c          |  10 +++
>  drivers/acpi/acpi_memhotplug.c |   2 +-
>  drivers/base/memory.c          |  41 +++++++++--
>  drivers/dax/kmem.c             |   2 +-
>  drivers/hv/hv_balloon.c        |   2 +-
>  drivers/s390/char/sclp_cmd.c   |   2 +-
>  drivers/xen/balloon.c          |   2 +-
>  include/linux/memory_hotplug.h |  31 ++++++++-
>  include/linux/memremap.h       |   2 +-
>  include/linux/page-flags.h     |  34 +++++++++
>  mm/compaction.c                |   7 ++
>  mm/memory_hotplug.c            | 152 ++++++++++++++++++++++++++++++++++-------
>  mm/page_alloc.c                |  22 +++++-
>  mm/page_isolation.c            |  14 +++-
>  mm/sparse.c                    |  93 +++++++++++++++++++++++++
>  mm/util.c                      |   2 +
>  19 files changed, 394 insertions(+), 42 deletions(-)
> 

Thanks for doing this, this will be very helpful :)
David Hildenbrand June 25, 2019, 8:33 a.m. UTC | #2
On 25.06.19 10:25, David Hildenbrand wrote:
> On 25.06.19 09:52, Oscar Salvador wrote:
>> Hi,
>>
>> It has been while since I sent previous version [1].
>>
>> In this version I added some feedback I got back then, like letting
>> the caller decide whether he wants allocating per memory block or
>> per memory range (patch#2), and having the chance to disable vmemmap when
>> users want to expose all hotpluggable memory to userspace (patch#5).
>>
>> [Testing]
>>
>> While I could test last version on powerpc, and Huawei's fellows helped me out
>> testing it on arm64, this time I could only test it on x86_64.
>> The codebase is quite the same, so I would not expect surprises.
>>
>>  - x86_64: small and large memblocks (128MB, 1G and 2G)
>>  - Kernel module that adds memory spanning multiple memblocks
>>    and remove that memory in a different granularity.
>>
>> So far, only acpi memory hotplug uses the new flag.
>> The other callers can be changed depending on their needs.
>>
>> Of course, more testing and feedback is appreciated.
>>
>> [Coverletter]
>>
>> This is another step to make memory hotplug more usable. The primary
>> goal of this patchset is to reduce memory overhead of the hot-added
>> memory (at least for SPARSEMEM_VMEMMAP memory model). The current way we use
>> to populate memmap (struct page array) has two main drawbacks:
> 
> Mental note: How will it be handled if a caller specifies "Allocate
> memmap from hotadded memory", but we are running under SPARSEMEM where
> we can't do this.
> 
>>
>> a) it consumes an additional memory until the hotadded memory itself is
>>    onlined and
>> b) memmap might end up on a different numa node which is especially true
>>    for movable_node configuration.
>>
>> a) it is a problem especially for memory hotplug based memory "ballooning"
>>    solutions when the delay between physical memory hotplug and the
>>    onlining can lead to OOM and that led to introduction of hacks like auto
>>    onlining (see 31bc3858ea3e ("memory-hotplug: add automatic onlining
>>    policy for the newly added memory")).
>>
>> b) can have performance drawbacks.
>>
>> Another minor case is that I have seen hot-add operations failing on archs
>> because they were running out of order-x pages.
>> E.g On powerpc, in certain configurations, we use order-8 pages,
>> and given 64KB base pagesize, that is 16MB.
>> If we run out of those, we just fail the operation and we cannot add
>> more memory.
> 
> At least for SPARSEMEM, we fallback to vmalloc() to work around this
> issue. I haven't looked into the populate_section_memmap() internals
> yet. Can you point me at the code that performs this allocation?
> 
>> We could fallback to base pages as x86_64 does, but we can do better.
>>
>> One way to mitigate all these issues is to simply allocate memmap array
>> (which is the largest memory footprint of the physical memory hotplug)
>> from the hot-added memory itself. SPARSEMEM_VMEMMAP memory model allows
>> us to map any pfn range so the memory doesn't need to be online to be
>> usable for the array. See patch 3 for more details.
>> This feature is only usable when CONFIG_SPARSEMEM_VMEMMAP is set.
>>
>> [Overall design]:
>>
>> Implementation wise we reuse vmem_altmap infrastructure to override
>> the default allocator used by vmemap_populate. Once the memmap is
>> allocated we need a way to mark altmap pfns used for the allocation.
>> If MHP_MEMMAP_{DEVICE,MEMBLOCK} flag was passed, we set up the layout of the
>> altmap structure at the beginning of __add_pages(), and then we call
>> mark_vmemmap_pages().
>>
>> The flags are either MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK, and only differ
>> in the way they allocate vmemmap pages within the memory blocks.
>>
>> MHP_MEMMAP_MEMBLOCK:
>>         - With this flag, we will allocate vmemmap pages in each memory block.
>>           This means that if we hot-add a range that spans multiple memory blocks,
>>           we will use the beginning of each memory block for the vmemmap pages.
>>           This strategy is good for cases where the caller wants the flexiblity
>>           to hot-remove memory in a different granularity than when it was added.
>>
>> MHP_MEMMAP_DEVICE:
>>         - With this flag, we will store all vmemmap pages at the beginning of
>>           hot-added memory.
>>
>> So it is a tradeoff of flexiblity vs contigous memory.
>> More info on the above can be found in patch#2.
>>
>> Depending on which flag is passed (MHP_MEMMAP_DEVICE or MHP_MEMMAP_MEMBLOCK),
>> mark_vmemmap_pages() gets called at a different stage.
>> With MHP_MEMMAP_MEMBLOCK, we call it once we have populated the sections
>> fitting in a single memblock, while with MHP_MEMMAP_DEVICE we wait until all
>> sections have been populated.
>>
>> mark_vmemmap_pages() marks the pages as vmemmap and sets some metadata:
>>
>> The current layout of the Vmemmap pages are:
>>
>>         [Head->refcount] : Nr sections used by this altmap
>>         [Head->private]  : Nr of vmemmap pages
>>         [Tail->freelist] : Pointer to the head page
>>
>> This is done to easy the computation we need in some places.
>> E.g:
>>
>> Example 1)
>> We hot-add 1GB on x86_64 (memory block 128MB) using
>> MHP_MEMMAP_DEVICE:
>>
>> head->_refcount = 8 sections
>> head->private = 4096 vmemmap pages
>> tail's->freelist = head
>>
>> Example 2)
>> We hot-add 1GB on x86_64 using MHP_MEMMAP_MEMBLOCK:
>>
>> [at the beginning of each memblock]
>> head->_refcount = 1 section
>> head->private = 512 vmemmap pages
>> tail's->freelist = head
>>
>> We have the refcount because when using MHP_MEMMAP_DEVICE, we need to know
>> how much do we have to defer the call to vmemmap_free().
>> The thing is that the first pages of the hot-added range are used to create
>> the memmap mapping, so we cannot remove those first, otherwise we would blow up
>> when accessing the other pages.
> 
> So, assuming we add_memory(1GB, MHP_MEMMAP_DEVICE) and then
> remove_memory(128MB) of the added memory, this will work?

Hmm, I guess this won't work - especially when removing the first 128MB
first, where the memmap resides.

Do we need MHP_MEMMAP_DEVICE at this point or could we start with
MHP_MEMMAP_MEMBLOCK? That "smells" like being the easier case.
Oscar Salvador June 26, 2019, 8:03 a.m. UTC | #3
On Tue, Jun 25, 2019 at 10:25:48AM +0200, David Hildenbrand wrote:
> > [Coverletter]
> > 
> > This is another step to make memory hotplug more usable. The primary
> > goal of this patchset is to reduce memory overhead of the hot-added
> > memory (at least for SPARSEMEM_VMEMMAP memory model). The current way we use
> > to populate memmap (struct page array) has two main drawbacks:

First off, thanks for looking into this :-)

> 
> Mental note: How will it be handled if a caller specifies "Allocate
> memmap from hotadded memory", but we are running under SPARSEMEM where
> we can't do this.

In add_memory_resource(), we have a call to mhp_check_correct_flags(), which is
in charge of checking if the flags passed are compliant with our configuration
among other things.
It also checks if both flags were passed (_MEMBLOCK|_DEVICE).

If a) any of the flags were specified and we are not on CONFIG_SPARSEMEM_VMEMMAP,
b) the flags are colliding with each other or c) the flags just do not make sense,
we print out a warning and drop the flags to 0, so we just ignore them.

I just realized that I can adjust the check even more (something for the next
version).

But to answer your question, flags are ignored under !CONFIG_SPARSEMEM_VMEMMAP.

> 
> > 
> > a) it consumes an additional memory until the hotadded memory itself is
> >    onlined and
> > b) memmap might end up on a different numa node which is especially true
> >    for movable_node configuration.
> > 
> > a) it is a problem especially for memory hotplug based memory "ballooning"
> >    solutions when the delay between physical memory hotplug and the
> >    onlining can lead to OOM and that led to introduction of hacks like auto
> >    onlining (see 31bc3858ea3e ("memory-hotplug: add automatic onlining
> >    policy for the newly added memory")).
> > 
> > b) can have performance drawbacks.
> > 
> > Another minor case is that I have seen hot-add operations failing on archs
> > because they were running out of order-x pages.
> > E.g On powerpc, in certain configurations, we use order-8 pages,
> > and given 64KB base pagesize, that is 16MB.
> > If we run out of those, we just fail the operation and we cannot add
> > more memory.
> 
> At least for SPARSEMEM, we fallback to vmalloc() to work around this
> issue. I haven't looked into the populate_section_memmap() internals
> yet. Can you point me at the code that performs this allocation?

Yes, on SPARSEMEM we first try to allocate the pages physical configuous, and
then fallback to vmalloc.
This is because on CONFIG_SPARSEMEM memory model, the translations pfn_to_page/
page_to_pfn do not expect the memory to be contiguous.

But that is not the case on CONFIG_SPARSEMEM_VMEMMAP.
We do expect the memory to be physical contigous there, that is why a simply
pfn_to_page/page_to_pfn is a matter of adding/substracting vmemmap/pfn.

Powerpc code is at:

https://elixir.bootlin.com/linux/v5.2-rc6/source/arch/powerpc/mm/init_64.c#L175



> So, assuming we add_memory(1GB, MHP_MEMMAP_DEVICE) and then
> remove_memory(128MB) of the added memory, this will work?

No, MHP_MEMMAP_DEVICE is meant to be used when hot-adding and hot-removing work
in the same granularity.
This is because all memmap pages will be stored at the beginning of the memory
range.
Allowing hot-removing in a different granularity on MHP_MEMMAP_DEVICE would imply
a lot of extra work.
For example, we would have to parse the vmemmap-head of the hot-removed range,
and punch a hole in there to clear the vmemmap pages, and then be very carefull
when deleting those pagetables.

So I followed Michal's advice, and I decided to let the caller specify if he
either wants to allocate per memory block or per hot-added range(device).
Where per memory block, allows us to do:

add_memory(1GB, MHP_MEMMAP_MEMBLOCK)
remove_memory(128MB)


> add_memory(8GB, MHP_MEMMAP_DEVICE)
> 
> For 8GB, we will need exactly 128MB of memmap if I did the math right.
> So exactly one section. This section will still be marked as being
> online (although not pages on it are actually online)?

Yap, 8GB will fill the first section with vmemmap pages.
It will be marked as online, yes.
This is not to diverge too much from what we have right now, and starting
treat some sections different than others.
E.g: Early sections that are used for memmap pages on early boot stage.

> > 
> > What we do is that since when we hot-remove a memory-range, sections are being
> > removed sequentially, we wait until we hit the last section, and then we free
> > the hole range to vmemmap_free backwards.
> > We know that it is the last section because in every pass we
> > decrease head->_refcount, and when it reaches 0, we got our last section.
> > 
> > We also have to be careful about those pages during online and offline
> > operations. They are simply skipped, so online will keep them
> > reserved and so unusable for any other purpose and offline ignores them
> > so they do not block the offline operation.
> 
> I assume that they will still be dumped normally by user space. (as they
> are described by a "memory resource" and not PG_Offline)

They are PG_Reserved.
Anyway, you mean by crash-tool?
David Hildenbrand June 26, 2019, 8:11 a.m. UTC | #4
On 26.06.19 10:03, Oscar Salvador wrote:
> On Tue, Jun 25, 2019 at 10:25:48AM +0200, David Hildenbrand wrote:
>>> [Coverletter]
>>>
>>> This is another step to make memory hotplug more usable. The primary
>>> goal of this patchset is to reduce memory overhead of the hot-added
>>> memory (at least for SPARSEMEM_VMEMMAP memory model). The current way we use
>>> to populate memmap (struct page array) has two main drawbacks:
> 
> First off, thanks for looking into this :-)

Thanks for working on this ;)

> 
>>
>> Mental note: How will it be handled if a caller specifies "Allocate
>> memmap from hotadded memory", but we are running under SPARSEMEM where
>> we can't do this.
> 
> In add_memory_resource(), we have a call to mhp_check_correct_flags(), which is
> in charge of checking if the flags passed are compliant with our configuration
> among other things.
> It also checks if both flags were passed (_MEMBLOCK|_DEVICE).
> 
> If a) any of the flags were specified and we are not on CONFIG_SPARSEMEM_VMEMMAP,
> b) the flags are colliding with each other or c) the flags just do not make sense,
> we print out a warning and drop the flags to 0, so we just ignore them.
> 
> I just realized that I can adjust the check even more (something for the next
> version).
> 
> But to answer your question, flags are ignored under !CONFIG_SPARSEMEM_VMEMMAP.

So it is indeed a hint only.

> 
>>
>>>
>>> a) it consumes an additional memory until the hotadded memory itself is
>>>    onlined and
>>> b) memmap might end up on a different numa node which is especially true
>>>    for movable_node configuration.
>>>
>>> a) it is a problem especially for memory hotplug based memory "ballooning"
>>>    solutions when the delay between physical memory hotplug and the
>>>    onlining can lead to OOM and that led to introduction of hacks like auto
>>>    onlining (see 31bc3858ea3e ("memory-hotplug: add automatic onlining
>>>    policy for the newly added memory")).
>>>
>>> b) can have performance drawbacks.
>>>
>>> Another minor case is that I have seen hot-add operations failing on archs
>>> because they were running out of order-x pages.
>>> E.g On powerpc, in certain configurations, we use order-8 pages,
>>> and given 64KB base pagesize, that is 16MB.
>>> If we run out of those, we just fail the operation and we cannot add
>>> more memory.
>>
>> At least for SPARSEMEM, we fallback to vmalloc() to work around this
>> issue. I haven't looked into the populate_section_memmap() internals
>> yet. Can you point me at the code that performs this allocation?
> 
> Yes, on SPARSEMEM we first try to allocate the pages physical configuous, and
> then fallback to vmalloc.
> This is because on CONFIG_SPARSEMEM memory model, the translations pfn_to_page/
> page_to_pfn do not expect the memory to be contiguous.
> 
> But that is not the case on CONFIG_SPARSEMEM_VMEMMAP.
> We do expect the memory to be physical contigous there, that is why a simply
> pfn_to_page/page_to_pfn is a matter of adding/substracting vmemmap/pfn.

Yeas, I explored that last week but didn't figure out where the actual
vmmap population code resided - thanks :)

> 
> Powerpc code is at:
> 
> https://elixir.bootlin.com/linux/v5.2-rc6/source/arch/powerpc/mm/init_64.c#L175
> 
> 
> 
>> So, assuming we add_memory(1GB, MHP_MEMMAP_DEVICE) and then
>> remove_memory(128MB) of the added memory, this will work?
> 
> No, MHP_MEMMAP_DEVICE is meant to be used when hot-adding and hot-removing work
> in the same granularity.
> This is because all memmap pages will be stored at the beginning of the memory
> range.
> Allowing hot-removing in a different granularity on MHP_MEMMAP_DEVICE would imply
> a lot of extra work.
> For example, we would have to parse the vmemmap-head of the hot-removed range,
> and punch a hole in there to clear the vmemmap pages, and then be very carefull
> when deleting those pagetables.
> 
> So I followed Michal's advice, and I decided to let the caller specify if he
> either wants to allocate per memory block or per hot-added range(device).
> Where per memory block, allows us to do:
> 
> add_memory(1GB, MHP_MEMMAP_MEMBLOCK)
> remove_memory(128MB)

Back then, I already mentioned that we might have some users that
remove_memory() they never added in a granularity it wasn't added. My
concerns back then were never fully sorted out.

arch/powerpc/platforms/powernv/memtrace.c

- Will remove memory in memory block size chunks it never added
- What if that memory resides on a DIMM added via MHP_MEMMAP_DEVICE?

Will it at least bail out? Or simply break?

IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save to be
introduced.
Oscar Salvador June 26, 2019, 8:15 a.m. UTC | #5
On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
> Back then, I already mentioned that we might have some users that
> remove_memory() they never added in a granularity it wasn't added. My
> concerns back then were never fully sorted out.
> 
> arch/powerpc/platforms/powernv/memtrace.c
> 
> - Will remove memory in memory block size chunks it never added
> - What if that memory resides on a DIMM added via MHP_MEMMAP_DEVICE?
> 
> Will it at least bail out? Or simply break?
> 
> IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save to be
> introduced.

Uhm, I will take a closer look and see if I can clear your concerns.
TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
yet.

I will get back to you once I tried it out.
Oscar Salvador June 26, 2019, 8:27 a.m. UTC | #6
On Wed, Jun 26, 2019 at 10:15:16AM +0200, Oscar Salvador wrote:
> On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
> > Back then, I already mentioned that we might have some users that
> > remove_memory() they never added in a granularity it wasn't added. My
> > concerns back then were never fully sorted out.
> > 
> > arch/powerpc/platforms/powernv/memtrace.c
> > 
> > - Will remove memory in memory block size chunks it never added
> > - What if that memory resides on a DIMM added via MHP_MEMMAP_DEVICE?
> > 
> > Will it at least bail out? Or simply break?
> > 
> > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save to be
> > introduced.
> 
> Uhm, I will take a closer look and see if I can clear your concerns.
> TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
> yet.
> 
> I will get back to you once I tried it out.

On a second though, it would be quite trivial to implement a check in
remove_memory() that does not allow to remove memory used with MHP_MEMMAP_DEVICE
in a different granularity:

+static bool check_vmemmap_granularity(u64 start, u64 size);
+{
+	unsigned long pfn;
+	unsigned int nr_pages;
+	struct page *p;
+
+	pfn = PHYS_PFN(start);
+	p = pfn_to_page(pfn);
+	nr_pages = size >> PAGE_SIZE;
+
+	if (PageVmemmap(p)) {
+		struct page *h = vmemmap_get_head(p);
+		unsigned long sections = (unsigned long)h->private;
+
+		if (sections * PAGES_PER_SECTION > nr_pages)
+			fail;
+	}
+	no_fail;
+}
+		
+
 static int __ref try_remove_memory(int nid, u64 start, u64 size)
 {
 	int rc = 0;
 
 	BUG_ON(check_hotplug_memory_range(start, size));
 
 	mem_hotplug_begin();
 
+	rc = check_vmemmap_granularity(start, size);
+	if (rc)
+		goto done;


The above is quite hacky, but it gives an idea.
I will try the code from arch/powerpc/platforms/powernv/memtrace.c and see how
can I implement a check.
David Hildenbrand June 26, 2019, 8:28 a.m. UTC | #7
On 26.06.19 10:15, Oscar Salvador wrote:
> On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
>> Back then, I already mentioned that we might have some users that
>> remove_memory() they never added in a granularity it wasn't added. My
>> concerns back then were never fully sorted out.
>>
>> arch/powerpc/platforms/powernv/memtrace.c
>>
>> - Will remove memory in memory block size chunks it never added
>> - What if that memory resides on a DIMM added via MHP_MEMMAP_DEVICE?
>>
>> Will it at least bail out? Or simply break?
>>
>> IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save to be
>> introduced.
> 
> Uhm, I will take a closer look and see if I can clear your concerns.
> TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
> yet.
> 
> I will get back to you once I tried it out.
> 

BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
very ugly and dangerous. We should never allow to manually
offline/online pages / hack into memory block states.

What I would want to see here is rather:

1. User space offlines the blocks to be used
2. memtrace installs a hotplug notifier and hinders the blocks it wants
to use from getting onlined.
3. memory is not added/removed/onlined/offlined in memtrace code.

CCing the DEVs.
David Hildenbrand June 26, 2019, 8:37 a.m. UTC | #8
On 26.06.19 10:27, Oscar Salvador wrote:
> On Wed, Jun 26, 2019 at 10:15:16AM +0200, Oscar Salvador wrote:
>> On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
>>> Back then, I already mentioned that we might have some users that
>>> remove_memory() they never added in a granularity it wasn't added. My
>>> concerns back then were never fully sorted out.
>>>
>>> arch/powerpc/platforms/powernv/memtrace.c
>>>
>>> - Will remove memory in memory block size chunks it never added
>>> - What if that memory resides on a DIMM added via MHP_MEMMAP_DEVICE?
>>>
>>> Will it at least bail out? Or simply break?
>>>
>>> IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save to be
>>> introduced.
>>
>> Uhm, I will take a closer look and see if I can clear your concerns.
>> TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
>> yet.
>>
>> I will get back to you once I tried it out.
> 
> On a second though, it would be quite trivial to implement a check in
> remove_memory() that does not allow to remove memory used with MHP_MEMMAP_DEVICE
> in a different granularity:
> 
> +static bool check_vmemmap_granularity(u64 start, u64 size);
> +{
> +	unsigned long pfn;
> +	unsigned int nr_pages;
> +	struct page *p;
> +
> +	pfn = PHYS_PFN(start);
> +	p = pfn_to_page(pfn);
> +	nr_pages = size >> PAGE_SIZE;
> +
> +	if (PageVmemmap(p)) {
> +		struct page *h = vmemmap_get_head(p);
> +		unsigned long sections = (unsigned long)h->private;
> +
> +		if (sections * PAGES_PER_SECTION > nr_pages)
> +			fail;
> +	}
> +	no_fail;
> +}
> +		
> +
>  static int __ref try_remove_memory(int nid, u64 start, u64 size)
>  {
>  	int rc = 0;
>  
>  	BUG_ON(check_hotplug_memory_range(start, size));
>  
>  	mem_hotplug_begin();
>  
> +	rc = check_vmemmap_granularity(start, size);
> +	if (rc)
> +		goto done;
> 
> 
> The above is quite hacky, but it gives an idea.
> I will try the code from arch/powerpc/platforms/powernv/memtrace.c and see how
> can I implement a check.
> 

Yeah, I would consider such a safety check mandatory for MHP_MEMMAP_DEVICE.
Rashmica Gupta July 2, 2019, 6:42 a.m. UTC | #9
Hi David,

Sorry for the late reply.

On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
> On 26.06.19 10:15, Oscar Salvador wrote:
> > On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
> > > Back then, I already mentioned that we might have some users that
> > > remove_memory() they never added in a granularity it wasn't
> > > added. My
> > > concerns back then were never fully sorted out.
> > > 
> > > arch/powerpc/platforms/powernv/memtrace.c
> > > 
> > > - Will remove memory in memory block size chunks it never added
> > > - What if that memory resides on a DIMM added via
> > > MHP_MEMMAP_DEVICE?
> > > 
> > > Will it at least bail out? Or simply break?
> > > 
> > > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save
> > > to be
> > > introduced.
> > 
> > Uhm, I will take a closer look and see if I can clear your
> > concerns.
> > TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
> > yet.
> > 
> > I will get back to you once I tried it out.
> > 
> 
> BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
> very ugly and dangerous.

Yes it would be nice to clean this up.

> We should never allow to manually
> offline/online pages / hack into memory block states.
> 
> What I would want to see here is rather:
> 
> 1. User space offlines the blocks to be used
> 2. memtrace installs a hotplug notifier and hinders the blocks it
> wants
> to use from getting onlined.
> 3. memory is not added/removed/onlined/offlined in memtrace code.
>

I remember looking into doing it a similar way. I can't recall the
details but my issue was probably 'how does userspace indicate to
the kernel that this memory being offlined should be removed'?

I don't know the mm code nor how the notifiers work very well so I
can't quite see how the above would work. I'm assuming memtrace would
register a hotplug notifier and when memory is offlined from userspace,
the callback func in memtrace would be called if the priority was high
enough? But how do we know that the memory being offlined is intended
for usto touch? Is there a way to offline memory from userspace not
using sysfs or have I missed something in the sysfs interface?

On a second read, perhaps you are assuming that memtrace is used after
adding new memory at runtime? If so, that is not the case. If not, then
would you be able to clarify what I'm not seeing?

Thanks.

> CCing the DEVs.
>
Oscar Salvador July 2, 2019, 7:48 a.m. UTC | #10
On Tue, Jul 02, 2019 at 04:42:34PM +1000, Rashmica Gupta wrote:
> Hi David,
> 
> Sorry for the late reply.
> 
> On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
> > On 26.06.19 10:15, Oscar Salvador wrote:
> > > On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
> > > > Back then, I already mentioned that we might have some users that
> > > > remove_memory() they never added in a granularity it wasn't
> > > > added. My
> > > > concerns back then were never fully sorted out.
> > > > 
> > > > arch/powerpc/platforms/powernv/memtrace.c
> > > > 
> > > > - Will remove memory in memory block size chunks it never added
> > > > - What if that memory resides on a DIMM added via
> > > > MHP_MEMMAP_DEVICE?
> > > > 
> > > > Will it at least bail out? Or simply break?
> > > > 
> > > > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save
> > > > to be
> > > > introduced.
> > > 
> > > Uhm, I will take a closer look and see if I can clear your
> > > concerns.
> > > TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
> > > yet.
> > > 
> > > I will get back to you once I tried it out.
> > > 
> > 
> > BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
> > very ugly and dangerous.
> 
> Yes it would be nice to clean this up.
> 
> > We should never allow to manually
> > offline/online pages / hack into memory block states.
> > 
> > What I would want to see here is rather:
> > 
> > 1. User space offlines the blocks to be used
> > 2. memtrace installs a hotplug notifier and hinders the blocks it
> > wants
> > to use from getting onlined.
> > 3. memory is not added/removed/onlined/offlined in memtrace code.
> >
> 
> I remember looking into doing it a similar way. I can't recall the
> details but my issue was probably 'how does userspace indicate to
> the kernel that this memory being offlined should be removed'?
> 
> I don't know the mm code nor how the notifiers work very well so I
> can't quite see how the above would work. I'm assuming memtrace would
> register a hotplug notifier and when memory is offlined from userspace,
> the callback func in memtrace would be called if the priority was high
> enough? But how do we know that the memory being offlined is intended
> for usto touch? Is there a way to offline memory from userspace not
> using sysfs or have I missed something in the sysfs interface?
> 
> On a second read, perhaps you are assuming that memtrace is used after
> adding new memory at runtime? If so, that is not the case. If not, then
> would you be able to clarify what I'm not seeing?

Hi Rashmica,

let us go the easy way here.
Could you please explain:

1) How memtrace works
2) Why it was designed, what is the goal of the interface?
3) When it is supposed to be used?

I have seen a couple of reports in the past from people running memtrace
and failing to do so sometimes, and back then I could not grasp why people
was using it, or under which circumstances was nice to have.
So it would be nice to have a detailed explanation from the person who wrote
it.

Thanks
Rashmica Gupta July 2, 2019, 8:52 a.m. UTC | #11
On Tue, Jul 2, 2019 at 5:48 PM Oscar Salvador <osalvador@suse.de> wrote:

> On Tue, Jul 02, 2019 at 04:42:34PM +1000, Rashmica Gupta wrote:
> > Hi David,
> >
> > Sorry for the late reply.
> >
> > On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
> > > On 26.06.19 10:15, Oscar Salvador wrote:
> > > > On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
> > > > > Back then, I already mentioned that we might have some users that
> > > > > remove_memory() they never added in a granularity it wasn't
> > > > > added. My
> > > > > concerns back then were never fully sorted out.
> > > > >
> > > > > arch/powerpc/platforms/powernv/memtrace.c
> > > > >
> > > > > - Will remove memory in memory block size chunks it never added
> > > > > - What if that memory resides on a DIMM added via
> > > > > MHP_MEMMAP_DEVICE?
> > > > >
> > > > > Will it at least bail out? Or simply break?
> > > > >
> > > > > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save
> > > > > to be
> > > > > introduced.
> > > >
> > > > Uhm, I will take a closer look and see if I can clear your
> > > > concerns.
> > > > TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
> > > > yet.
> > > >
> > > > I will get back to you once I tried it out.
> > > >
> > >
> > > BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
> > > very ugly and dangerous.
> >
> > Yes it would be nice to clean this up.
> >
> > > We should never allow to manually
> > > offline/online pages / hack into memory block states.
> > >
> > > What I would want to see here is rather:
> > >
> > > 1. User space offlines the blocks to be used
> > > 2. memtrace installs a hotplug notifier and hinders the blocks it
> > > wants
> > > to use from getting onlined.
> > > 3. memory is not added/removed/onlined/offlined in memtrace code.
> > >
> >
> > I remember looking into doing it a similar way. I can't recall the
> > details but my issue was probably 'how does userspace indicate to
> > the kernel that this memory being offlined should be removed'?
> >
> > I don't know the mm code nor how the notifiers work very well so I
> > can't quite see how the above would work. I'm assuming memtrace would
> > register a hotplug notifier and when memory is offlined from userspace,
> > the callback func in memtrace would be called if the priority was high
> > enough? But how do we know that the memory being offlined is intended
> > for usto touch? Is there a way to offline memory from userspace not
> > using sysfs or have I missed something in the sysfs interface?
> >
> > On a second read, perhaps you are assuming that memtrace is used after
> > adding new memory at runtime? If so, that is not the case. If not, then
> > would you be able to clarify what I'm not seeing?
>
> Hi Rashmica,
>
> let us go the easy way here.
> Could you please explain:
>
>
Sure!


> 1) How memtrace works
>

 You write the size of the chunk of memory you want into the debugfs file
and memtrace will attempt to find a contiguous section of memory of that
size
that can be offlined. If it finds that, then the memory is removed from the
kernel's mappings. If you want a different size, then you write that to the
debugsfs file and memtrace will re-add the memory it first removed and then
try to offline and remove the a chunk of the new size.



> 2) Why it was designed, what is the goal of the interface?
> 3) When it is supposed to be used?
>
>
There is a hardware debugging facility (htm) on some power chips. To use
this you need a contiguous portion of memory for the output to be dumped
to - and we obviously don't want this memory to be simultaneously used by
the kernel.

At boot time we can portion off a section of memory for this (and not tell
the
kernel about it), but sometimes you want to be able to use the hardware
debugging facilities and you haven't done this and you don't want to reboot
your machine - and memtrace is the solution for this.

If you're curious one tool that uses this debugging facility is here:
https://github.com/open-power/pdbg. Relevant files are libpdbg/htm.c and
src/htm.c.


I have seen a couple of reports in the past from people running memtrace
> and failing to do so sometimes, and back then I could not grasp why people
> was using it, or under which circumstances was nice to have.
> So it would be nice to have a detailed explanation from the person who
> wrote
> it.
>
>
Is that enough detail?


> Thanks
>
> --
> Oscar Salvador
> SUSE L3
>
Rashmica Gupta July 10, 2019, 1:14 a.m. UTC | #12
Woops, looks like my phone doesn't send plain text emails :/

On Tue, Jul 2, 2019 at 6:52 PM Rashmica Gupta <rashmica.g@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 5:48 PM Oscar Salvador <osalvador@suse.de> wrote:
>>
>> On Tue, Jul 02, 2019 at 04:42:34PM +1000, Rashmica Gupta wrote:
>> > Hi David,
>> >
>> > Sorry for the late reply.
>> >
>> > On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
>> > > On 26.06.19 10:15, Oscar Salvador wrote:
>> > > > On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
>> > > > > Back then, I already mentioned that we might have some users that
>> > > > > remove_memory() they never added in a granularity it wasn't
>> > > > > added. My
>> > > > > concerns back then were never fully sorted out.
>> > > > >
>> > > > > arch/powerpc/platforms/powernv/memtrace.c
>> > > > >
>> > > > > - Will remove memory in memory block size chunks it never added
>> > > > > - What if that memory resides on a DIMM added via
>> > > > > MHP_MEMMAP_DEVICE?
>> > > > >
>> > > > > Will it at least bail out? Or simply break?
>> > > > >
>> > > > > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save
>> > > > > to be
>> > > > > introduced.
>> > > >
>> > > > Uhm, I will take a closer look and see if I can clear your
>> > > > concerns.
>> > > > TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
>> > > > yet.
>> > > >
>> > > > I will get back to you once I tried it out.
>> > > >
>> > >
>> > > BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
>> > > very ugly and dangerous.
>> >
>> > Yes it would be nice to clean this up.
>> >
>> > > We should never allow to manually
>> > > offline/online pages / hack into memory block states.
>> > >
>> > > What I would want to see here is rather:
>> > >
>> > > 1. User space offlines the blocks to be used
>> > > 2. memtrace installs a hotplug notifier and hinders the blocks it
>> > > wants
>> > > to use from getting onlined.
>> > > 3. memory is not added/removed/onlined/offlined in memtrace code.
>> > >
>> >
>> > I remember looking into doing it a similar way. I can't recall the
>> > details but my issue was probably 'how does userspace indicate to
>> > the kernel that this memory being offlined should be removed'?
>> >
>> > I don't know the mm code nor how the notifiers work very well so I
>> > can't quite see how the above would work. I'm assuming memtrace would
>> > register a hotplug notifier and when memory is offlined from userspace,
>> > the callback func in memtrace would be called if the priority was high
>> > enough? But how do we know that the memory being offlined is intended
>> > for usto touch? Is there a way to offline memory from userspace not
>> > using sysfs or have I missed something in the sysfs interface?
>> >
>> > On a second read, perhaps you are assuming that memtrace is used after
>> > adding new memory at runtime? If so, that is not the case. If not, then
>> > would you be able to clarify what I'm not seeing?
>>
>> Hi Rashmica,
>>
>> let us go the easy way here.
>> Could you please explain:
>>
>
> Sure!
>
>>
>> 1) How memtrace works
>
>
>  You write the size of the chunk of memory you want into the debugfs file
> and memtrace will attempt to find a contiguous section of memory of that size
> that can be offlined. If it finds that, then the memory is removed from the
> kernel's mappings. If you want a different size, then you write that to the
> debugsfs file and memtrace will re-add the memory it first removed and then
> try to offline and remove the a chunk of the new size.
>
>
>>
>> 2) Why it was designed, what is the goal of the interface?
>> 3) When it is supposed to be used?
>>
>
> There is a hardware debugging facility (htm) on some power chips. To use
> this you need a contiguous portion of memory for the output to be dumped
> to - and we obviously don't want this memory to be simultaneously used by
> the kernel.
>
> At boot time we can portion off a section of memory for this (and not tell the
> kernel about it), but sometimes you want to be able to use the hardware
> debugging facilities and you haven't done this and you don't want to reboot
> your machine - and memtrace is the solution for this.
>
> If you're curious one tool that uses this debugging facility is here:
> https://github.com/open-power/pdbg. Relevant files are libpdbg/htm.c and src/htm.c.
>
>
>> I have seen a couple of reports in the past from people running memtrace
>> and failing to do so sometimes, and back then I could not grasp why people
>> was using it, or under which circumstances was nice to have.
>> So it would be nice to have a detailed explanation from the person who wrote
>> it.
>>
>
> Is that enough detail?
>
>>
>> Thanks
>>
>> --
>> Oscar Salvador
>> SUSE L3
David Hildenbrand July 16, 2019, 12:28 p.m. UTC | #13
On 02.07.19 08:42, Rashmica Gupta wrote:
> Hi David,
> 
> Sorry for the late reply.

Hi,

sorry I was on PTO :)

> 
> On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
>> On 26.06.19 10:15, Oscar Salvador wrote:
>>> On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand wrote:
>>>> Back then, I already mentioned that we might have some users that
>>>> remove_memory() they never added in a granularity it wasn't
>>>> added. My
>>>> concerns back then were never fully sorted out.
>>>>
>>>> arch/powerpc/platforms/powernv/memtrace.c
>>>>
>>>> - Will remove memory in memory block size chunks it never added
>>>> - What if that memory resides on a DIMM added via
>>>> MHP_MEMMAP_DEVICE?
>>>>
>>>> Will it at least bail out? Or simply break?
>>>>
>>>> IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is save
>>>> to be
>>>> introduced.
>>>
>>> Uhm, I will take a closer look and see if I can clear your
>>> concerns.
>>> TBH, I did not try to use arch/powerpc/platforms/powernv/memtrace.c
>>> yet.
>>>
>>> I will get back to you once I tried it out.
>>>
>>
>> BTW, I consider the code in arch/powerpc/platforms/powernv/memtrace.c
>> very ugly and dangerous.
> 
> Yes it would be nice to clean this up.
> 
>> We should never allow to manually
>> offline/online pages / hack into memory block states.
>>
>> What I would want to see here is rather:
>>
>> 1. User space offlines the blocks to be used
>> 2. memtrace installs a hotplug notifier and hinders the blocks it
>> wants
>> to use from getting onlined.
>> 3. memory is not added/removed/onlined/offlined in memtrace code.
>>
> 
> I remember looking into doing it a similar way. I can't recall the
> details but my issue was probably 'how does userspace indicate to
> the kernel that this memory being offlined should be removed'?

Instead of indicating a "size", indicate the offline memory blocks that
the driver should use. E.g. by memory block id for each node

0:20-24,1:30-32

Of course, other interfaces might make sense.

You can then start using these memory blocks and hinder them from
getting onlined (as a safety net) via memory notifiers.

That would at least avoid you having to call
add_memory/remove_memory/offline_pages/device_online/modifying memblock
states manually.

(binding the memory block devices to a driver would be nicer, but the
infrastructure is not really there yet - we have no such drivers in
place yet)

> 
> I don't know the mm code nor how the notifiers work very well so I
> can't quite see how the above would work. I'm assuming memtrace would
> register a hotplug notifier and when memory is offlined from userspace,
> the callback func in memtrace would be called if the priority was high
> enough? But how do we know that the memory being offlined is intended
> for usto touch? Is there a way to offline memory from userspace not
> using sysfs or have I missed something in the sysfs interface?

The notifier would really only be used to hinder onlining as a safety
net. User space prepares (offlines) the memory blocks and then tells the
drivers which memory blocks to use.

> 
> On a second read, perhaps you are assuming that memtrace is used after
> adding new memory at runtime? If so, that is not the case. If not, then
> would you be able to clarify what I'm not seeing?

The main problem I see is that you are calling
add_memory/remove_memory() on memory your device driver doesn't own. It
could reside on a DIMM if I am not mistaking (or later on
paravirtualized memory devices like virtio-mem if I ever get to
implement them ;) ).

How is it guaranteed that the memory you are allocating does not reside
on a DIMM for example added via add_memory() by the ACPI driver?
Rashmica Gupta July 29, 2019, 5:42 a.m. UTC | #14
On Tue, 2019-07-16 at 14:28 +0200, David Hildenbrand wrote:
> On 02.07.19 08:42, Rashmica Gupta wrote:
> > Hi David,
> > 
> > Sorry for the late reply.
> 
> Hi,
> 
> sorry I was on PTO :)
> 
> > On Wed, 2019-06-26 at 10:28 +0200, David Hildenbrand wrote:
> > > On 26.06.19 10:15, Oscar Salvador wrote:
> > > > On Wed, Jun 26, 2019 at 10:11:06AM +0200, David Hildenbrand
> > > > wrote:
> > > > > Back then, I already mentioned that we might have some users
> > > > > that
> > > > > remove_memory() they never added in a granularity it wasn't
> > > > > added. My
> > > > > concerns back then were never fully sorted out.
> > > > > 
> > > > > arch/powerpc/platforms/powernv/memtrace.c
> > > > > 
> > > > > - Will remove memory in memory block size chunks it never
> > > > > added
> > > > > - What if that memory resides on a DIMM added via
> > > > > MHP_MEMMAP_DEVICE?
> > > > > 
> > > > > Will it at least bail out? Or simply break?
> > > > > 
> > > > > IOW: I am not yet 100% convinced that MHP_MEMMAP_DEVICE is
> > > > > save
> > > > > to be
> > > > > introduced.
> > > > 
> > > > Uhm, I will take a closer look and see if I can clear your
> > > > concerns.
> > > > TBH, I did not try to use
> > > > arch/powerpc/platforms/powernv/memtrace.c
> > > > yet.
> > > > 
> > > > I will get back to you once I tried it out.
> > > > 
> > > 
> > > BTW, I consider the code in
> > > arch/powerpc/platforms/powernv/memtrace.c
> > > very ugly and dangerous.
> > 
> > Yes it would be nice to clean this up.
> > 
> > > We should never allow to manually
> > > offline/online pages / hack into memory block states.
> > > 
> > > What I would want to see here is rather:
> > > 
> > > 1. User space offlines the blocks to be used
> > > 2. memtrace installs a hotplug notifier and hinders the blocks it
> > > wants
> > > to use from getting onlined.
> > > 3. memory is not added/removed/onlined/offlined in memtrace code.
> > > 
> > 
> > I remember looking into doing it a similar way. I can't recall the
> > details but my issue was probably 'how does userspace indicate to
> > the kernel that this memory being offlined should be removed'?
> 
> Instead of indicating a "size", indicate the offline memory blocks
> that
> the driver should use. E.g. by memory block id for each node
> 
> 0:20-24,1:30-32
> 
> Of course, other interfaces might make sense.
> 
> You can then start using these memory blocks and hinder them from
> getting onlined (as a safety net) via memory notifiers.
> 
> That would at least avoid you having to call
> add_memory/remove_memory/offline_pages/device_online/modifying
> memblock
> states manually.

I see what you're saying and that definitely sounds safer.

We would still need to call remove_memory and add_memory from memtrace
as
just offlining memory doesn't remove it from the linear page tables
(if 
it's still in the page tables then hardware can prefetch it and if
hardware tracing is using it then the box checkstops).

> 
> (binding the memory block devices to a driver would be nicer, but the
> infrastructure is not really there yet - we have no such drivers in
> place yet)
> 
> > I don't know the mm code nor how the notifiers work very well so I
> > can't quite see how the above would work. I'm assuming memtrace
> > would
> > register a hotplug notifier and when memory is offlined from
> > userspace,
> > the callback func in memtrace would be called if the priority was
> > high
> > enough? But how do we know that the memory being offlined is
> > intended
> > for usto touch? Is there a way to offline memory from userspace not
> > using sysfs or have I missed something in the sysfs interface?
> 
> The notifier would really only be used to hinder onlining as a safety
> net. User space prepares (offlines) the memory blocks and then tells
> the
> drivers which memory blocks to use.
> 
> > On a second read, perhaps you are assuming that memtrace is used
> > after
> > adding new memory at runtime? If so, that is not the case. If not,
> > then
> > would you be able to clarify what I'm not seeing?
> 
> The main problem I see is that you are calling
> add_memory/remove_memory() on memory your device driver doesn't own.
> It
> could reside on a DIMM if I am not mistaking (or later on
> paravirtualized memory devices like virtio-mem if I ever get to
> implement them ;) ).

This is just for baremetal/powernv so shouldn't affect virtual memory
devices.

> 
> How is it guaranteed that the memory you are allocating does not
> reside
> on a DIMM for example added via add_memory() by the ACPI driver?

Good point. We don't have ACPI on powernv but currently this would try
to remove memory from any online memory node, not just the ones that
are backed by RAM. oops.
David Hildenbrand July 29, 2019, 8:06 a.m. UTC | #15
>> Of course, other interfaces might make sense.
>>
>> You can then start using these memory blocks and hinder them from
>> getting onlined (as a safety net) via memory notifiers.
>>
>> That would at least avoid you having to call
>> add_memory/remove_memory/offline_pages/device_online/modifying
>> memblock
>> states manually.
> 
> I see what you're saying and that definitely sounds safer.
> 
> We would still need to call remove_memory and add_memory from memtrace
> as
> just offlining memory doesn't remove it from the linear page tables
> (if 
> it's still in the page tables then hardware can prefetch it and if
> hardware tracing is using it then the box checkstops).

That prefetching part is interesting (and nasty as well). If we could at
least get rid of the manual onlining/offlining, I would be able to sleep
better at night ;) One step at a time.

> 
>>
>> (binding the memory block devices to a driver would be nicer, but the
>> infrastructure is not really there yet - we have no such drivers in
>> place yet)
>>
>>> I don't know the mm code nor how the notifiers work very well so I
>>> can't quite see how the above would work. I'm assuming memtrace
>>> would
>>> register a hotplug notifier and when memory is offlined from
>>> userspace,
>>> the callback func in memtrace would be called if the priority was
>>> high
>>> enough? But how do we know that the memory being offlined is
>>> intended
>>> for usto touch? Is there a way to offline memory from userspace not
>>> using sysfs or have I missed something in the sysfs interface?
>>
>> The notifier would really only be used to hinder onlining as a safety
>> net. User space prepares (offlines) the memory blocks and then tells
>> the
>> drivers which memory blocks to use.
>>
>>> On a second read, perhaps you are assuming that memtrace is used
>>> after
>>> adding new memory at runtime? If so, that is not the case. If not,
>>> then
>>> would you be able to clarify what I'm not seeing?
>>
>> The main problem I see is that you are calling
>> add_memory/remove_memory() on memory your device driver doesn't own.
>> It
>> could reside on a DIMM if I am not mistaking (or later on
>> paravirtualized memory devices like virtio-mem if I ever get to
>> implement them ;) ).
> 
> This is just for baremetal/powernv so shouldn't affect virtual memory
> devices.

Good to now.

> 
>>
>> How is it guaranteed that the memory you are allocating does not
>> reside
>> on a DIMM for example added via add_memory() by the ACPI driver?
> 
> Good point. We don't have ACPI on powernv but currently this would try
> to remove memory from any online memory node, not just the ones that
> are backed by RAM. oops.

Okay, so essentially no memory hotplug/unplug along with memtrace. (can
we document that somewhere?). I think add_memory()/try_remove_memory()
could be tolerable in these environments (as it's only boot memory).
Rashmica Gupta July 30, 2019, 7:08 a.m. UTC | #16
On Mon, 2019-07-29 at 10:06 +0200, David Hildenbrand wrote:
> > > Of course, other interfaces might make sense.
> > > 
> > > You can then start using these memory blocks and hinder them from
> > > getting onlined (as a safety net) via memory notifiers.
> > > 
> > > That would at least avoid you having to call
> > > add_memory/remove_memory/offline_pages/device_online/modifying
> > > memblock
> > > states manually.
> > 
> > I see what you're saying and that definitely sounds safer.
> > 
> > We would still need to call remove_memory and add_memory from
> > memtrace
> > as
> > just offlining memory doesn't remove it from the linear page tables
> > (if 
> > it's still in the page tables then hardware can prefetch it and if
> > hardware tracing is using it then the box checkstops).
> 
> That prefetching part is interesting (and nasty as well). If we could
> at
> least get rid of the manual onlining/offlining, I would be able to
> sleep
> better at night ;) One step at a time.
>

Ok, I'll get to that soon :)

> > > (binding the memory block devices to a driver would be nicer, but
> > > the
> > > infrastructure is not really there yet - we have no such drivers
> > > in
> > > place yet)
> > > 
> > > > I don't know the mm code nor how the notifiers work very well
> > > > so I
> > > > can't quite see how the above would work. I'm assuming memtrace
> > > > would
> > > > register a hotplug notifier and when memory is offlined from
> > > > userspace,
> > > > the callback func in memtrace would be called if the priority
> > > > was
> > > > high
> > > > enough? But how do we know that the memory being offlined is
> > > > intended
> > > > for usto touch? Is there a way to offline memory from userspace
> > > > not
> > > > using sysfs or have I missed something in the sysfs interface?
> > > 
> > > The notifier would really only be used to hinder onlining as a
> > > safety
> > > net. User space prepares (offlines) the memory blocks and then
> > > tells
> > > the
> > > drivers which memory blocks to use.
> > > 
> > > > On a second read, perhaps you are assuming that memtrace is
> > > > used
> > > > after
> > > > adding new memory at runtime? If so, that is not the case. If
> > > > not,
> > > > then
> > > > would you be able to clarify what I'm not seeing?
> > > 
> > > The main problem I see is that you are calling
> > > add_memory/remove_memory() on memory your device driver doesn't
> > > own.
> > > It
> > > could reside on a DIMM if I am not mistaking (or later on
> > > paravirtualized memory devices like virtio-mem if I ever get to
> > > implement them ;) ).
> > 
> > This is just for baremetal/powernv so shouldn't affect virtual
> > memory
> > devices.
> 
> Good to now.
> 
> > > How is it guaranteed that the memory you are allocating does not
> > > reside
> > > on a DIMM for example added via add_memory() by the ACPI driver?
> > 
> > Good point. We don't have ACPI on powernv but currently this would
> > try
> > to remove memory from any online memory node, not just the ones
> > that
> > are backed by RAM. oops.
> 
> Okay, so essentially no memory hotplug/unplug along with memtrace.
> (can
> we document that somewhere?). I think
> add_memory()/try_remove_memory()
> could be tolerable in these environments (as it's only boot memory).
>
Sure thing.
Rashmica Gupta July 31, 2019, 2:21 a.m. UTC | #17
On Mon, 2019-07-29 at 10:06 +0200, David Hildenbrand wrote:
> > > Of course, other interfaces might make sense.
> > > 
> > > You can then start using these memory blocks and hinder them from
> > > getting onlined (as a safety net) via memory notifiers.
> > > 
> > > That would at least avoid you having to call
> > > add_memory/remove_memory/offline_pages/device_online/modifying
> > > memblock
> > > states manually.
> > 
> > I see what you're saying and that definitely sounds safer.
> > 
> > We would still need to call remove_memory and add_memory from
> > memtrace
> > as
> > just offlining memory doesn't remove it from the linear page tables
> > (if 
> > it's still in the page tables then hardware can prefetch it and if
> > hardware tracing is using it then the box checkstops).
> 
> That prefetching part is interesting (and nasty as well). If we could
> at
> least get rid of the manual onlining/offlining, I would be able to
> sleep
> better at night ;) One step at a time.
> 

What are your thoughts on adding remove to state_store in
drivers/base/memory.c? And an accompanying add? So then userspace could
do "echo remove > memory34/state"? 

Then most of the memtrace code could be moved to a userspace tool. The
only bit that we would need to keep in the kernel is setting up debugfs
files in memtrace_init_debugfs.
David Hildenbrand July 31, 2019, 9:39 a.m. UTC | #18
On 31.07.19 04:21, Rashmica Gupta wrote:
> On Mon, 2019-07-29 at 10:06 +0200, David Hildenbrand wrote:
>>>> Of course, other interfaces might make sense.
>>>>
>>>> You can then start using these memory blocks and hinder them from
>>>> getting onlined (as a safety net) via memory notifiers.
>>>>
>>>> That would at least avoid you having to call
>>>> add_memory/remove_memory/offline_pages/device_online/modifying
>>>> memblock
>>>> states manually.
>>>
>>> I see what you're saying and that definitely sounds safer.
>>>
>>> We would still need to call remove_memory and add_memory from
>>> memtrace
>>> as
>>> just offlining memory doesn't remove it from the linear page tables
>>> (if 
>>> it's still in the page tables then hardware can prefetch it and if
>>> hardware tracing is using it then the box checkstops).
>>
>> That prefetching part is interesting (and nasty as well). If we could
>> at
>> least get rid of the manual onlining/offlining, I would be able to
>> sleep
>> better at night ;) One step at a time.
>>
> 
> What are your thoughts on adding remove to state_store in
> drivers/base/memory.c? And an accompanying add? So then userspace could
> do "echo remove > memory34/state"? 

I consider such an interface dangerous (removing random memory you don't
own, especially in the context of Oscars work some blocks would suddenly
not be removable again) and only of limited used. The requirement of
memtrace is really special, and it only works somewhat reliably with
boot memory.

FWIW, we do have a "probe" interface on some architectures but decided
that a "remove" interface belongs into a debug/test kernel module. The
memory block device API is already messed up enough (e.g., "removable"
property which doesn't indicate at all if memory can be removed, only if
it could be offlined) - we decided to keep changes at a minimum for now
- rather clean up than add new stuff.

> Then most of the memtrace code could be moved to a userspace tool. The
> only bit that we would need to keep in the kernel is setting up debugfs
> files in memtrace_init_debugfs.

The nice thing is, with remove_memory() you will now get an error in
case all memory blocks are not offline yet. So it only boils down to
calling add_memory()/remove_memory() without even checking the state of
the blocks. (only nids have to be handled manually correctly)
Michal Hocko July 31, 2019, 12:08 p.m. UTC | #19
On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
[...]
> > 2) Why it was designed, what is the goal of the interface?
> > 3) When it is supposed to be used?
> >
> >
> There is a hardware debugging facility (htm) on some power chips. To use
> this you need a contiguous portion of memory for the output to be dumped
> to - and we obviously don't want this memory to be simultaneously used by
> the kernel.

How much memory are we talking about here? Just curious.
Rashmica Gupta July 31, 2019, 11:06 p.m. UTC | #20
On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
> [...]
> > > 2) Why it was designed, what is the goal of the interface?
> > > 3) When it is supposed to be used?
> > > 
> > > 
> > There is a hardware debugging facility (htm) on some power chips.
> > To use
> > this you need a contiguous portion of memory for the output to be
> > dumped
> > to - and we obviously don't want this memory to be simultaneously
> > used by
> > the kernel.
> 
> How much memory are we talking about here? Just curious.

From what I've seen a couple of GB per node, so maybe 2-10GB total.
Michal Hocko Aug. 1, 2019, 7:17 a.m. UTC | #21
On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
> > On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
> > [...]
> > > > 2) Why it was designed, what is the goal of the interface?
> > > > 3) When it is supposed to be used?
> > > > 
> > > > 
> > > There is a hardware debugging facility (htm) on some power chips.
> > > To use
> > > this you need a contiguous portion of memory for the output to be
> > > dumped
> > > to - and we obviously don't want this memory to be simultaneously
> > > used by
> > > the kernel.
> > 
> > How much memory are we talking about here? Just curious.
> 
> From what I've seen a couple of GB per node, so maybe 2-10GB total.

OK, that is really a lot to keep around unused just in case the
debugging is going to be used.

I am still not sure the current approach of (ab)using memory hotplug is
ideal. Sure there is some overlap but you shouldn't really need to
offline the required memory range at all. All you need is to isolate the
memory from any existing user and the page allocator. Have you checked
alloc_contig_range?
David Hildenbrand Aug. 1, 2019, 7:18 a.m. UTC | #22
On 01.08.19 09:17, Michal Hocko wrote:
> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
>>> [...]
>>>>> 2) Why it was designed, what is the goal of the interface?
>>>>> 3) When it is supposed to be used?
>>>>>
>>>>>
>>>> There is a hardware debugging facility (htm) on some power chips.
>>>> To use
>>>> this you need a contiguous portion of memory for the output to be
>>>> dumped
>>>> to - and we obviously don't want this memory to be simultaneously
>>>> used by
>>>> the kernel.
>>>
>>> How much memory are we talking about here? Just curious.
>>
>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
> 
> OK, that is really a lot to keep around unused just in case the
> debugging is going to be used.
> 
> I am still not sure the current approach of (ab)using memory hotplug is
> ideal. Sure there is some overlap but you shouldn't really need to
> offline the required memory range at all. All you need is to isolate the
> memory from any existing user and the page allocator. Have you checked
> alloc_contig_range?
> 

Rashmica mentioned somewhere in this thread that the virtual mapping
must not be in place, otherwise the HW might prefetch some of this
memory, leading to errors with memtrace (which checks that in HW).
Michal Hocko Aug. 1, 2019, 7:24 a.m. UTC | #23
On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
> On 01.08.19 09:17, Michal Hocko wrote:
> > On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
> >> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
> >>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
> >>> [...]
> >>>>> 2) Why it was designed, what is the goal of the interface?
> >>>>> 3) When it is supposed to be used?
> >>>>>
> >>>>>
> >>>> There is a hardware debugging facility (htm) on some power chips.
> >>>> To use
> >>>> this you need a contiguous portion of memory for the output to be
> >>>> dumped
> >>>> to - and we obviously don't want this memory to be simultaneously
> >>>> used by
> >>>> the kernel.
> >>>
> >>> How much memory are we talking about here? Just curious.
> >>
> >> From what I've seen a couple of GB per node, so maybe 2-10GB total.
> > 
> > OK, that is really a lot to keep around unused just in case the
> > debugging is going to be used.
> > 
> > I am still not sure the current approach of (ab)using memory hotplug is
> > ideal. Sure there is some overlap but you shouldn't really need to
> > offline the required memory range at all. All you need is to isolate the
> > memory from any existing user and the page allocator. Have you checked
> > alloc_contig_range?
> > 
> 
> Rashmica mentioned somewhere in this thread that the virtual mapping
> must not be in place, otherwise the HW might prefetch some of this
> memory, leading to errors with memtrace (which checks that in HW).

Does anything prevent from unmapping the pfn range from the direct
mapping?
David Hildenbrand Aug. 1, 2019, 7:26 a.m. UTC | #24
On 01.08.19 09:24, Michal Hocko wrote:
> On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
>> On 01.08.19 09:17, Michal Hocko wrote:
>>> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
>>>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
>>>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
>>>>> [...]
>>>>>>> 2) Why it was designed, what is the goal of the interface?
>>>>>>> 3) When it is supposed to be used?
>>>>>>>
>>>>>>>
>>>>>> There is a hardware debugging facility (htm) on some power chips.
>>>>>> To use
>>>>>> this you need a contiguous portion of memory for the output to be
>>>>>> dumped
>>>>>> to - and we obviously don't want this memory to be simultaneously
>>>>>> used by
>>>>>> the kernel.
>>>>>
>>>>> How much memory are we talking about here? Just curious.
>>>>
>>>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
>>>
>>> OK, that is really a lot to keep around unused just in case the
>>> debugging is going to be used.
>>>
>>> I am still not sure the current approach of (ab)using memory hotplug is
>>> ideal. Sure there is some overlap but you shouldn't really need to
>>> offline the required memory range at all. All you need is to isolate the
>>> memory from any existing user and the page allocator. Have you checked
>>> alloc_contig_range?
>>>
>>
>> Rashmica mentioned somewhere in this thread that the virtual mapping
>> must not be in place, otherwise the HW might prefetch some of this
>> memory, leading to errors with memtrace (which checks that in HW).
> 
> Does anything prevent from unmapping the pfn range from the direct
> mapping?

I am not sure about the implications of having
pfn_valid()/pfn_present()/pfn_online() return true but accessing it
results in crashes. (suspend, kdump, whatever other technology touches
online memory)

(sounds more like a hack to me than just going ahead and
removing/readding the memory via a clean interface we have)
David Hildenbrand Aug. 1, 2019, 7:31 a.m. UTC | #25
On 01.08.19 09:26, David Hildenbrand wrote:
> On 01.08.19 09:24, Michal Hocko wrote:
>> On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
>>> On 01.08.19 09:17, Michal Hocko wrote:
>>>> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
>>>>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
>>>>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
>>>>>> [...]
>>>>>>>> 2) Why it was designed, what is the goal of the interface?
>>>>>>>> 3) When it is supposed to be used?
>>>>>>>>
>>>>>>>>
>>>>>>> There is a hardware debugging facility (htm) on some power chips.
>>>>>>> To use
>>>>>>> this you need a contiguous portion of memory for the output to be
>>>>>>> dumped
>>>>>>> to - and we obviously don't want this memory to be simultaneously
>>>>>>> used by
>>>>>>> the kernel.
>>>>>>
>>>>>> How much memory are we talking about here? Just curious.
>>>>>
>>>>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
>>>>
>>>> OK, that is really a lot to keep around unused just in case the
>>>> debugging is going to be used.
>>>>
>>>> I am still not sure the current approach of (ab)using memory hotplug is
>>>> ideal. Sure there is some overlap but you shouldn't really need to
>>>> offline the required memory range at all. All you need is to isolate the
>>>> memory from any existing user and the page allocator. Have you checked
>>>> alloc_contig_range?
>>>>
>>>
>>> Rashmica mentioned somewhere in this thread that the virtual mapping
>>> must not be in place, otherwise the HW might prefetch some of this
>>> memory, leading to errors with memtrace (which checks that in HW).
>>
>> Does anything prevent from unmapping the pfn range from the direct
>> mapping?
> 
> I am not sure about the implications of having
> pfn_valid()/pfn_present()/pfn_online() return true but accessing it
> results in crashes. (suspend, kdump, whatever other technology touches
> online memory)

(oneidea: we could of course go ahead and mark the pages PG_offline
before unmapping the pfn range to work around these issues)

> 
> (sounds more like a hack to me than just going ahead and
> removing/readding the memory via a clean interface we have)
>
Michal Hocko Aug. 1, 2019, 7:34 a.m. UTC | #26
On Thu 01-08-19 09:26:35, David Hildenbrand wrote:
> On 01.08.19 09:24, Michal Hocko wrote:
> > On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
> >> On 01.08.19 09:17, Michal Hocko wrote:
> >>> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
> >>>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
> >>>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
> >>>>> [...]
> >>>>>>> 2) Why it was designed, what is the goal of the interface?
> >>>>>>> 3) When it is supposed to be used?
> >>>>>>>
> >>>>>>>
> >>>>>> There is a hardware debugging facility (htm) on some power chips.
> >>>>>> To use
> >>>>>> this you need a contiguous portion of memory for the output to be
> >>>>>> dumped
> >>>>>> to - and we obviously don't want this memory to be simultaneously
> >>>>>> used by
> >>>>>> the kernel.
> >>>>>
> >>>>> How much memory are we talking about here? Just curious.
> >>>>
> >>>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
> >>>
> >>> OK, that is really a lot to keep around unused just in case the
> >>> debugging is going to be used.
> >>>
> >>> I am still not sure the current approach of (ab)using memory hotplug is
> >>> ideal. Sure there is some overlap but you shouldn't really need to
> >>> offline the required memory range at all. All you need is to isolate the
> >>> memory from any existing user and the page allocator. Have you checked
> >>> alloc_contig_range?
> >>>
> >>
> >> Rashmica mentioned somewhere in this thread that the virtual mapping
> >> must not be in place, otherwise the HW might prefetch some of this
> >> memory, leading to errors with memtrace (which checks that in HW).
> > 
> > Does anything prevent from unmapping the pfn range from the direct
> > mapping?
> 
> I am not sure about the implications of having
> pfn_valid()/pfn_present()/pfn_online() return true but accessing it
> results in crashes. (suspend, kdump, whatever other technology touches
> online memory)

If those pages are marked as Reserved then nobody should be touching
them anyway.
 
> (sounds more like a hack to me than just going ahead and
> removing/readding the memory via a clean interface we have)

Right, but the interface that we have is quite restricted in what it can
really offline.
Michal Hocko Aug. 1, 2019, 7:39 a.m. UTC | #27
On Thu 01-08-19 09:31:09, David Hildenbrand wrote:
> On 01.08.19 09:26, David Hildenbrand wrote:
[...]
> > I am not sure about the implications of having
> > pfn_valid()/pfn_present()/pfn_online() return true but accessing it
> > results in crashes. (suspend, kdump, whatever other technology touches
> > online memory)
> 
> (oneidea: we could of course go ahead and mark the pages PG_offline
> before unmapping the pfn range to work around these issues)

PG_reserved and an elevated reference count should be enough to drive
any pfn walker out. Pfn walkers shouldn't touch any page unless they
know and recognize their type.
Michal Hocko Aug. 1, 2019, 7:48 a.m. UTC | #28
On Thu 01-08-19 09:39:57, Michal Hocko wrote:
> On Thu 01-08-19 09:31:09, David Hildenbrand wrote:
> > On 01.08.19 09:26, David Hildenbrand wrote:
> [...]
> > > I am not sure about the implications of having
> > > pfn_valid()/pfn_present()/pfn_online() return true but accessing it
> > > results in crashes. (suspend, kdump, whatever other technology touches
> > > online memory)
> > 
> > (oneidea: we could of course go ahead and mark the pages PG_offline
> > before unmapping the pfn range to work around these issues)
> 
> PG_reserved and an elevated reference count should be enough to drive
> any pfn walker out. Pfn walkers shouldn't touch any page unless they
> know and recognize their type.

Btw. this shouldn't be much different from DEBUG_PAGE_ALLOC in
principle. The memory is valid, but not mapped to the kernel virtual
space. Nobody should be really touching it anyway.
David Hildenbrand Aug. 1, 2019, 7:50 a.m. UTC | #29
On 01.08.19 09:34, Michal Hocko wrote:
> On Thu 01-08-19 09:26:35, David Hildenbrand wrote:
>> On 01.08.19 09:24, Michal Hocko wrote:
>>> On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
>>>> On 01.08.19 09:17, Michal Hocko wrote:
>>>>> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
>>>>>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
>>>>>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
>>>>>>> [...]
>>>>>>>>> 2) Why it was designed, what is the goal of the interface?
>>>>>>>>> 3) When it is supposed to be used?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> There is a hardware debugging facility (htm) on some power chips.
>>>>>>>> To use
>>>>>>>> this you need a contiguous portion of memory for the output to be
>>>>>>>> dumped
>>>>>>>> to - and we obviously don't want this memory to be simultaneously
>>>>>>>> used by
>>>>>>>> the kernel.
>>>>>>>
>>>>>>> How much memory are we talking about here? Just curious.
>>>>>>
>>>>>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
>>>>>
>>>>> OK, that is really a lot to keep around unused just in case the
>>>>> debugging is going to be used.
>>>>>
>>>>> I am still not sure the current approach of (ab)using memory hotplug is
>>>>> ideal. Sure there is some overlap but you shouldn't really need to
>>>>> offline the required memory range at all. All you need is to isolate the
>>>>> memory from any existing user and the page allocator. Have you checked
>>>>> alloc_contig_range?
>>>>>
>>>>
>>>> Rashmica mentioned somewhere in this thread that the virtual mapping
>>>> must not be in place, otherwise the HW might prefetch some of this
>>>> memory, leading to errors with memtrace (which checks that in HW).
>>>
>>> Does anything prevent from unmapping the pfn range from the direct
>>> mapping?
>>
>> I am not sure about the implications of having
>> pfn_valid()/pfn_present()/pfn_online() return true but accessing it
>> results in crashes. (suspend, kdump, whatever other technology touches
>> online memory)
> 
> If those pages are marked as Reserved then nobody should be touching
> them anyway.

Which is not true as I remember we already discussed - I even documented
what PG_reserved can mean after that discussion in page-flags.h (e.g.,
memmap of boot memory) - that's why we introduced PG_offline after all.
Michal Hocko Aug. 1, 2019, 8:04 a.m. UTC | #30
On Thu 01-08-19 09:50:29, David Hildenbrand wrote:
> On 01.08.19 09:34, Michal Hocko wrote:
> > On Thu 01-08-19 09:26:35, David Hildenbrand wrote:
> >> On 01.08.19 09:24, Michal Hocko wrote:
> >>> On Thu 01-08-19 09:18:47, David Hildenbrand wrote:
> >>>> On 01.08.19 09:17, Michal Hocko wrote:
> >>>>> On Thu 01-08-19 09:06:40, Rashmica Gupta wrote:
> >>>>>> On Wed, 2019-07-31 at 14:08 +0200, Michal Hocko wrote:
> >>>>>>> On Tue 02-07-19 18:52:01, Rashmica Gupta wrote:
> >>>>>>> [...]
> >>>>>>>>> 2) Why it was designed, what is the goal of the interface?
> >>>>>>>>> 3) When it is supposed to be used?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>> There is a hardware debugging facility (htm) on some power chips.
> >>>>>>>> To use
> >>>>>>>> this you need a contiguous portion of memory for the output to be
> >>>>>>>> dumped
> >>>>>>>> to - and we obviously don't want this memory to be simultaneously
> >>>>>>>> used by
> >>>>>>>> the kernel.
> >>>>>>>
> >>>>>>> How much memory are we talking about here? Just curious.
> >>>>>>
> >>>>>> From what I've seen a couple of GB per node, so maybe 2-10GB total.
> >>>>>
> >>>>> OK, that is really a lot to keep around unused just in case the
> >>>>> debugging is going to be used.
> >>>>>
> >>>>> I am still not sure the current approach of (ab)using memory hotplug is
> >>>>> ideal. Sure there is some overlap but you shouldn't really need to
> >>>>> offline the required memory range at all. All you need is to isolate the
> >>>>> memory from any existing user and the page allocator. Have you checked
> >>>>> alloc_contig_range?
> >>>>>
> >>>>
> >>>> Rashmica mentioned somewhere in this thread that the virtual mapping
> >>>> must not be in place, otherwise the HW might prefetch some of this
> >>>> memory, leading to errors with memtrace (which checks that in HW).
> >>>
> >>> Does anything prevent from unmapping the pfn range from the direct
> >>> mapping?
> >>
> >> I am not sure about the implications of having
> >> pfn_valid()/pfn_present()/pfn_online() return true but accessing it
> >> results in crashes. (suspend, kdump, whatever other technology touches
> >> online memory)
> > 
> > If those pages are marked as Reserved then nobody should be touching
> > them anyway.
> 
> Which is not true as I remember we already discussed - I even documented
> what PG_reserved can mean after that discussion in page-flags.h (e.g.,
> memmap of boot memory) - that's why we introduced PG_offline after all.

Sorry, my statement was imprecise. What I meant is what we have
documented:
 * PG_reserved is set for special pages. The "struct page" of such a page
 * should in general not be touched (e.g. set dirty) except by its owner.

the owner part is important.
David Hildenbrand Aug. 1, 2019, 9:18 a.m. UTC | #31
On 01.08.19 09:48, Michal Hocko wrote:
> On Thu 01-08-19 09:39:57, Michal Hocko wrote:
>> On Thu 01-08-19 09:31:09, David Hildenbrand wrote:
>>> On 01.08.19 09:26, David Hildenbrand wrote:
>> [...]
>>>> I am not sure about the implications of having
>>>> pfn_valid()/pfn_present()/pfn_online() return true but accessing it
>>>> results in crashes. (suspend, kdump, whatever other technology touches
>>>> online memory)
>>>
>>> (oneidea: we could of course go ahead and mark the pages PG_offline
>>> before unmapping the pfn range to work around these issues)
>>
>> PG_reserved and an elevated reference count should be enough to drive
>> any pfn walker out. Pfn walkers shouldn't touch any page unless they
>> know and recognize their type.
> 
> Btw. this shouldn't be much different from DEBUG_PAGE_ALLOC in
> principle. The memory is valid, but not mapped to the kernel virtual
> space. Nobody should be really touching it anyway.
> 

I guess that could work (I am happy with anything that gets rid of
offline_pages()/device_online() here :D ).

So for each node, alloc_contig_range() (if I remember correctly, all
pages in the range have to be in the same zone), set them PG_reserved (+
maybe something else, we'll have to see). Then, unmap them.

The reverse when freeing the memory. Guess this should leave the current
user space interface unmodified.

I can see that guard pages use a special page type (PageGuard), not sure
if something like that is really required. We'll have to see.