Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On Mon, Apr 02, 2018 at 05:17:35PM +0800, Jia He wrote:
>
>
>On 4/2/2018 4:12 PM, Wei Yang Wrote:
>> On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
>> >
>> > On 3/28/2018 5:18 PM, Wei Yang Wrote:
>> > > Oops, I should reply this thread. Forget about the reply on another
>> > > thread.
>> > >
>> > > On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>> > > > Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>> > > > where possible") optimized the loop in memmap_init_zone(). But it
>> > > > causes
>> > > > possible panic bug. So Daniel Vacek reverted it later.
>> > > >
>> > > Why this has a bug? Do you have some link about it?
>> > >
>> > > If the audience could know the potential risk, it would be helpful to
>> > > review
>> > > the code and decide whether to take it back.
>> > Hi Wei
>> > Paul firstly submit a commit b92df1de5 to improve the loop in
>> > memmap_init_zone.
>> > And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
>> > because
>> > there is evidence that this bug_on was caused by b92df1de5 [1].
>> >
>> > But things didn't get better, 864b75f9d6b caused booting hang issue on
>> > arm{64} [2]
>> > So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
>> >
>> > [1] https://patchwork.kernel.org/patch/10251145/
>> > [2] https://lkml.org/lkml/2018/3/14/469
>> I took some time to look into the discussion, while the root cause seems not
>> clear now?
>>
>Frankly speaking, to me the root cause of that bug_on is not completedly
>clear :-) Daniel ever gave me some hints as followed, but currently I have
>no x86 platform to understand the details.
>
>"On arm and arm64, memblock is used by default. But generic version of
>pfn_valid() is based on mem sections and memblock_next_valid_pfn()
>does not always return the next valid one but skips more resulting in
>some valid frames to be skipped (as if they were invalid). And that's
>why kernel was eventually crashing on some !arm machines."
>
This means a system with memblock is safe to use this function?
As I know, mem_section is based on memblock, so in which case
memblock_next_valid_pfn() skips a valid pfn? A little confused.
>--
>Cheers,
>Jia
--
Wei Yang
Help you, Help me
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On Mon, Apr 02, 2018 at 05:17:35PM +0800, Jia He wrote:
>
>
>On 4/2/2018 4:12 PM, Wei Yang Wrote:
>> On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
>> >
>> > On 3/28/2018 5:18 PM, Wei Yang Wrote:
>> > > Oops, I should reply this thread. Forget about the reply on another
>> > > thread.
>> > >
>> > > On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>> > > > Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>> > > > where possible") optimized the loop in memmap_init_zone(). But it
>> > > > causes
>> > > > possible panic bug. So Daniel Vacek reverted it later.
>> > > >
>> > > Why this has a bug? Do you have some link about it?
>> > >
>> > > If the audience could know the potential risk, it would be helpful to
>> > > review
>> > > the code and decide whether to take it back.
>> > Hi Wei
>> > Paul firstly submit a commit b92df1de5 to improve the loop in
>> > memmap_init_zone.
>> > And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
>> > because
>> > there is evidence that this bug_on was caused by b92df1de5 [1].
>> >
>> > But things didn't get better, 864b75f9d6b caused booting hang issue on
>> > arm{64} [2]
>> > So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
>> >
>> > [1] https://patchwork.kernel.org/patch/10251145/
>> > [2] https://lkml.org/lkml/2018/3/14/469
>> I took some time to look into the discussion, while the root cause seems not
>> clear now?
>>
>Frankly speaking, to me the root cause of that bug_on is not completedly
>clear :-) Daniel ever gave me some hints as followed, but currently I have
>no x86 platform to understand the details.
>
>"On arm and arm64, memblock is used by default. But generic version of
>pfn_valid() is based on mem sections and memblock_next_valid_pfn()
>does not always return the next valid one but skips more resulting in
>some valid frames to be skipped (as if they were invalid). And that's
>why kernel was eventually crashing on some !arm machines."
>
This means a system with memblock is safe to use this function?
As I know, mem_section is based on memblock, so in which case
memblock_next_valid_pfn() skips a valid pfn? A little confused.
>--
>Cheers,
>Jia
--
Wei Yang
Help you, Help me
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On 4/2/2018 4:12 PM, Wei Yang Wrote:
On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
On 3/28/2018 5:18 PM, Wei Yang Wrote:
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
Hi Wei
Paul firstly submit a commit b92df1de5 to improve the loop in
memmap_init_zone.
And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
because
there is evidence that this bug_on was caused by b92df1de5 [1].
But things didn't get better, 864b75f9d6b caused booting hang issue on
arm{64} [2]
So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
[1] https://patchwork.kernel.org/patch/10251145/
[2] https://lkml.org/lkml/2018/3/14/469
I took some time to look into the discussion, while the root cause seems not
clear now?
Frankly speaking, to me the root cause of that bug_on is not completedly
clear :-) Daniel ever gave me some hints as followed, but currently I have
no x86 platform to understand the details.
"On arm and arm64, memblock is used by default. But generic version of
pfn_valid() is based on mem sections and memblock_next_valid_pfn()
does not always return the next valid one but skips more resulting in
some valid frames to be skipped (as if they were invalid). And that's
why kernel was eventually crashing on some !arm machines."
--
Cheers,
Jia
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On 4/2/2018 4:12 PM, Wei Yang Wrote:
On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
On 3/28/2018 5:18 PM, Wei Yang Wrote:
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
Hi Wei
Paul firstly submit a commit b92df1de5 to improve the loop in
memmap_init_zone.
And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
because
there is evidence that this bug_on was caused by b92df1de5 [1].
But things didn't get better, 864b75f9d6b caused booting hang issue on
arm{64} [2]
So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
[1] https://patchwork.kernel.org/patch/10251145/
[2] https://lkml.org/lkml/2018/3/14/469
I took some time to look into the discussion, while the root cause seems not
clear now?
Frankly speaking, to me the root cause of that bug_on is not completedly
clear :-) Daniel ever gave me some hints as followed, but currently I have
no x86 platform to understand the details.
"On arm and arm64, memblock is used by default. But generic version of
pfn_valid() is based on mem sections and memblock_next_valid_pfn()
does not always return the next valid one but skips more resulting in
some valid frames to be skipped (as if they were invalid). And that's
why kernel was eventually crashing on some !arm machines."
--
Cheers,
Jia
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
>
>
>On 3/28/2018 5:18 PM, Wei Yang Wrote:
>> Oops, I should reply this thread. Forget about the reply on another thread.
>>
>> On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>> > Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>> > where possible") optimized the loop in memmap_init_zone(). But it causes
>> > possible panic bug. So Daniel Vacek reverted it later.
>> >
>> Why this has a bug? Do you have some link about it?
>>
>> If the audience could know the potential risk, it would be helpful to review
>> the code and decide whether to take it back.
>Hi Wei
>Paul firstly submit a commit b92df1de5 to improve the loop in
>memmap_init_zone.
>And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
>because
>there is evidence that this bug_on was caused by b92df1de5 [1].
>
>But things didn't get better, 864b75f9d6b caused booting hang issue on
>arm{64} [2]
>So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
>
>[1] https://patchwork.kernel.org/patch/10251145/
>[2] https://lkml.org/lkml/2018/3/14/469
I took some time to look into the discussion, while the root cause seems not
clear now?
--
Wei Yang
Help you, Help me
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On Wed, Mar 28, 2018 at 05:49:23PM +0800, Jia He wrote:
>
>
>On 3/28/2018 5:18 PM, Wei Yang Wrote:
>> Oops, I should reply this thread. Forget about the reply on another thread.
>>
>> On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>> > Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>> > where possible") optimized the loop in memmap_init_zone(). But it causes
>> > possible panic bug. So Daniel Vacek reverted it later.
>> >
>> Why this has a bug? Do you have some link about it?
>>
>> If the audience could know the potential risk, it would be helpful to review
>> the code and decide whether to take it back.
>Hi Wei
>Paul firstly submit a commit b92df1de5 to improve the loop in
>memmap_init_zone.
>And Daniel tried to fix a bug_on panic issue on X86 in commit 864b75f9d6b
>because
>there is evidence that this bug_on was caused by b92df1de5 [1].
>
>But things didn't get better, 864b75f9d6b caused booting hang issue on
>arm{64} [2]
>So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
>
>[1] https://patchwork.kernel.org/patch/10251145/
>[2] https://lkml.org/lkml/2018/3/14/469
I took some time to look into the discussion, while the root cause seems not
clear now?
--
Wei Yang
Help you, Help me
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On 3/28/2018 5:18 PM, Wei Yang Wrote:
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
Hi Wei
Paul firstly submit a commit b92df1de5 to improve the loop in
memmap_init_zone.
And Daniel tried to fix a bug_on panic issue on X86 in commit
864b75f9d6b because
there is evidence that this bug_on was caused by b92df1de5 [1].
But things didn't get better, 864b75f9d6b caused booting hang issue on
arm{64} [2]
So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
[1] https://patchwork.kernel.org/patch/10251145/
[2] https://lkml.org/lkml/2018/3/14/469
But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
enable. And as verified by Eugeniu Rosca, arm can benifit from this
commit. So remain the memblock_next_valid_pfn.
Signed-off-by: Jia He
---
include/linux/memblock.h | 4
mm/memblock.c| 29 +
mm/page_alloc.c | 11 ++-
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 0257aee..efbbe4b 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long
*out_start_pfn,
i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long memblock_next_valid_pfn(unsigned long pfn);
+#endif
+
/**
* for_each_free_mem_range - iterate through free memblock areas
* @i: u64 used as loop variable
diff --git a/mm/memblock.c b/mm/memblock.c
index ba7c878..bea5a9c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
nid,
*out_nid = r->nid;
}
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
+{
+ struct memblock_type *type =
+ unsigned int right = type->cnt;
+ unsigned int mid, left = 0;
+ phys_addr_t addr = PFN_PHYS(++pfn);
+
+ do {
+ mid = (right + left) / 2;
+
+ if (addr < type->regions[mid].base)
+ right = mid;
+ else if (addr >= (type->regions[mid].base +
+ type->regions[mid].size))
+ left = mid + 1;
+ else {
+ /* addr is within the region, so pfn is valid */
+ return pfn;
+ }
+ } while (left < right);
+
+ if (right == type->cnt)
+ return -1UL;
+ else
+ return PHYS_PFN(type->regions[right].base);
+}
+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
+
/**
* memblock_set_node - set node ID on memblock regions
* @base: base of area to set node ID for
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c19f5ac..2a967f7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
nid, unsigned long zone,
if (context != MEMMAP_EARLY)
goto not_early;
- if (!early_pfn_valid(pfn))
+ if (!early_pfn_valid(pfn)) {
+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
In commit b92df1de5d28, it use CONFIG_HAVE_MEMBLOCK_NODE_MAP.
Not get the point of your change.
Please get more information about the reason why using
CONFIG_HAVE_MEMBLOCK in
d49d47e mm: page_alloc: skip over regions of invalid pfns on UMA
And this commit is dependent on b92df1de, so it is also reverted.
Cheers,
Jia
+ /*
+* Skip to the pfn preceding the next valid one (or
+* end_pfn), such that we hit a valid pfn (or end_pfn)
+* on our next iteration of the loop.
+*/
+ pfn = memblock_next_valid_pfn(pfn) - 1;
+#endif
continue;
+ }
if (!early_pfn_in_nid(pfn, nid))
continue;
if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
--
2.7.4
--
Cheers,
Jia
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
On 3/28/2018 5:18 PM, Wei Yang Wrote:
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
Hi Wei
Paul firstly submit a commit b92df1de5 to improve the loop in
memmap_init_zone.
And Daniel tried to fix a bug_on panic issue on X86 in commit
864b75f9d6b because
there is evidence that this bug_on was caused by b92df1de5 [1].
But things didn't get better, 864b75f9d6b caused booting hang issue on
arm{64} [2]
So maintainer decided to reverted both b92df1de5 and 864b75f9d6b
[1] https://patchwork.kernel.org/patch/10251145/
[2] https://lkml.org/lkml/2018/3/14/469
But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
enable. And as verified by Eugeniu Rosca, arm can benifit from this
commit. So remain the memblock_next_valid_pfn.
Signed-off-by: Jia He
---
include/linux/memblock.h | 4
mm/memblock.c| 29 +
mm/page_alloc.c | 11 ++-
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 0257aee..efbbe4b 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long
*out_start_pfn,
i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long memblock_next_valid_pfn(unsigned long pfn);
+#endif
+
/**
* for_each_free_mem_range - iterate through free memblock areas
* @i: u64 used as loop variable
diff --git a/mm/memblock.c b/mm/memblock.c
index ba7c878..bea5a9c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
nid,
*out_nid = r->nid;
}
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
+{
+ struct memblock_type *type =
+ unsigned int right = type->cnt;
+ unsigned int mid, left = 0;
+ phys_addr_t addr = PFN_PHYS(++pfn);
+
+ do {
+ mid = (right + left) / 2;
+
+ if (addr < type->regions[mid].base)
+ right = mid;
+ else if (addr >= (type->regions[mid].base +
+ type->regions[mid].size))
+ left = mid + 1;
+ else {
+ /* addr is within the region, so pfn is valid */
+ return pfn;
+ }
+ } while (left < right);
+
+ if (right == type->cnt)
+ return -1UL;
+ else
+ return PHYS_PFN(type->regions[right].base);
+}
+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
+
/**
* memblock_set_node - set node ID on memblock regions
* @base: base of area to set node ID for
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c19f5ac..2a967f7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
nid, unsigned long zone,
if (context != MEMMAP_EARLY)
goto not_early;
- if (!early_pfn_valid(pfn))
+ if (!early_pfn_valid(pfn)) {
+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
In commit b92df1de5d28, it use CONFIG_HAVE_MEMBLOCK_NODE_MAP.
Not get the point of your change.
Please get more information about the reason why using
CONFIG_HAVE_MEMBLOCK in
d49d47e mm: page_alloc: skip over regions of invalid pfns on UMA
And this commit is dependent on b92df1de, so it is also reverted.
Cheers,
Jia
+ /*
+* Skip to the pfn preceding the next valid one (or
+* end_pfn), such that we hit a valid pfn (or end_pfn)
+* on our next iteration of the loop.
+*/
+ pfn = memblock_next_valid_pfn(pfn) - 1;
+#endif
continue;
+ }
if (!early_pfn_in_nid(pfn, nid))
continue;
if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
--
2.7.4
--
Cheers,
Jia
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>where possible") optimized the loop in memmap_init_zone(). But it causes
>possible panic bug. So Daniel Vacek reverted it later.
>
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
>But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
>enable. And as verified by Eugeniu Rosca, arm can benifit from this
>commit. So remain the memblock_next_valid_pfn.
>
>Signed-off-by: Jia He
>---
> include/linux/memblock.h | 4
> mm/memblock.c| 29 +
> mm/page_alloc.c | 11 ++-
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/memblock.h b/include/linux/memblock.h
>index 0257aee..efbbe4b 100644
>--- a/include/linux/memblock.h
>+++ b/include/linux/memblock.h
>@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned
>long *out_start_pfn,
>i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
> #endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
>
>+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
>+unsigned long memblock_next_valid_pfn(unsigned long pfn);
>+#endif
>+
> /**
> * for_each_free_mem_range - iterate through free memblock areas
> * @i: u64 used as loop variable
>diff --git a/mm/memblock.c b/mm/memblock.c
>index ba7c878..bea5a9c 100644
>--- a/mm/memblock.c
>+++ b/mm/memblock.c
>@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
>nid,
> *out_nid = r->nid;
> }
>
>+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
>+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
>+{
>+ struct memblock_type *type =
>+ unsigned int right = type->cnt;
>+ unsigned int mid, left = 0;
>+ phys_addr_t addr = PFN_PHYS(++pfn);
>+
>+ do {
>+ mid = (right + left) / 2;
>+
>+ if (addr < type->regions[mid].base)
>+ right = mid;
>+ else if (addr >= (type->regions[mid].base +
>+type->regions[mid].size))
>+ left = mid + 1;
>+ else {
>+ /* addr is within the region, so pfn is valid */
>+ return pfn;
>+ }
>+ } while (left < right);
>+
>+ if (right == type->cnt)
>+ return -1UL;
>+ else
>+ return PHYS_PFN(type->regions[right].base);
>+}
>+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
>+
> /**
> * memblock_set_node - set node ID on memblock regions
> * @base: base of area to set node ID for
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index c19f5ac..2a967f7 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
>nid, unsigned long zone,
> if (context != MEMMAP_EARLY)
> goto not_early;
>
>- if (!early_pfn_valid(pfn))
>+ if (!early_pfn_valid(pfn)) {
>+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
In commit b92df1de5d28, it use CONFIG_HAVE_MEMBLOCK_NODE_MAP.
Not get the point of your change.
>+ /*
>+ * Skip to the pfn preceding the next valid one (or
>+ * end_pfn), such that we hit a valid pfn (or end_pfn)
>+ * on our next iteration of the loop.
>+ */
>+ pfn = memblock_next_valid_pfn(pfn) - 1;
>+#endif
> continue;
>+ }
> if (!early_pfn_in_nid(pfn, nid))
> continue;
> if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
>--
>2.7.4
--
Wei Yang
Help you, Help me
Re: [PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
Oops, I should reply this thread. Forget about the reply on another thread.
On Sun, Mar 25, 2018 at 08:02:15PM -0700, Jia He wrote:
>Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
>where possible") optimized the loop in memmap_init_zone(). But it causes
>possible panic bug. So Daniel Vacek reverted it later.
>
Why this has a bug? Do you have some link about it?
If the audience could know the potential risk, it would be helpful to review
the code and decide whether to take it back.
>But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
>enable. And as verified by Eugeniu Rosca, arm can benifit from this
>commit. So remain the memblock_next_valid_pfn.
>
>Signed-off-by: Jia He
>---
> include/linux/memblock.h | 4
> mm/memblock.c| 29 +
> mm/page_alloc.c | 11 ++-
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/memblock.h b/include/linux/memblock.h
>index 0257aee..efbbe4b 100644
>--- a/include/linux/memblock.h
>+++ b/include/linux/memblock.h
>@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned
>long *out_start_pfn,
>i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
> #endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
>
>+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
>+unsigned long memblock_next_valid_pfn(unsigned long pfn);
>+#endif
>+
> /**
> * for_each_free_mem_range - iterate through free memblock areas
> * @i: u64 used as loop variable
>diff --git a/mm/memblock.c b/mm/memblock.c
>index ba7c878..bea5a9c 100644
>--- a/mm/memblock.c
>+++ b/mm/memblock.c
>@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
>nid,
> *out_nid = r->nid;
> }
>
>+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
>+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
>+{
>+ struct memblock_type *type =
>+ unsigned int right = type->cnt;
>+ unsigned int mid, left = 0;
>+ phys_addr_t addr = PFN_PHYS(++pfn);
>+
>+ do {
>+ mid = (right + left) / 2;
>+
>+ if (addr < type->regions[mid].base)
>+ right = mid;
>+ else if (addr >= (type->regions[mid].base +
>+type->regions[mid].size))
>+ left = mid + 1;
>+ else {
>+ /* addr is within the region, so pfn is valid */
>+ return pfn;
>+ }
>+ } while (left < right);
>+
>+ if (right == type->cnt)
>+ return -1UL;
>+ else
>+ return PHYS_PFN(type->regions[right].base);
>+}
>+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
>+
> /**
> * memblock_set_node - set node ID on memblock regions
> * @base: base of area to set node ID for
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index c19f5ac..2a967f7 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
>nid, unsigned long zone,
> if (context != MEMMAP_EARLY)
> goto not_early;
>
>- if (!early_pfn_valid(pfn))
>+ if (!early_pfn_valid(pfn)) {
>+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
In commit b92df1de5d28, it use CONFIG_HAVE_MEMBLOCK_NODE_MAP.
Not get the point of your change.
>+ /*
>+ * Skip to the pfn preceding the next valid one (or
>+ * end_pfn), such that we hit a valid pfn (or end_pfn)
>+ * on our next iteration of the loop.
>+ */
>+ pfn = memblock_next_valid_pfn(pfn) - 1;
>+#endif
> continue;
>+ }
> if (!early_pfn_in_nid(pfn, nid))
> continue;
> if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
>--
>2.7.4
--
Wei Yang
Help you, Help me
[PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
enable. And as verified by Eugeniu Rosca, arm can benifit from this
commit. So remain the memblock_next_valid_pfn.
Signed-off-by: Jia He
---
include/linux/memblock.h | 4
mm/memblock.c| 29 +
mm/page_alloc.c | 11 ++-
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 0257aee..efbbe4b 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long
*out_start_pfn,
i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long memblock_next_valid_pfn(unsigned long pfn);
+#endif
+
/**
* for_each_free_mem_range - iterate through free memblock areas
* @i: u64 used as loop variable
diff --git a/mm/memblock.c b/mm/memblock.c
index ba7c878..bea5a9c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
nid,
*out_nid = r->nid;
}
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
+{
+ struct memblock_type *type =
+ unsigned int right = type->cnt;
+ unsigned int mid, left = 0;
+ phys_addr_t addr = PFN_PHYS(++pfn);
+
+ do {
+ mid = (right + left) / 2;
+
+ if (addr < type->regions[mid].base)
+ right = mid;
+ else if (addr >= (type->regions[mid].base +
+ type->regions[mid].size))
+ left = mid + 1;
+ else {
+ /* addr is within the region, so pfn is valid */
+ return pfn;
+ }
+ } while (left < right);
+
+ if (right == type->cnt)
+ return -1UL;
+ else
+ return PHYS_PFN(type->regions[right].base);
+}
+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
+
/**
* memblock_set_node - set node ID on memblock regions
* @base: base of area to set node ID for
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c19f5ac..2a967f7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
nid, unsigned long zone,
if (context != MEMMAP_EARLY)
goto not_early;
- if (!early_pfn_valid(pfn))
+ if (!early_pfn_valid(pfn)) {
+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
+ /*
+* Skip to the pfn preceding the next valid one (or
+* end_pfn), such that we hit a valid pfn (or end_pfn)
+* on our next iteration of the loop.
+*/
+ pfn = memblock_next_valid_pfn(pfn) - 1;
+#endif
continue;
+ }
if (!early_pfn_in_nid(pfn, nid))
continue;
if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
--
2.7.4
[PATCH v3 1/5] mm: page_alloc: remain memblock_next_valid_pfn() when CONFIG_HAVE_ARCH_PFN_VALID is enable
Commit b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns
where possible") optimized the loop in memmap_init_zone(). But it causes
possible panic bug. So Daniel Vacek reverted it later.
But memblock_next_valid_pfn is valid when CONFIG_HAVE_ARCH_PFN_VALID is
enable. And as verified by Eugeniu Rosca, arm can benifit from this
commit. So remain the memblock_next_valid_pfn.
Signed-off-by: Jia He
---
include/linux/memblock.h | 4
mm/memblock.c| 29 +
mm/page_alloc.c | 11 ++-
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 0257aee..efbbe4b 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -203,6 +203,10 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long
*out_start_pfn,
i >= 0; __next_mem_pfn_range(, nid, p_start, p_end, p_nid))
#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long memblock_next_valid_pfn(unsigned long pfn);
+#endif
+
/**
* for_each_free_mem_range - iterate through free memblock areas
* @i: u64 used as loop variable
diff --git a/mm/memblock.c b/mm/memblock.c
index ba7c878..bea5a9c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1102,6 +1102,35 @@ void __init_memblock __next_mem_pfn_range(int *idx, int
nid,
*out_nid = r->nid;
}
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+unsigned long __init_memblock memblock_next_valid_pfn(unsigned long pfn)
+{
+ struct memblock_type *type =
+ unsigned int right = type->cnt;
+ unsigned int mid, left = 0;
+ phys_addr_t addr = PFN_PHYS(++pfn);
+
+ do {
+ mid = (right + left) / 2;
+
+ if (addr < type->regions[mid].base)
+ right = mid;
+ else if (addr >= (type->regions[mid].base +
+ type->regions[mid].size))
+ left = mid + 1;
+ else {
+ /* addr is within the region, so pfn is valid */
+ return pfn;
+ }
+ } while (left < right);
+
+ if (right == type->cnt)
+ return -1UL;
+ else
+ return PHYS_PFN(type->regions[right].base);
+}
+#endif /*CONFIG_HAVE_ARCH_PFN_VALID*/
+
/**
* memblock_set_node - set node ID on memblock regions
* @base: base of area to set node ID for
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c19f5ac..2a967f7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5483,8 +5483,17 @@ void __meminit memmap_init_zone(unsigned long size, int
nid, unsigned long zone,
if (context != MEMMAP_EARLY)
goto not_early;
- if (!early_pfn_valid(pfn))
+ if (!early_pfn_valid(pfn)) {
+#if (defined CONFIG_HAVE_MEMBLOCK) && (defined CONFIG_HAVE_ARCH_PFN_VALID)
+ /*
+* Skip to the pfn preceding the next valid one (or
+* end_pfn), such that we hit a valid pfn (or end_pfn)
+* on our next iteration of the loop.
+*/
+ pfn = memblock_next_valid_pfn(pfn) - 1;
+#endif
continue;
+ }
if (!early_pfn_in_nid(pfn, nid))
continue;
if (!update_defer_init(pgdat, pfn, end_pfn, _initialised))
--
2.7.4
