2023-04-06 04:12:45

by John Hubbard

[permalink] [raw]
Subject: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
functionality was first developed on x86, it also works on arm64.
However, when trying this out on an arm64 system, it turns out that
there is a massive slowdown during the setup and teardown phases.

This slowdown is due to lots of calls to WARN_ON()'s that are checking
for pages that are out of the physical range for the CPU. However,
that's a design feature of device private pages: they are specfically
chosen in order to be outside of the range of the CPU's true physical
pages.

x86 doesn't have this warning. It only checks that pages are properly
aligned. I've shown a comparison below between x86 (which works well)
and arm64 (which has these warnings).

memunmap_pages()
pageunmap_range()
if (pgmap->type == MEMORY_DEVICE_PRIVATE)
__remove_pages()
__remove_section()
sparse_remove_section()
section_deactivate()
depopulate_section_memmap()
/* arch/arm64/mm/mmu.c */
vmemmap_free()
{
WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
...
}

/* arch/x86/mm/init_64.c */
vmemmap_free()
{
VM_BUG_ON(!PAGE_ALIGNED(start));
VM_BUG_ON(!PAGE_ALIGNED(end));
...
}

So, the warning is a false positive for this case. Therefore, skip the
warning if CONFIG_DEVICE_PRIVATE is set.

Signed-off-by: John Hubbard <[email protected]>
cc: <[email protected]>
---
arch/arm64/mm/mmu.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 6f9d8898a025..d5c9b611a8d1 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1157,8 +1157,10 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
struct vmem_altmap *altmap)
{
+/* Device private pages are outside of the CPU's physical page range. */
+#ifndef CONFIG_DEVICE_PRIVATE
WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
-
+#endif
if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
return vmemmap_populate_basepages(start, end, node, altmap);
else
@@ -1169,8 +1171,10 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
void vmemmap_free(unsigned long start, unsigned long end,
struct vmem_altmap *altmap)
{
+/* Device private pages are outside of the CPU's physical page range. */
+#ifndef CONFIG_DEVICE_PRIVATE
WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
-
+#endif
unmap_hotplug_range(start, end, true, altmap);
free_empty_tables(start, end, VMEMMAP_START, VMEMMAP_END);
}
--
2.40.0


2023-04-06 07:39:56

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

Hello John,

On Thu, 6 Apr 2023 at 06:05, John Hubbard <[email protected]> wrote:
>
> Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
> functionality was first developed on x86, it also works on arm64.
> However, when trying this out on an arm64 system, it turns out that
> there is a massive slowdown during the setup and teardown phases.
>
> This slowdown is due to lots of calls to WARN_ON()'s that are checking
> for pages that are out of the physical range for the CPU. However,
> that's a design feature of device private pages: they are specfically
> chosen in order to be outside of the range of the CPU's true physical
> pages.
>

Currently, the vmemmap region is dimensioned to only cover the PFN
range that backs the linear map. So the WARN() seems appropriate here:
you are mapping struct page[] ranges outside of the allocated window,
and afaict, you might actually wrap around and corrupt the linear map
at the start of the kernel VA space like this.


> x86 doesn't have this warning. It only checks that pages are properly
> aligned. I've shown a comparison below between x86 (which works well)
> and arm64 (which has these warnings).
>
> memunmap_pages()
> pageunmap_range()
> if (pgmap->type == MEMORY_DEVICE_PRIVATE)
> __remove_pages()
> __remove_section()
> sparse_remove_section()
> section_deactivate()
> depopulate_section_memmap()
> /* arch/arm64/mm/mmu.c */
> vmemmap_free()
> {
> WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
> ...
> }
>
> /* arch/x86/mm/init_64.c */
> vmemmap_free()
> {
> VM_BUG_ON(!PAGE_ALIGNED(start));
> VM_BUG_ON(!PAGE_ALIGNED(end));
> ...
> }
>
> So, the warning is a false positive for this case. Therefore, skip the
> warning if CONFIG_DEVICE_PRIVATE is set.
>

I don't think this is a false positive. We'll need to adjust
VMEMMAP_SIZE to account for this.


> Signed-off-by: John Hubbard <[email protected]>
> cc: <[email protected]>
> ---
> arch/arm64/mm/mmu.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 6f9d8898a025..d5c9b611a8d1 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1157,8 +1157,10 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
> int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> struct vmem_altmap *altmap)
> {
> +/* Device private pages are outside of the CPU's physical page range. */
> +#ifndef CONFIG_DEVICE_PRIVATE
> WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
> -
> +#endif
> if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
> return vmemmap_populate_basepages(start, end, node, altmap);
> else
> @@ -1169,8 +1171,10 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> void vmemmap_free(unsigned long start, unsigned long end,
> struct vmem_altmap *altmap)
> {
> +/* Device private pages are outside of the CPU's physical page range. */
> +#ifndef CONFIG_DEVICE_PRIVATE
> WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
> -
> +#endif
> unmap_hotplug_range(start, end, true, altmap);
> free_empty_tables(start, end, VMEMMAP_START, VMEMMAP_END);
> }
> --
> 2.40.0
>

2023-04-06 20:10:52

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On Wed, 5 Apr 2023 21:05:15 -0700 John Hubbard <[email protected]> wrote:

> Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
> functionality was first developed on x86, it also works on arm64.
> However, when trying this out on an arm64 system, it turns out that
> there is a massive slowdown during the setup and teardown phases.
>
> This slowdown is due to lots of calls to WARN_ON()'s that are checking
> for pages that are out of the physical range for the CPU. However,
> that's a design feature of device private pages: they are specfically
> chosen in order to be outside of the range of the CPU's true physical
> pages.
>
> ...
>
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1157,8 +1157,10 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
> int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> struct vmem_altmap *altmap)
> {
> +/* Device private pages are outside of the CPU's physical page range. */
> +#ifndef CONFIG_DEVICE_PRIVATE
> WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));

For a simple expression like this to cause a "massive slowdown", I
assume the WARN is triggering. But changelog doesn't mention massive
dmesg spewage?

Given Ard's comments, perhaps a switch to WARN_ON_ONCE() would suit?

2023-04-06 20:21:03

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On 4/6/23 13:07, Andrew Morton wrote:
> On Wed, 5 Apr 2023 21:05:15 -0700 John Hubbard <[email protected]> wrote:
>
>> Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
>> functionality was first developed on x86, it also works on arm64.
>> However, when trying this out on an arm64 system, it turns out that
>> there is a massive slowdown during the setup and teardown phases.
>>
>> This slowdown is due to lots of calls to WARN_ON()'s that are checking
>> for pages that are out of the physical range for the CPU. However,
>> that's a design feature of device private pages: they are specfically
>> chosen in order to be outside of the range of the CPU's true physical
>> pages.
>>
>> ...
>>
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1157,8 +1157,10 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
>> int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>> struct vmem_altmap *altmap)
>> {
>> +/* Device private pages are outside of the CPU's physical page range. */
>> +#ifndef CONFIG_DEVICE_PRIVATE
>> WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
>
> For a simple expression like this to cause a "massive slowdown", I
> assume the WARN is triggering. But changelog doesn't mention massive
> dmesg spewage?

Well, it should. Whoever wrote that needs to improve the changelog. :)

>
> Given Ard's comments, perhaps a switch to WARN_ON_ONCE() would suit?

That would fix up the user-visible problems, which would be very nice.

Meanwhile, I'm trying to sort out whether this really is a false
positive for arm64.

thanks,
--
John Hubbard
NVIDIA

2023-04-07 00:16:54

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On 4/6/23 00:31, Ard Biesheuvel wrote:
> Hello John,
>
> On Thu, 6 Apr 2023 at 06:05, John Hubbard <[email protected]> wrote:
>>
>> Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
>> functionality was first developed on x86, it also works on arm64.
>> However, when trying this out on an arm64 system, it turns out that
>> there is a massive slowdown during the setup and teardown phases.
>>
>> This slowdown is due to lots of calls to WARN_ON()'s that are checking
>> for pages that are out of the physical range for the CPU. However,
>> that's a design feature of device private pages: they are specfically
>> chosen in order to be outside of the range of the CPU's true physical
>> pages.
>>

Hi Ard,

Thank you for looking at this so soon, I've been working on filling
in some details and studying what you said.

By the way, to address an implicit question from Andrew on the other
thread, the reason that this slows things down is due to a loop in
__add_pages() that repeatedly calls through to vmemmap_populate(),
like this:

device driver setup: allocate struct pages for the device (GPU)
memremap_pages(pagemap.type = MEMORY_DEVICE_PRIVATE)
pagemap_range()
__add_pages() /* device private case does this */
for (; pfn < end_pfn; pfn += cur_nr_pages) {
/* this loops 125 times on an x86 test machine: */
sparse_add_section()
section_activate()
populate_section_memmap()
__populate_section_memmap()
vmemmap_populate()

>
> Currently, the vmemmap region is dimensioned to only cover the PFN
> range that backs the linear map. So the WARN() seems appropriate here:
> you are mapping struct page[] ranges outside of the allocated window,
> and afaict, you might actually wrap around and corrupt the linear map
> at the start of the kernel VA space like this.
>

Well...it's only doing a partial hotplug of these pages, just enough to get
ZONE_DEVICE to work. As I understand it so far, only the struct pages
themselves are ever accessed, not any mappings.

More below:

...
>> /* arch/x86/mm/init_64.c */
>> vmemmap_free()
>> {
>> VM_BUG_ON(!PAGE_ALIGNED(start));
>> VM_BUG_ON(!PAGE_ALIGNED(end));
>> ...
>> }
>>
>> So, the warning is a false positive for this case. Therefore, skip the
>> warning if CONFIG_DEVICE_PRIVATE is set.
>>
>
> I don't think this is a false positive. We'll need to adjust
> VMEMMAP_SIZE to account for this.
>

Looking at the (new to me) arm64 code for this, VMEMMAP_SIZE is only
ever used to calculate VMEMMAP_END, which in turn is used for the
WARN_ON()'s in question, plus as the "ceiling" arg to free_empty_tables().

It seems Mostly Harmless. How would you feel about changing it to a
WARN_ON_ONCE() as Andrew suggested? That would completely fix the
user-visible symptoms:

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 6f9d8898a025..82d4205af9f2 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1157,7 +1157,7 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
struct vmem_altmap *altmap)
{
- WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
+ WARN_ON_ONCE((start < VMEMMAP_START) || (end > VMEMMAP_END));

if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
return vmemmap_populate_basepages(start, end, node, altmap);
@@ -1169,7 +1169,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
void vmemmap_free(unsigned long start, unsigned long end,
struct vmem_altmap *altmap)
{
- WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
+ WARN_ON_ONCE((start < VMEMMAP_START) || (end > VMEMMAP_END));

unmap_hotplug_range(start, end, true, altmap);
free_empty_tables(start, end, VMEMMAP_START, VMEMMAP_END);



thanks,
--
John Hubbard
NVIDIA

2023-04-07 10:49:33

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On Fri, 7 Apr 2023 at 02:13, John Hubbard <[email protected]> wrote:
>
> On 4/6/23 00:31, Ard Biesheuvel wrote:
> > Hello John,
> >
> > On Thu, 6 Apr 2023 at 06:05, John Hubbard <[email protected]> wrote:
> >>
> >> Although CONFIG_DEVICE_PRIVATE and hmm_range_fault() and related
> >> functionality was first developed on x86, it also works on arm64.
> >> However, when trying this out on an arm64 system, it turns out that
> >> there is a massive slowdown during the setup and teardown phases.
> >>
> >> This slowdown is due to lots of calls to WARN_ON()'s that are checking
> >> for pages that are out of the physical range for the CPU. However,
> >> that's a design feature of device private pages: they are specfically
> >> chosen in order to be outside of the range of the CPU's true physical
> >> pages.
> >>
>
> Hi Ard,
>
> Thank you for looking at this so soon, I've been working on filling
> in some details and studying what you said.
>
> By the way, to address an implicit question from Andrew on the other
> thread, the reason that this slows things down is due to a loop in
> __add_pages() that repeatedly calls through to vmemmap_populate(),
> like this:
>
> device driver setup: allocate struct pages for the device (GPU)
> memremap_pages(pagemap.type = MEMORY_DEVICE_PRIVATE)
> pagemap_range()
> __add_pages() /* device private case does this */
> for (; pfn < end_pfn; pfn += cur_nr_pages) {
> /* this loops 125 times on an x86 test machine: */
> sparse_add_section()
> section_activate()
> populate_section_memmap()
> __populate_section_memmap()
> vmemmap_populate()
>
> >
> > Currently, the vmemmap region is dimensioned to only cover the PFN
> > range that backs the linear map. So the WARN() seems appropriate here:
> > you are mapping struct page[] ranges outside of the allocated window,
> > and afaict, you might actually wrap around and corrupt the linear map
> > at the start of the kernel VA space like this.
> >
>
> Well...it's only doing a partial hotplug of these pages, just enough to get
> ZONE_DEVICE to work. As I understand it so far, only the struct pages
> themselves are ever accessed, not any mappings.
>

That is what I am talking about - the struct pages are allocated in a
region that is reserved for something else.

Maybe an example helps here:

When running the 39-bit VA kernel build on a AMD Seatte board, we will
have (assuming sizeof(struct page) == 64)

memstart_addr := 0x80_0000_0000

PAGE_OFFSET := 0xffff_ff80_0000_0000

VMEMMAP_SHIFT := 6
VMEMMAP_START := 0xffff_fffe_0000_0000
VMEMMAP_SIZE := 0x1_0000_0000

pfn_to_page() conversions are based on ordinary array indexing
involving vemmap[], where vmemmap is defined as

#define vmemmap \
((struct page *)VMEMMAP_START - (memstart_addr >> PAGE_SHIFT))

So the PFN associated with the first usable DRAM address is
0x800_0000, and pfn_to_page(0x800_0000) will return VMEMMAP_START.

pfn_to_page(x) for any x < 0x800_0000 will produce a kernel VA that
points into the vmalloc area, and may conflict with the kernel
mapping, modules mappings, per-CPU mappings, IO mappings, etc etc.

pfn_to_page(x) for values 0xc00_0000 < x < 0x1000_0000 will produce a
kernel VA that points outside the region set aside for the vmemmap.
This region is currently unused, but that will likely change soon.

pfn_to_page(x) for any x >= 0x1000_0000 will wrap around and produce a
bogus address in the user range.

The bottom line is that the VMEMMAP region is dimensioned to cover
system memory only, i.e., what can be covered by the kernel direct
map. If you want to allocate struct pages for thing that are not
system memory, you will need to enlarge the VMEMMAP region, and ensure
that request_mem_region() produces a region that is covered by it.

This is going to be tricky with LPA2, because there, the 4k pages
configuration already uses up half of the vmalloc region to cover the
linear map, so we have to consider this carefully.


> More below:
>
> ...
> >> /* arch/x86/mm/init_64.c */
> >> vmemmap_free()
> >> {
> >> VM_BUG_ON(!PAGE_ALIGNED(start));
> >> VM_BUG_ON(!PAGE_ALIGNED(end));
> >> ...
> >> }
> >>
> >> So, the warning is a false positive for this case. Therefore, skip the
> >> warning if CONFIG_DEVICE_PRIVATE is set.
> >>
> >
> > I don't think this is a false positive. We'll need to adjust
> > VMEMMAP_SIZE to account for this.
> >
>
> Looking at the (new to me) arm64 code for this, VMEMMAP_SIZE is only
> ever used to calculate VMEMMAP_END, which in turn is used for the
> WARN_ON()'s in question, plus as the "ceiling" arg to free_empty_tables().
>
> It seems Mostly Harmless. How would you feel about changing it to a
> WARN_ON_ONCE() as Andrew suggested? That would completely fix the
> user-visible symptoms:
>

Actually, it should be a hard error, given the above.

> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 6f9d8898a025..82d4205af9f2 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1157,7 +1157,7 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
> int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> struct vmem_altmap *altmap)
> {
> - WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
> + WARN_ON_ONCE((start < VMEMMAP_START) || (end > VMEMMAP_END));
>
> if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
> return vmemmap_populate_basepages(start, end, node, altmap);
> @@ -1169,7 +1169,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> void vmemmap_free(unsigned long start, unsigned long end,
> struct vmem_altmap *altmap)
> {
> - WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
> + WARN_ON_ONCE((start < VMEMMAP_START) || (end > VMEMMAP_END));
>
> unmap_hotplug_range(start, end, true, altmap);
> free_empty_tables(start, end, VMEMMAP_START, VMEMMAP_END);
>
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>

2023-04-10 07:53:19

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On 4/7/23 03:45, Ard Biesheuvel wrote:
...
> That is what I am talking about - the struct pages are allocated in a
> region that is reserved for something else.
>
> Maybe an example helps here:

It does! After also poking around quite a lot, and comparing to x86,
it is starting to become clearer now.

>
> When running the 39-bit VA kernel build on a AMD Seatte board, we will
> have (assuming sizeof(struct page) == 64)
>
> memstart_addr := 0x80_0000_0000
>
> PAGE_OFFSET := 0xffff_ff80_0000_0000
>
> VMEMMAP_SHIFT := 6
> VMEMMAP_START := 0xffff_fffe_0000_0000
> VMEMMAP_SIZE := 0x1_0000_0000
>
> pfn_to_page() conversions are based on ordinary array indexing
> involving vemmap[], where vmemmap is defined as
>
> #define vmemmap \
> ((struct page *)VMEMMAP_START - (memstart_addr >> PAGE_SHIFT))
>
> So the PFN associated with the first usable DRAM address is
> 0x800_0000, and pfn_to_page(0x800_0000) will return VMEMMAP_START.

OK, I see how that's set up, yes.

>
> pfn_to_page(x) for any x < 0x800_0000 will produce a kernel VA that
> points into the vmalloc area, and may conflict with the kernel
> mapping, modules mappings, per-CPU mappings, IO mappings, etc etc.
>
> pfn_to_page(x) for values 0xc00_0000 < x < 0x1000_0000 will produce a
> kernel VA that points outside the region set aside for the vmemmap.
> This region is currently unused, but that will likely change soon.
>

I tentatively think I'm in this case right now. Because there is no wrap
around happening in my particular config, which is CONFIG_ARM64_VA_BITS
== 48, and PAGE_SIZE == 4KB and sizeof(struct page) == 64 (details
below).

It occurs to me that ZONE_DEVICE and (within that category) device
private page support need only support rather large setups. On x86, it
requires 64-bit. And on arm64, from what I'm learning after a day or so
of looking around and comparing, I think we must require at least 48 bit
VA support. Otherwise there's just no room for things.

And for smaller systems, everyone disables this fancy automatic handling
(hmm_range_fault()-based page migration) anyway, partly because of the
VA and PA small ranges, but also because of size and performance
constraints.

> pfn_to_page(x) for any x >= 0x1000_0000 will wrap around and produce a
> bogus address in the user range.
>
> The bottom line is that the VMEMMAP region is dimensioned to cover
> system memory only, i.e., what can be covered by the kernel direct
> map. If you want to allocate struct pages for thing that are not
> system memory, you will need to enlarge the VMEMMAP region, and ensure
> that request_mem_region() produces a region that is covered by it.
>
> This is going to be tricky with LPA2, because there, the 4k pages
> configuration already uses up half of the vmalloc region to cover the
> linear map, so we have to consider this carefully.

Things are interlocked a little differently on arm64, than on x86, and
the layout is also different. One other interesting thing jumps out at
me: On arm64, the (VMALLOC_END - VMALLOC_START) size is *huge*: 123 TB
on my config. And it seems to cover the kernel mapping. On x86, those
are separate. This still confuses me a bit and I wonder if I'm reading
it wrong?

Also, below are the values on my 48 bit VA setup. I'm listing these in
order to help jumpstart thinking about how exactly to extend
VMEMMAP_SIZE. GPUs have on the order of GB's of memory these days, so
that's the order of magnitude that's needed.

PAGE_OFFSET: 0xffff000000000000
PAGE_END: 0xffff800000000000
high_memory: 0xffff087f80000000 (8 TB)

VMALLOC_START: 0xffff800008000000
VMALLOC_END: 0xfffffbfff0000000 (123 TB)

vmemmap: 0xfffffbfffe000000
VMEMMAP_START: 0xfffffc0000000000
VMEMMAP_END: 0xfffffe0000000000

Typical device private struct page
that is causing warnings: 0xffffffffaee00000

VMEMMAP_SIZE: 0x0000020000000000 (2 TB)
VMEMMAP_SHIFT: 6

PHYS_OFFSET: 0x0000000080000000
memstart_addr (signed 64-bit): 0x0000000080000000

MODULES_VADDR: 0xffff800000000000
MODULES_END: 0xffff800008000000 (128 MB)

PAGE_SHIFT: 12
PAGE_SIZE: 0x0000000000001000 (4 KB)
PAGE_MASK: 0xfffffffffffff000

PMD_SIZE: 0x0000000000200000 (2 MB)
PMD_MASK: 0xffffffffffe00000

PUD_SIZE: 0x0000000040000000 (1 GB)
PUD_MASK: 0xffffffffc0000000

PGDIR_SIZE: 0x0000008000000000 (512 GB)

PTE_ADDR_MASK: 0x0000fffffffff000
sizeof(struct page): 64

thanks,
--
John Hubbard
NVIDIA

2023-04-11 02:56:55

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On 4/10/23 00:39, John Hubbard wrote:
>> pfn_to_page(x) for values 0xc00_0000 < x < 0x1000_0000 will produce a
>> kernel VA that points outside the region set aside for the vmemmap.
>> This region is currently unused, but that will likely change soon.
>>
>
> I tentatively think I'm in this case right now. Because there is no wrap
> around happening in my particular config, which is CONFIG_ARM64_VA_BITS
> == 48, and PAGE_SIZE == 4KB and sizeof(struct page) == 64 (details
> below).
>

Correction, actually it *is* wrapping around, and ending up as a bogus
user space address, as you said it would when being above the range:

page_to_pfn(0xffffffffaee00000): 0x0000000ffec38000


> It occurs to me that ZONE_DEVICE and (within that category) device
> private page support need only support rather large setups. On x86, it
> requires 64-bit. And on arm64, from what I'm learning after a day or so
> of looking around and comparing, I think we must require at least 48 bit
> VA support. Otherwise there's just no room for things.

I'm still not sure of how to make room, but working on it.


thanks,
--
John Hubbard
NVIDIA

2023-05-12 14:53:49

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On Tue, 11 Apr 2023 at 04:48, John Hubbard <[email protected]> wrote:
>
> On 4/10/23 00:39, John Hubbard wrote:
> >> pfn_to_page(x) for values 0xc00_0000 < x < 0x1000_0000 will produce a
> >> kernel VA that points outside the region set aside for the vmemmap.
> >> This region is currently unused, but that will likely change soon.
> >>
> >
> > I tentatively think I'm in this case right now. Because there is no wrap
> > around happening in my particular config, which is CONFIG_ARM64_VA_BITS
> > == 48, and PAGE_SIZE == 4KB and sizeof(struct page) == 64 (details
> > below).
> >
>
> Correction, actually it *is* wrapping around, and ending up as a bogus
> user space address, as you said it would when being above the range:
>
> page_to_pfn(0xffffffffaee00000): 0x0000000ffec38000
>

Interesting.

>
> > It occurs to me that ZONE_DEVICE and (within that category) device
> > private page support need only support rather large setups. On x86, it
> > requires 64-bit. And on arm64, from what I'm learning after a day or so
> > of looking around and comparing, I think we must require at least 48 bit
> > VA support. Otherwise there's just no room for things.
>
> I'm still not sure of how to make room, but working on it.
>

The assumption that only the linear map needs to be covered by struct
pages is rather fundamental to the arm64 mm code, as far as I am
aware.

Given that these device memory regions have no direct correspondence
with the linear map at all, couldn't we simply vmalloc() a range of
memory for struct pages for such a region and wire that up in the
existing code? That seems far more maintainable to me than
reorganizing the entire kernel VA space, and only for some choices for
the dimensions.

2023-05-13 02:37:39

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] arm64/mm: don't WARN when alloc/free-ing device private pages

On 5/12/23 07:42, Ard Biesheuvel wrote:
>> I'm still not sure of how to make room, but working on it.
>>
>
> The assumption that only the linear map needs to be covered by struct
> pages is rather fundamental to the arm64 mm code, as far as I am
> aware.
>
> Given that these device memory regions have no direct correspondence
> with the linear map at all, couldn't we simply vmalloc() a range of
> memory for struct pages for such a region and wire that up in the
> existing code? That seems far more maintainable to me than
> reorganizing the entire kernel VA space, and only for some choices for
> the dimensions.

The vmalloc approach does sound like it should Just Work, yes. I'll try it out.

And now I'm trying to remember why Jerome didn't use that approach for x86
originally. If this fixes HMM on arm64, I'll revisit that question too.

Really appreciate the help and advice here.


thanks,
--
John Hubbard
NVIDIA