Skip to contentSkip to conversation

memblock_is_region_memory() vs. memblock_is_region_reserved()

PATCH In review Stephen Boyd 3 participants 5 messages May 15, 2012 – Jun 8, 2012

Subsystems (2)
Memory management memblock and memory management initialization
lore
Share & tools
lore

Stephen Boyd

author
#0 lore raw

I was looking at memblock and noticed that memblock_is_region_memory() is implemented differently than memblock_is_region_reserved().

memblock_is_region_memory() returns true only if the region you are testing is fully contained within the bounds of a memory block. If the region is half in and half out of a memory region (i.e. overlaps memory and a hole) it is not memory and returns false.

memblock_is_region_reserved() is the opposite. If the region are you are testing is half in and half out of a reserved region (i.e. overlaps reserved memory) it returns true, otherwise it returns false.

These functions sound like they do the same thing by testing to see if what you specify is either memory or reserved memory, but the semantics are a bit different in that the former doesn't allow overlap, and the latter accepts overlap. Should we rename memblock_is_region_reserved() to memblock_overlaps_reserved() to make it clearer what the intention is? Or perhaps rename memblock_is_region_memory() to memblock_is_region_within_memory()?

If anything, perhaps this patch will help clarify things?

mm/memblock.c +20 -0
index a44eab3..f097e79 100644
@@ -855,6 +855,16 @@ int __init_memblock memblock_is_memory(phys_addr_t addr)
 	return memblock_search(&memblock.memory, addr) != -1;
 }
 
+/**
+ * memblock_is_region_memory - check if a region is a subset of memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) is a subset of a memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size)
 {
 	int idx = memblock_search(&memblock.memory, base);

@@ -867,6 +877,16 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size
 		 memblock.memory.regions[idx].size) >= end;
 }
 
+/**
+ * memblock_is_region_reserved - check if a region intersects reserved memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) intersects a reserved memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_reserved(phys_addr_t base, phys_addr_t size)
 {
 	memblock_cap_size(base, &size);
#1 lore raw
Quoted 12 lines · Stephen Boyd

I was looking at memblock and noticed that memblock_is_region_memory() is implemented differently than memblock_is_region_reserved().

memblock_is_region_memory() returns true only if the region you are testing is fully contained within the bounds of a memory block. If the region is half in and half out of a memory region (i.e. overlaps memory and a hole) it is not memory and returns false.

memblock_is_region_reserved() is the opposite. If the region are you are testing is half in and half out of a reserved region (i.e. overlaps reserved memory) it returns true, otherwise it returns false.

Quoted 7 lines

These functions sound like they do the same thing by testing to see if what you specify is either memory or reserved memory, but the semantics are a bit different in that the former doesn't allow overlap, and the latter accepts overlap. Should we rename memblock_is_region_reserved() to memblock_overlaps_reserved() to make it clearer what the intention is? Or perhaps rename memblock_is_region_memory() to memblock_is_region_within_memory()?

The difference is there because for a memory region to be available, all of it should be while a memory region partially reserved should still be treated as reserved.

Quoted 1 line

If anything, perhaps this patch will help clarify things?

Yeah, looks good to me. Can you please add patch description, sign-off and send it to Ingo?

Thanks.

Signature · 1 line
tejun
#2 lore raw

PATCH 1/1 memblock: Document memblock_is_region_{memory,reserved}()

At first glance one would think that memblock_is_region_memory() and memblock_is_region_reserved() would be implemented in the same way. Unfortunately they aren't and the former returns whether the region specified is a subset of a memory bank while the latter returns whether the region specified intersects with reserved memory.

Document the two functions so that users aren't tempted to make the implementation the same between them and to clarify the purpose of the functions.

Cc Tejun Heo <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 mm/memblock.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
mm/memblock.c +20 -0
index a44eab3..e35e39b 100644
@@ -855,6 +855,16 @@ int __init_memblock memblock_is_memory(phys_addr_t addr)
 	return memblock_search(&memblock.memory, addr) != -1;
 }
 
+/**
+ * memblock_is_region_memory - check if a region is a subset of memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) is a subset of a memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size)
 {
 	int idx = memblock_search(&memblock.memory, base);

@@ -867,6 +877,16 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size
 		 memblock.memory.regions[idx].size) >= end;
 }
 
+/**
+ * memblock_is_region_reserved - check if a region intersects reserved memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) intersects a reserved memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_reserved(phys_addr_t base, phys_addr_t size)
 {
 	memblock_cap_size(base, &size);
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#3 lore raw

Re: [PATCH] memblock: Document memblock_is_region_{memory,reserved}()

Quoted 7 lines · Stephen Boyd

At first glance one would think that memblock_is_region_memory() and memblock_is_region_reserved() would be implemented in the same way. Unfortunately they aren't and the former returns whether the region specified is a subset of a memory bank while the latter returns whether the region specified intersects with reserved memory.

Quoted 7 lines

Document the two functions so that users aren't tempted to make the implementation the same between them and to clarify the purpose of the functions.

Cc Tejun Heo <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>

---

Hi Ingo, can you pick up or review this patch (I realize it was sent during the merge window)?

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#4 lore raw

[tip:core/urgent] memblock: Document memblock_is_region_{memory, reserved}()

In reply to Stephen Boyd
Commit-ID:  eab309494ae2b9e15f85520f00de3893162c2e43
Gitweb:     http://git.kernel.org/tip/eab309494ae2b9e15f85520f00de3893162c2e43
Author:     Stephen Boyd <[email protected]>
AuthorDate: Thu, 24 May 2012 00:45:21 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 8 Jun 2012 11:59:46 +0200

memblock: Document memblock_is_region_{memory,reserved}()

At first glance one would think that memblock_is_region_memory() and memblock_is_region_reserved() would be implemented in the same way. Unfortunately they aren't and the former returns whether the region specified is a subset of a memory bank while the latter returns whether the region specified intersects with reserved memory.

Document the two functions so that users aren't tempted to make the implementation the same between them and to clarify the purpose of the functions.

Signed-off-by Stephen Boyd <[email protected]>
Cc Tejun Heo <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 mm/memblock.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
mm/memblock.c +20 -0
index 952123e..32a0a5e 100644
@@ -867,6 +867,16 @@ int __init_memblock memblock_is_memory(phys_addr_t addr)
 	return memblock_search(&memblock.memory, addr) != -1;
 }
 
+/**
+ * memblock_is_region_memory - check if a region is a subset of memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) is a subset of a memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size)
 {
 	int idx = memblock_search(&memblock.memory, base);

@@ -879,6 +889,16 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size
 		 memblock.memory.regions[idx].size) >= end;
 }
 
+/**
+ * memblock_is_region_reserved - check if a region intersects reserved memory
+ * @base: base of region to check
+ * @size: size of region to check
+ *
+ * Check if the region [@base, @base+@size) intersects a reserved memory block.
+ *
+ * RETURNS:
+ * 0 if false, non-zero if true
+ */
 int __init_memblock memblock_is_region_reserved(phys_addr_t base, phys_addr_t size)
 {
 	memblock_cap_size(base, &size);