[PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Faiyaz Mohammed]

This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
memory region")' is keeping the no-map regions in memblock.memory with
MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
but during the initialization sparse_init mark all memblock.memory as
present using for_each_mem_pfn_range, which is creating the memmap for
no-map memblock regions. To avoid it skiping the memblock.memory regions
set with MEMBLOCK_NOMAP set and with this change we will be able to save
~11MB memory for ~612MB carve out.

With change:
[    0.000000] memblock_alloc_exact_nid_raw: 115343360 bytes align=0x200000 nid=0 from=0x0000000080000000 max_addr=0x0000000000000000 sparse_buffer_init+0x60/0x8c
[    0.000000] memblock_reserve: [0x0000000932c00000-0x00000009399fffff] memblock_alloc_range_nid+0xbc/0x1a0
[    0.000000] On node 0 totalpages: 1627824
[    0.000000] DMA32 zone: 5383 pages used for memmap
[    0.000000] Normal zone: 20052 pages used for memmap

Without change:
[    0.000000] memblock_alloc_exact_nid_raw: 117440512 bytes align=0x200000 nid=0 from=0x0000000080000000 max_addr=0x0000000000000000 sparse_buffer_init+0x60/0x8c
[    0.000000] memblock_reserve: [0x0000000932a00000-0x00000009399fffff] memblock_alloc_range_nid+0xbc/0x1a0
[    0.000000] On node 0 totalpages: 1788416
[    0.000000] DMA32 zone: 8192 pages used for memmap
[    0.000000] Normal zone: 20052 pages used for memmap.

Signed-off-by: Faiyaz Mohammed <quic_faiyazm@quicinc.com>
---
 mm/memblock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 2270e3c..a4fde9d 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1175,8 +1175,13 @@ void __init_memblock __next_mem_pfn_range(int *idx, int nid,
 		r = &type->regions[*idx];
 		r_nid = memblock_get_region_node(r);
 
+		/* If memblock region is nomap then skip the region. */
+		if (memblock_is_nomap(r))
+			continue;
+
 		if (PFN_UP(r->base) >= PFN_DOWN(r->base + r->size))
 			continue;
+
 		if (nid == MAX_NUMNODES || nid == r_nid)
 			break;
 	}
-- 
2.7.4

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> memory region")' is keeping the no-map regions in memblock.memory with
> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> but during the initialization sparse_init mark all memblock.memory as
> present using for_each_mem_pfn_range, which is creating the memmap for
> no-map memblock regions. To avoid it skiping the memblock.memory regions
> set with MEMBLOCK_NOMAP set and with this change we will be able to save
> ~11MB memory for ~612MB carve out.

The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
really don't like the idea if adding more implicit assumptions about how
NOMAP memory may or may not be used in a generic iterator function.

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Faiyaz Mohammed]

On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
>> memory region")' is keeping the no-map regions in memblock.memory with
>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
>> but during the initialization sparse_init mark all memblock.memory as
>> present using for_each_mem_pfn_range, which is creating the memmap for
>> no-map memblock regions. To avoid it skiping the memblock.memory regions
>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
>> ~11MB memory for ~612MB carve out.
> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> really don't like the idea if adding more implicit assumptions about how
> NOMAP memory may or may not be used in a generic iterator function.

Sorry for delayed response.
Yes, it is possible that implicit assumption can create
misunderstanding. How about adding command line option and control the
no-map region in fdt.c driver, to decide whether to keep "no-map" region
with NOMAP flag or remove?. Something like below

--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1180,8 +1180,10 @@ int __init __weak
early_init_dt_reserve_memory_arch(phys_addr_t base,
                 */
                if (memblock_is_region_reserved(base, size))
                        return -EBUSY;
-
-               return memblock_mark_nomap(base, size);
+               if (remove_nomap_region)
+                       return memblock_remove(base, size);
+               else
+                       return memblock_mark_nomap(base, size);
Thanks and regards,
Mohammed Faiyaz

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Rob Herring]

On Thu, May 5, 2022 at 10:17 AM Faiyaz Mohammed
<quic_faiyazm@quicinc.com> wrote:
>
>
> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> > On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> >> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> >> memory region")' is keeping the no-map regions in memblock.memory with
> >> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> >> but during the initialization sparse_init mark all memblock.memory as
> >> present using for_each_mem_pfn_range, which is creating the memmap for
> >> no-map memblock regions. To avoid it skiping the memblock.memory regions
> >> set with MEMBLOCK_NOMAP set and with this change we will be able to save
> >> ~11MB memory for ~612MB carve out.
> > The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> > really don't like the idea if adding more implicit assumptions about how
> > NOMAP memory may or may not be used in a generic iterator function.
>
> Sorry for delayed response.
> Yes, it is possible that implicit assumption can create
> misunderstanding. How about adding command line option and control the
> no-map region in fdt.c driver, to decide whether to keep "no-map" region
> with NOMAP flag or remove?. Something like below

No. That just added another dimension to the test matrix. Having
things from multiple sources is always a mess.

>
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1180,8 +1180,10 @@ int __init __weak
> early_init_dt_reserve_memory_arch(phys_addr_t base,
>                  */
>                 if (memblock_is_region_reserved(base, size))
>                         return -EBUSY;
> -
> -               return memblock_mark_nomap(base, size);
> +               if (remove_nomap_region)
> +                       return memblock_remove(base, size);
> +               else
> +                       return memblock_mark_nomap(base, size);
> Thanks and regards,
> Mohammed Faiyaz
>

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> 
> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> > On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> >> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> >> memory region")' is keeping the no-map regions in memblock.memory with
> >> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> >> but during the initialization sparse_init mark all memblock.memory as
> >> present using for_each_mem_pfn_range, which is creating the memmap for
> >> no-map memblock regions. To avoid it skiping the memblock.memory regions
> >> set with MEMBLOCK_NOMAP set and with this change we will be able to save
> >> ~11MB memory for ~612MB carve out.
> > The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> > really don't like the idea if adding more implicit assumptions about how
> > NOMAP memory may or may not be used in a generic iterator function.
> 
> Sorry for delayed response.
> Yes, it is possible that implicit assumption can create
> misunderstanding. How about adding command line option and control the
> no-map region in fdt.c driver, to decide whether to keep "no-map" region
> with NOMAP flag or remove?. Something like below

I really don't like memblock_remove() for such cases.
Pretending there is a hole when there is an actual DRAM makes things really
hairy when it comes to memory map and page allocator initialization.
You wouldn't want to trade system stability and random memory corruptions
for 11M of "saved" memory.
 
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1180,8 +1180,10 @@ int __init __weak
> early_init_dt_reserve_memory_arch(phys_addr_t base,
>                  */
>                 if (memblock_is_region_reserved(base, size))
>                         return -EBUSY;
> -
> -               return memblock_mark_nomap(base, size);
> +               if (remove_nomap_region)
> +                       return memblock_remove(base, size);
> +               else
> +                       return memblock_mark_nomap(base, size);
> Thanks and regards,
> Mohammed Faiyaz
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Faiyaz Mohammed]

On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
>> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
>>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
>>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
>>>> memory region")' is keeping the no-map regions in memblock.memory with
>>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
>>>> but during the initialization sparse_init mark all memblock.memory as
>>>> present using for_each_mem_pfn_range, which is creating the memmap for
>>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
>>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
>>>> ~11MB memory for ~612MB carve out.
>>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
>>> really don't like the idea if adding more implicit assumptions about how
>>> NOMAP memory may or may not be used in a generic iterator function.
>> Sorry for delayed response.
>> Yes, it is possible that implicit assumption can create
>> misunderstanding. How about adding command line option and control the
>> no-map region in fdt.c driver, to decide whether to keep "no-map" region
>> with NOMAP flag or remove?. Something like below
> I really don't like memblock_remove() for such cases.
> Pretending there is a hole when there is an actual DRAM makes things really
> hairy when it comes to memory map and page allocator initialization.
> You wouldn't want to trade system stability and random memory corruptions
> for 11M of "saved" memory.
Creating memory map for holes memory is adding 11MB overhead which is
huge on low

memory target and same time 11MB memory saving is good enough on low
memory target.

Or we can have separate list of NOMAP like reserved?.

Any other suggestion to address this issue?.

Thanks and regards,
Mohammed Faiyaz

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
> 
> On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> > On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> >> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> >>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> >>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> >>>> memory region")' is keeping the no-map regions in memblock.memory with
> >>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> >>>> but during the initialization sparse_init mark all memblock.memory as
> >>>> present using for_each_mem_pfn_range, which is creating the memmap for
> >>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
> >>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
> >>>> ~11MB memory for ~612MB carve out.
> >>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> >>> really don't like the idea if adding more implicit assumptions about how
> >>> NOMAP memory may or may not be used in a generic iterator function.
> >> Sorry for delayed response.
> >> Yes, it is possible that implicit assumption can create
> >> misunderstanding. How about adding command line option and control the
> >> no-map region in fdt.c driver, to decide whether to keep "no-map" region
> >> with NOMAP flag or remove?. Something like below
> > I really don't like memblock_remove() for such cases.
> > Pretending there is a hole when there is an actual DRAM makes things really
> > hairy when it comes to memory map and page allocator initialization.
> > You wouldn't want to trade system stability and random memory corruptions
> > for 11M of "saved" memory.
>
> Creating memory map for holes memory is adding 11MB overhead which is
> huge on low memory target and same time 11MB memory saving is good enough
> on low memory target.
> 
> Or we can have separate list of NOMAP like reserved?.
> 
> Any other suggestion to address this issue?.

Make your firmware to report the memory that Linux cannot use as a hole,
i.e. _not_ report it as memory.
 
> Thanks and regards,
> Mohammed Faiyaz

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Vijayanand Jitta]

On 5/9/2022 5:12 PM, Mike Rapoport wrote:
> On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
>>
>> On 5/5/2022 10:24 PM, Mike Rapoport wrote:
>>> On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
>>>> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
>>>>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
>>>>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
>>>>>> memory region")' is keeping the no-map regions in memblock.memory with
>>>>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
>>>>>> but during the initialization sparse_init mark all memblock.memory as
>>>>>> present using for_each_mem_pfn_range, which is creating the memmap for
>>>>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
>>>>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
>>>>>> ~11MB memory for ~612MB carve out.
>>>>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
>>>>> really don't like the idea if adding more implicit assumptions about how
>>>>> NOMAP memory may or may not be used in a generic iterator function.
>>>> Sorry for delayed response.
>>>> Yes, it is possible that implicit assumption can create
>>>> misunderstanding. How about adding command line option and control the
>>>> no-map region in fdt.c driver, to decide whether to keep "no-map" region
>>>> with NOMAP flag or remove?. Something like below
>>> I really don't like memblock_remove() for such cases.
>>> Pretending there is a hole when there is an actual DRAM makes things really
>>> hairy when it comes to memory map and page allocator initialization.
>>> You wouldn't want to trade system stability and random memory corruptions
>>> for 11M of "saved" memory.
>>
>> Creating memory map for holes memory is adding 11MB overhead which is
>> huge on low memory target and same time 11MB memory saving is good enough
>> on low memory target.
>>
>> Or we can have separate list of NOMAP like reserved?.
>>
>> Any other suggestion to address this issue?.
> 
> Make your firmware to report the memory that Linux cannot use as a hole,
> i.e. _not_ report it as memory.
>  

Thanks, Mike for the comments.

Few concerns with this approach.

1) One concern is, even if firmware doesn't report these regions as
memory, we would need addresses for these to be part of device tree so
that the clients would be able to get these addresses. Otherwise there
is no way for client to know these addresses.

2) This would also add a dependency on firmware to be able to pass these
regions not as memory, though we know that these regions would be used
by the clients. Isn't it better to have such control within the kernel ?

Let me know your comments on these.

Thanks,
Vijay
>> Thanks and regards,
>> Mohammed Faiyaz
> 

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

Hi Vijay,

On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
> 
> On 5/9/2022 5:12 PM, Mike Rapoport wrote:
> > On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
> >>
> >> On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> >>> On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> >>>> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> >>>>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> >>>>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> >>>>>> memory region")' is keeping the no-map regions in memblock.memory with
> >>>>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> >>>>>> but during the initialization sparse_init mark all memblock.memory as
> >>>>>> present using for_each_mem_pfn_range, which is creating the memmap for
> >>>>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
> >>>>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
> >>>>>> ~11MB memory for ~612MB carve out.
> >>>>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> >>>>> really don't like the idea if adding more implicit assumptions about how
> >>>>> NOMAP memory may or may not be used in a generic iterator function.
> >>>> Sorry for delayed response.
> >>>> Yes, it is possible that implicit assumption can create
> >>>> misunderstanding. How about adding command line option and control the
> >>>> no-map region in fdt.c driver, to decide whether to keep "no-map" region
> >>>> with NOMAP flag or remove?. Something like below
> >>> I really don't like memblock_remove() for such cases.
> >>> Pretending there is a hole when there is an actual DRAM makes things really
> >>> hairy when it comes to memory map and page allocator initialization.
> >>> You wouldn't want to trade system stability and random memory corruptions
> >>> for 11M of "saved" memory.
> >>
> >> Creating memory map for holes memory is adding 11MB overhead which is
> >> huge on low memory target and same time 11MB memory saving is good enough
> >> on low memory target.
> >>
> >> Or we can have separate list of NOMAP like reserved?.
> >>
> >> Any other suggestion to address this issue?.
> > 
> > Make your firmware to report the memory that Linux cannot use as a hole,
> > i.e. _not_ report it as memory.
> >  
> 
> Thanks, Mike for the comments.
> 
> Few concerns with this approach.
> 
> 1) One concern is, even if firmware doesn't report these regions as
> memory, we would need addresses for these to be part of device tree so
> that the clients would be able to get these addresses. Otherwise there
> is no way for client to know these addresses.
> 
> 2) This would also add a dependency on firmware to be able to pass these
> regions not as memory, though we know that these regions would be used
> by the clients. Isn't it better to have such control within the kernel ?

If it is memory that is used by the kernel it should be reported as memory
and have the memory map. 
If this is a hole in the memory layout from the kernel perspective, then
kernel should not bother with this memory.

And I'm not buying "low memory target" argument if you have enough memory
to carve out ~600M for some mysterious clients.
 
> Let me know your comments on these.
> 
> Thanks,
> Vijay

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Aiqun Yu (Maria)]

On 8/6/2022 3:22 AM, Mike Rapoport wrote:
> Hi Vijay,
> 
> On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
>>
>> On 5/9/2022 5:12 PM, Mike Rapoport wrote:
>>> On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
>>>>
>>>> On 5/5/2022 10:24 PM, Mike Rapoport wrote:
>>>>> On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
>>>>>> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
>>>>>>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
>>>>>>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
>>>>>>>> memory region")' is keeping the no-map regions in memblock.memory with
>>>>>>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
>>>>>>>> but during the initialization sparse_init mark all memblock.memory as
>>>>>>>> present using for_each_mem_pfn_range, which is creating the memmap for
>>>>>>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
>>>>>>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
>>>>>>>> ~11MB memory for ~612MB carve out.
>>>>>>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
>>>>>>> really don't like the idea if adding more implicit assumptions about how
>>>>>>> NOMAP memory may or may not be used in a generic iterator function.
>>>>>> Sorry for delayed response.
>>>>>> Yes, it is possible that implicit assumption can create
>>>>>> misunderstanding. How about adding command line option and control the
>>>>>> no-map region in fdt.c driver, to decide whether to keep "no-map" region
>>>>>> with NOMAP flag or remove?. Something like below
>>>>> I really don't like memblock_remove() for such cases.
>>>>> Pretending there is a hole when there is an actual DRAM makes things really
>>>>> hairy when it comes to memory map and page allocator initialization.
>>>>> You wouldn't want to trade system stability and random memory corruptions
>>>>> for 11M of "saved" memory.
>>>>
>>>> Creating memory map for holes memory is adding 11MB overhead which is
>>>> huge on low memory target and same time 11MB memory saving is good enough
>>>> on low memory target.
>>>>
>>>> Or we can have separate list of NOMAP like reserved?.
>>>>
>>>> Any other suggestion to address this issue?.
>>>
>>> Make your firmware to report the memory that Linux cannot use as a hole,
>>> i.e. _not_ report it as memory.
>>>   
>>
>> Thanks, Mike for the comments.
>>
>> Few concerns with this approach.
>>
>> 1) One concern is, even if firmware doesn't report these regions as
>> memory, we would need addresses for these to be part of device tree so
>> that the clients would be able to get these addresses. Otherwise there
>> is no way for client to know these addresses.
>>
>> 2) This would also add a dependency on firmware to be able to pass these
>> regions not as memory, though we know that these regions would be used
>> by the clients. Isn't it better to have such control within the kernel ?
> 
> If it is memory that is used by the kernel it should be reported as memory
> and have the memory map.
> If this is a hole in the memory layout from the kernel perspective, then
> kernel should not bother with this memory.
Hi Mike,

We've put effort on bootloader side to implement the similar suggestion 
of os bootloader to convey the reserved memory by omit the hole from 
/memory@0{reg=[]} directly.
While there is a concern from device tree spec perspective, link [1]: "A 
memory device node is required for all devicetrees and describes the 
physical memory layout for the system. "
Do you have any idea on this pls?

[1] 
https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst
> 
> And I'm not buying "low memory target" argument if you have enough memory
> to carve out ~600M for some mysterious clients.
Just for your information, for low memory target, the carve out can be 
more than ~60M out of 128M in total.
>   
>> Let me know your comments on these.
>>
>> Thanks,
>> Vijay
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Thu, Feb 08, 2024 at 02:37:25PM +0800, Aiqun Yu (Maria) wrote:
> 
> On 8/6/2022 3:22 AM, Mike Rapoport wrote:
> > Hi Vijay,
> > 
> > On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
> > > 
> > > On 5/9/2022 5:12 PM, Mike Rapoport wrote:
> > > > On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
> > > > > 
> > > > > On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> > > > > > On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> > > > > > > On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> > > > > > > > On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> > > > > > > > > This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> > > > > > > > > memory region")' is keeping the no-map regions in memblock.memory with
> > > > > > > > > MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> > > > > > > > > but during the initialization sparse_init mark all memblock.memory as
> > > > > > > > > present using for_each_mem_pfn_range, which is creating the memmap for
> > > > > > > > > no-map memblock regions. To avoid it skiping the memblock.memory regions
> > > > > > > > > set with MEMBLOCK_NOMAP set and with this change we will be able to save
> > > > > > > > > ~11MB memory for ~612MB carve out.
> > > > > > > > The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> > > > > > > > really don't like the idea if adding more implicit assumptions about how
> > > > > > > > NOMAP memory may or may not be used in a generic iterator function.
> > > > > > > Sorry for delayed response.
> > > > > > > Yes, it is possible that implicit assumption can create
> > > > > > > misunderstanding. How about adding command line option and control the
> > > > > > > no-map region in fdt.c driver, to decide whether to keep "no-map" region
> > > > > > > with NOMAP flag or remove?. Something like below
> > > > > > I really don't like memblock_remove() for such cases.
> > > > > > Pretending there is a hole when there is an actual DRAM makes things really
> > > > > > hairy when it comes to memory map and page allocator initialization.
> > > > > > You wouldn't want to trade system stability and random memory corruptions
> > > > > > for 11M of "saved" memory.
> > > > > 
> > > > > Creating memory map for holes memory is adding 11MB overhead which is
> > > > > huge on low memory target and same time 11MB memory saving is good enough
> > > > > on low memory target.
> > > > > 
> > > > > Or we can have separate list of NOMAP like reserved?.
> > > > > 
> > > > > Any other suggestion to address this issue?.
> > > > 
> > > > Make your firmware to report the memory that Linux cannot use as a hole,
> > > > i.e. _not_ report it as memory.
> > > 
> > > Thanks, Mike for the comments.
> > > 
> > > Few concerns with this approach.
> > > 
> > > 1) One concern is, even if firmware doesn't report these regions as
> > > memory, we would need addresses for these to be part of device tree so
> > > that the clients would be able to get these addresses. Otherwise there
> > > is no way for client to know these addresses.
> > > 
> > > 2) This would also add a dependency on firmware to be able to pass these
> > > regions not as memory, though we know that these regions would be used
> > > by the clients. Isn't it better to have such control within the kernel ?
> > 
> > If it is memory that is used by the kernel it should be reported as memory
> > and have the memory map.
> > If this is a hole in the memory layout from the kernel perspective, then
> > kernel should not bother with this memory.
> Hi Mike,
> 
> We've put effort on bootloader side to implement the similar suggestion of
> os bootloader to convey the reserved memory by omit the hole from
> /memory@0{reg=[]} directly.
> While there is a concern from device tree spec perspective, link [1]: "A
> memory device node is required for all devicetrees and describes the
> physical memory layout for the system. "
> Do you have any idea on this pls?

I'm not sure I understand your concern. Isn't there a /memory node that
describes the memory available to Linux in your devicetree?
 
> [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst
> > 
> > And I'm not buying "low memory target" argument if you have enough memory
> > to carve out ~600M for some mysterious clients.
>
> Just for your information, for low memory target, the carve out can be more
> than ~60M out of 128M in total.

If saving ~1M of memory map is important, hide the carve out from Linux
entirely.

> > > Let me know your comments on these.
> > > 
> > > Thanks,
> > > Vijay
> 
> -- 
> Thx and BRs,
> Aiqun(Maria) Yu

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Dmitry Baryshkov]

On Wed, 14 Feb 2024 at 09:44, Mike Rapoport <rppt@kernel.org> wrote:
>
> On Thu, Feb 08, 2024 at 02:37:25PM +0800, Aiqun Yu (Maria) wrote:
> >
> > On 8/6/2022 3:22 AM, Mike Rapoport wrote:
> > > Hi Vijay,
> > >
> > > On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
> > > >
> > > > On 5/9/2022 5:12 PM, Mike Rapoport wrote:
> > > > > On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
> > > > > >
> > > > > > On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> > > > > > > On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> > > > > > > > On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> > > > > > > > > On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> > > > > > > > > > This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> > > > > > > > > > memory region")' is keeping the no-map regions in memblock.memory with
> > > > > > > > > > MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> > > > > > > > > > but during the initialization sparse_init mark all memblock.memory as
> > > > > > > > > > present using for_each_mem_pfn_range, which is creating the memmap for
> > > > > > > > > > no-map memblock regions. To avoid it skiping the memblock.memory regions
> > > > > > > > > > set with MEMBLOCK_NOMAP set and with this change we will be able to save
> > > > > > > > > > ~11MB memory for ~612MB carve out.
> > > > > > > > > The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> > > > > > > > > really don't like the idea if adding more implicit assumptions about how
> > > > > > > > > NOMAP memory may or may not be used in a generic iterator function.
> > > > > > > > Sorry for delayed response.
> > > > > > > > Yes, it is possible that implicit assumption can create
> > > > > > > > misunderstanding. How about adding command line option and control the
> > > > > > > > no-map region in fdt.c driver, to decide whether to keep "no-map" region
> > > > > > > > with NOMAP flag or remove?. Something like below
> > > > > > > I really don't like memblock_remove() for such cases.
> > > > > > > Pretending there is a hole when there is an actual DRAM makes things really
> > > > > > > hairy when it comes to memory map and page allocator initialization.
> > > > > > > You wouldn't want to trade system stability and random memory corruptions
> > > > > > > for 11M of "saved" memory.
> > > > > >
> > > > > > Creating memory map for holes memory is adding 11MB overhead which is
> > > > > > huge on low memory target and same time 11MB memory saving is good enough
> > > > > > on low memory target.
> > > > > >
> > > > > > Or we can have separate list of NOMAP like reserved?.
> > > > > >
> > > > > > Any other suggestion to address this issue?.
> > > > >
> > > > > Make your firmware to report the memory that Linux cannot use as a hole,
> > > > > i.e. _not_ report it as memory.
> > > >
> > > > Thanks, Mike for the comments.
> > > >
> > > > Few concerns with this approach.
> > > >
> > > > 1) One concern is, even if firmware doesn't report these regions as
> > > > memory, we would need addresses for these to be part of device tree so
> > > > that the clients would be able to get these addresses. Otherwise there
> > > > is no way for client to know these addresses.
> > > >
> > > > 2) This would also add a dependency on firmware to be able to pass these
> > > > regions not as memory, though we know that these regions would be used
> > > > by the clients. Isn't it better to have such control within the kernel ?
> > >
> > > If it is memory that is used by the kernel it should be reported as memory
> > > and have the memory map.
> > > If this is a hole in the memory layout from the kernel perspective, then
> > > kernel should not bother with this memory.
> > Hi Mike,
> >
> > We've put effort on bootloader side to implement the similar suggestion of
> > os bootloader to convey the reserved memory by omit the hole from
> > /memory@0{reg=[]} directly.
> > While there is a concern from device tree spec perspective, link [1]: "A
> > memory device node is required for all devicetrees and describes the
> > physical memory layout for the system. "
> > Do you have any idea on this pls?
>
> I'm not sure I understand your concern. Isn't there a /memory node that
> describes the memory available to Linux in your devicetree?

That was the question. It looks like your opinion on /memory was that
it describes "memory available to Linux", while device tree spec
defines it as "physical memory layout".

>
> > [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst
> > >
> > > And I'm not buying "low memory target" argument if you have enough memory
> > > to carve out ~600M for some mysterious clients.
> >
> > Just for your information, for low memory target, the carve out can be more
> > than ~60M out of 128M in total.
>
> If saving ~1M of memory map is important, hide the carve out from Linux
> entirely.
>
> > > > Let me know your comments on these.
> > > >
> > > > Thanks,
> > > > Vijay
> >
> > --
> > Thx and BRs,
> > Aiqun(Maria) Yu
>
> --
> Sincerely yours,
> Mike.



-- 
With best wishes
Dmitry

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Wed, Feb 14, 2024 at 10:11:40AM +0200, Dmitry Baryshkov wrote:
> On Wed, 14 Feb 2024 at 09:44, Mike Rapoport <rppt@kernel.org> wrote:
> >
> > On Thu, Feb 08, 2024 at 02:37:25PM +0800, Aiqun Yu (Maria) wrote:
> > >
> > > On 8/6/2022 3:22 AM, Mike Rapoport wrote:
> > > > Hi Vijay,
> > > >
> > > > On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
> > > > >
> > > > > On 5/9/2022 5:12 PM, Mike Rapoport wrote:
> > > > > > On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
> > > > > > >
> > > > > > > On 5/5/2022 10:24 PM, Mike Rapoport wrote:
> > > > > > > > On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
> > > > > > > > > On 4/12/2022 10:56 PM, Mike Rapoport wrote:
> > > > > > > > > > On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
> > > > > > > > > > > This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
> > > > > > > > > > > memory region")' is keeping the no-map regions in memblock.memory with
> > > > > > > > > > > MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
> > > > > > > > > > > but during the initialization sparse_init mark all memblock.memory as
> > > > > > > > > > > present using for_each_mem_pfn_range, which is creating the memmap for
> > > > > > > > > > > no-map memblock regions. To avoid it skiping the memblock.memory regions
> > > > > > > > > > > set with MEMBLOCK_NOMAP set and with this change we will be able to save
> > > > > > > > > > > ~11MB memory for ~612MB carve out.
> > > > > > > > > > The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
> > > > > > > > > > really don't like the idea if adding more implicit assumptions about how
> > > > > > > > > > NOMAP memory may or may not be used in a generic iterator function.
> > > > > > > > > Sorry for delayed response.
> > > > > > > > > Yes, it is possible that implicit assumption can create
> > > > > > > > > misunderstanding. How about adding command line option and control the
> > > > > > > > > no-map region in fdt.c driver, to decide whether to keep "no-map" region
> > > > > > > > > with NOMAP flag or remove?. Something like below
> > > > > > > > I really don't like memblock_remove() for such cases.
> > > > > > > > Pretending there is a hole when there is an actual DRAM makes things really
> > > > > > > > hairy when it comes to memory map and page allocator initialization.
> > > > > > > > You wouldn't want to trade system stability and random memory corruptions
> > > > > > > > for 11M of "saved" memory.
> > > > > > >
> > > > > > > Creating memory map for holes memory is adding 11MB overhead which is
> > > > > > > huge on low memory target and same time 11MB memory saving is good enough
> > > > > > > on low memory target.
> > > > > > >
> > > > > > > Or we can have separate list of NOMAP like reserved?.
> > > > > > >
> > > > > > > Any other suggestion to address this issue?.
> > > > > >
> > > > > > Make your firmware to report the memory that Linux cannot use as a hole,
> > > > > > i.e. _not_ report it as memory.
> > > > >
> > > > > Thanks, Mike for the comments.
> > > > >
> > > > > Few concerns with this approach.
> > > > >
> > > > > 1) One concern is, even if firmware doesn't report these regions as
> > > > > memory, we would need addresses for these to be part of device tree so
> > > > > that the clients would be able to get these addresses. Otherwise there
> > > > > is no way for client to know these addresses.
> > > > >
> > > > > 2) This would also add a dependency on firmware to be able to pass these
> > > > > regions not as memory, though we know that these regions would be used
> > > > > by the clients. Isn't it better to have such control within the kernel ?
> > > >
> > > > If it is memory that is used by the kernel it should be reported as memory
> > > > and have the memory map.
> > > > If this is a hole in the memory layout from the kernel perspective, then
> > > > kernel should not bother with this memory.
> > > Hi Mike,
> > >
> > > We've put effort on bootloader side to implement the similar suggestion of
> > > os bootloader to convey the reserved memory by omit the hole from
> > > /memory@0{reg=[]} directly.
> > > While there is a concern from device tree spec perspective, link [1]: "A
> > > memory device node is required for all devicetrees and describes the
> > > physical memory layout for the system. "
> > > Do you have any idea on this pls?
> >
> > I'm not sure I understand your concern. Isn't there a /memory node that
> > describes the memory available to Linux in your devicetree?
> 
> That was the question. It looks like your opinion on /memory was that
> it describes "memory available to Linux", while device tree spec
> defines it as "physical memory layout".
 
I suggested a workaround that will allow to save memory map for the
carveout.
The memory map is a run time description of the physical memory layout and
core mm relies on availability of struct page for every physical frame.
Having only partial memory map will lead to subtle bugs and crashes, so
it's not an option.

> > > [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst
> 
> -- 
> With best wishes
> Dmitry

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Aiqun Yu (Maria)]

On 2/14/2024 5:15 PM, Mike Rapoport wrote:
> On Wed, Feb 14, 2024 at 10:11:40AM +0200, Dmitry Baryshkov wrote:
>> On Wed, 14 Feb 2024 at 09:44, Mike Rapoport <rppt@kernel.org> wrote:
>>>
>>> On Thu, Feb 08, 2024 at 02:37:25PM +0800, Aiqun Yu (Maria) wrote:
>>>>
>>>> On 8/6/2022 3:22 AM, Mike Rapoport wrote:
>>>>> Hi Vijay,
>>>>>
>>>>> On Wed, Aug 03, 2022 at 04:27:33PM +0530, Vijayanand Jitta wrote:
>>>>>>
>>>>>> On 5/9/2022 5:12 PM, Mike Rapoport wrote:
>>>>>>> On Mon, May 09, 2022 at 04:37:30PM +0530, Faiyaz Mohammed wrote:
>>>>>>>>
>>>>>>>> On 5/5/2022 10:24 PM, Mike Rapoport wrote:
>>>>>>>>> On Thu, May 05, 2022 at 08:46:15PM +0530, Faiyaz Mohammed wrote:
>>>>>>>>>> On 4/12/2022 10:56 PM, Mike Rapoport wrote:
>>>>>>>>>>> On Tue, Apr 12, 2022 at 12:39:32AM +0530, Faiyaz Mohammed wrote:
>>>>>>>>>>>> This 'commit 86588296acbf ("fdt: Properly handle "no-map" field in the
>>>>>>>>>>>> memory region")' is keeping the no-map regions in memblock.memory with
>>>>>>>>>>>> MEMBLOCK_NOMAP flag set to use no-map memory for EFI using memblock api's,
>>>>>>>>>>>> but during the initialization sparse_init mark all memblock.memory as
>>>>>>>>>>>> present using for_each_mem_pfn_range, which is creating the memmap for
>>>>>>>>>>>> no-map memblock regions. To avoid it skiping the memblock.memory regions
>>>>>>>>>>>> set with MEMBLOCK_NOMAP set and with this change we will be able to save
>>>>>>>>>>>> ~11MB memory for ~612MB carve out.
>>>>>>>>>>> The MEMBLOCK_NOMAP is very fragile and caused a lot of issues already. I
>>>>>>>>>>> really don't like the idea if adding more implicit assumptions about how
>>>>>>>>>>> NOMAP memory may or may not be used in a generic iterator function.
>>>>>>>>>> Sorry for delayed response.
>>>>>>>>>> Yes, it is possible that implicit assumption can create
>>>>>>>>>> misunderstanding. How about adding command line option and control the
>>>>>>>>>> no-map region in fdt.c driver, to decide whether to keep "no-map" region
>>>>>>>>>> with NOMAP flag or remove?. Something like below
>>>>>>>>> I really don't like memblock_remove() for such cases.
>>>>>>>>> Pretending there is a hole when there is an actual DRAM makes things really
>>>>>>>>> hairy when it comes to memory map and page allocator initialization.
>>>>>>>>> You wouldn't want to trade system stability and random memory corruptions
>>>>>>>>> for 11M of "saved" memory.
>>>>>>>>
>>>>>>>> Creating memory map for holes memory is adding 11MB overhead which is
>>>>>>>> huge on low memory target and same time 11MB memory saving is good enough
>>>>>>>> on low memory target.
>>>>>>>>
>>>>>>>> Or we can have separate list of NOMAP like reserved?.
>>>>>>>>
>>>>>>>> Any other suggestion to address this issue?.
>>>>>>>
>>>>>>> Make your firmware to report the memory that Linux cannot use as a hole,
>>>>>>> i.e. _not_ report it as memory.
>>>>>>
>>>>>> Thanks, Mike for the comments.
>>>>>>
>>>>>> Few concerns with this approach.
>>>>>>
>>>>>> 1) One concern is, even if firmware doesn't report these regions as
>>>>>> memory, we would need addresses for these to be part of device tree so
>>>>>> that the clients would be able to get these addresses. Otherwise there
>>>>>> is no way for client to know these addresses.
>>>>>>
>>>>>> 2) This would also add a dependency on firmware to be able to pass these
>>>>>> regions not as memory, though we know that these regions would be used
>>>>>> by the clients. Isn't it better to have such control within the kernel ?
>>>>>
>>>>> If it is memory that is used by the kernel it should be reported as memory
>>>>> and have the memory map.
>>>>> If this is a hole in the memory layout from the kernel perspective, then
>>>>> kernel should not bother with this memory.
>>>> Hi Mike,
>>>>
>>>> We've put effort on bootloader side to implement the similar suggestion of
>>>> os bootloader to convey the reserved memory by omit the hole from
>>>> /memory@0{reg=[]} directly.
>>>> While there is a concern from device tree spec perspective, link [1]: "A
>>>> memory device node is required for all devicetrees and describes the
>>>> physical memory layout for the system. "
>>>> Do you have any idea on this pls?
>>>
>>> I'm not sure I understand your concern. Isn't there a /memory node that
>>> describes the memory available to Linux in your devicetree?
>>
>> That was the question. It looks like your opinion on /memory was that
>> it describes "memory available to Linux", while device tree spec
>> defines it as "physical memory layout".
>   
> I suggested a workaround that will allow to save memory map for the
> carveout.
> The memory map is a run time description of the physical memory layout and
> core mm relies on availability of struct page for every physical frame.
> Having only partial memory map will lead to subtle bugs and crashes, so
> it's not an option.
Any idea of a formal solution for this case?
It is a real use case for the commercial device. Memory saving is always 
a good topic for commercial devices. So for a total 128MB memory, ~60MB 
for kernel total available memory, and ~1M free memory saving is 
important from OEM point of view.

There are 3 types of memory:
1. used by firmware and not available to kernel at any time.
Either struct page can be avoided by kernel. Or bootloader not pass this 
part of physical memory was discussed here.
Any good ideas?
2. shared by firmware/subsystem, and can be read/write access by kernel.
Just as it is now. Struct page can be allocated inside kernel and also 
reserved memory for this.
3. freely used by kernel.
Just as it is now.
> 
>>>> [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst
>>
>> -- 
>> With best wishes
>> Dmitry
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu

Re: [PATCH] mm: memblock: avoid to create memmap for memblock nomap regions

[Mike Rapoport]

On Tue, Feb 20, 2024 at 02:28:32PM +0800, Aiqun Yu (Maria) wrote:
> > > > > Hi Mike,
> > > > > 
> > > > > We've put effort on bootloader side to implement the similar suggestion of
> > > > > os bootloader to convey the reserved memory by omit the hole from
> > > > > /memory@0{reg=[]} directly.
> > > > > While there is a concern from device tree spec perspective, link [1]: "A
> > > > > memory device node is required for all devicetrees and describes the
> > > > > physical memory layout for the system. "
> > > > > Do you have any idea on this pls?
> > > > 
> > > > I'm not sure I understand your concern. Isn't there a /memory node that
> > > > describes the memory available to Linux in your devicetree?
> > > 
> > > That was the question. It looks like your opinion on /memory was that
> > > it describes "memory available to Linux", while device tree spec
> > > defines it as "physical memory layout".
> > >
> > I suggested a workaround that will allow to save memory map for the
> > carveout.
> > The memory map is a run time description of the physical memory layout and
> > core mm relies on availability of struct page for every physical frame.
> > Having only partial memory map will lead to subtle bugs and crashes, so
> > it's not an option.
>
> Any idea of a formal solution for this case?
> It is a real use case for the commercial device. Memory saving is always a
> good topic for commercial devices. So for a total 128MB memory, ~60MB for
> kernel total available memory, and ~1M free memory saving is important from
> OEM point of view.
> 
> There are 3 types of memory:
> 1. used by firmware and not available to kernel at any time.
> Either struct page can be avoided by kernel. Or bootloader not pass this
> part of physical memory was discussed here.
> Any good ideas?

As I said, struct page must exist for all physical memory known to kernel.
If hiding the memory that is not available to kernel does not work for you
I don't have other ideas.

> 2. shared by firmware/subsystem, and can be read/write access by kernel.
> Just as it is now. Struct page can be allocated inside kernel and also
> reserved memory for this.
> 3. freely used by kernel.
> Just as it is now.



-- 
Sincerely yours,
Mike.