memblock_is_region_memory() vs. memblock_is_region_reserved()

memblock_is_region_memory() vs. memblock_is_region_reserved()

[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.

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?

diff --git a/mm/memblock.c b/mm/memblock.c
index a44eab3..f097e79 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -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);

Re: memblock_is_region_memory() vs. memblock_is_region_reserved()

[Tejun Heo]

On Mon, May 14, 2012 at 06:11:20PM -0700, Stephen Boyd wrote:
> 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()?

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.

> 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.

-- 
tejun

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

[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.

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 <tj@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 mm/memblock.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index a44eab3..e35e39b 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -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);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Stephen Boyd]

On 05/24/12 00:45, Stephen Boyd wrote:
> 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 <tj@kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---

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

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[tip-bot for Stephen Boyd]

Commit-ID:  eab309494ae2b9e15f85520f00de3893162c2e43
Gitweb:     http://git.kernel.org/tip/eab309494ae2b9e15f85520f00de3893162c2e43
Author:     Stephen Boyd <sboyd@codeaurora.org>
AuthorDate: Thu, 24 May 2012 00:45:21 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
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 <sboyd@codeaurora.org>
Cc: Tejun Heo <tj@kernel.org>
Link: http://lkml.kernel.org/r/1337845521-32755-1-git-send-email-sboyd@codeaurora.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 mm/memblock.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index 952123e..32a0a5e 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -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);