mm/hotplug: remove unnecessary BUG_ON in __offline_pages()

PATCH In review Xishi Qiu 3 participants 4 messages Jul 30, 2013 – Aug 1, 2013

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

Xishi Qiu

author

2013-07-30 06:49 UTC #0 lore raw

PATCH 1/1 mm/hotplug: remove unnecessary BUG_ON in __offline_pages()

I think we can remove "BUG_ON(start_pfn >= end_pfn)" in __offline_pages(), because in memory_block_action() "nr_pages = PAGES_PER_SECTION * sections_per_block" is always greater than 0.

memory_block_action()

	offline_pages()
		__offline_pages()
			BUG_ON(start_pfn >= end_pfn)

Signed-off-by Xishi Qiu <[email protected]>

---
 mm/memory_hotplug.c | 1 -
 1 file changed, 1 deletion(-)

mm/memory_hotplug.c +0 -1

index ca1dd3a..8e333f9 100644
@@ -1472,7 +1472,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 
-	BUG_ON(start_pfn >= end_pfn);
 	/* at least, alignment against pageblock is necessary */
 	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
 		return -EINVAL;

Signature · 1 line

1.8.2.2

Toshi Kani

2013-07-30 15:40 UTC #1 lore raw

Quoted 3 lines · Xishi Qiu

I think we can remove "BUG_ON(start_pfn >= end_pfn)" in __offline_pages(), because in memory_block_action() "nr_pages = PAGES_PER_SECTION * sections_per_block" is always greater than 0.

BUG_ON() is used for checking a condition that should never happen, unless there is a bug. So, to me, what you described seems to match with the use of BUG_ON() to prevent a potential bug in the caller.

Thanks,
-Toshi

Quoted 15 lines

memory_block_action()
	offline_pages()
		__offline_pages()
			BUG_ON(start_pfn >= end_pfn)
Signed-off-by Xishi Qiu <[email protected]>
---
 mm/memory_hotplug.c | 1 -
 1 file changed, 1 deletion(-)
mm/memory_hotplug.c +0 -0
index ca1dd3a..8e333f9 100644

mm/memory_hotplug.c static int __ref __offline_pages(unsigned long start_pfn, · line 1477

Quoted 7 lines

mm/memory_hotplug.c +0 -1

@@ -1472,7 +1472,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 
-	BUG_ON(start_pfn >= end_pfn);
 	/* at least, alignment against pageblock is necessary */
 	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
 		return -EINVAL;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dave Hansen

Reviewed-by on v1 1/1

2013-07-31 16:56 UTC #2 lore raw

In reply to Xishi Qiu

Quoted 3 lines · Xishi Qiu

I think we can remove "BUG_ON(start_pfn >= end_pfn)" in __offline_pages(), because in memory_block_action() "nr_pages = PAGES_PER_SECTION * sections_per_block" is always greater than 0.

...

mm/memory_hotplug.c static int __ref __offline_pages(unsigned long start_pfn, · line 1477

Quoted 10 lines

mm/memory_hotplug.c +0 -1

@@ -1472,7 +1472,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 
-	BUG_ON(start_pfn >= end_pfn);
 	/* at least, alignment against pageblock is necessary */
 	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
 		return -EINVAL;

I think you're saying that you don't see a way to hit this BUG_ON() in practice. That does appear to be true, unless sections_per_block ended up 0 or negative. The odds of getting in to this code if 'sections_per_block' was bogus are pretty small.

Or, is this a theoretical thing that folks might run in to when adding new features or developing? It's in a cold path and the cost of the check is miniscule. The original author (cc'd) also saw a need to put this in probably because he actually ran in to this.

In any case, it looks fairly safe to me:

Reviewed-by Dave Hansen <[email protected]>

Xishi Qiu

author

2013-08-01 01:35 UTC #3 lore raw

Quoted 11 lines · Dave Hansen

Quoted 3 lines · Xishi Qiu
I think we can remove "BUG_ON(start_pfn >= end_pfn)" in __offline_pages(), because in memory_block_action() "nr_pages = PAGES_PER_SECTION * sections_per_block" is always greater than 0.
...
Quoted 6 lines
mm/memory_hotplug.c +0 -0
@@ -1472,7 +1472,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 

Quoted 10 lines

mm/memory_hotplug.c static int __ref __offline_pages(unsigned long start_pfn, · line 1477
Quoted 4 lines
mm/memory_hotplug.c +0 -1
-	BUG_ON(start_pfn >= end_pfn);
 	/* at least, alignment against pageblock is necessary */
 	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
 		return -EINVAL;
I think you're saying that you don't see a way to hit this BUG_ON() in practice. That does appear to be true, unless sections_per_block ended up 0 or negative. The odds of getting in to this code if 'sections_per_block' was bogus are pretty small.

Yes, I find there is an only to hit this BUG_ON() in v3.11, and "sections_per_block" seems to be always greater than 0.

Quoted 5 lines

Or, is this a theoretical thing that folks might run in to when adding new features or developing? It's in a cold path and the cost of the check is miniscule. The original author (cc'd) also saw a need to put this in probably because he actually ran in to this.

In v2.6.32, If info->length==0, this way may hit this BUG_ON(). acpi_memory_disable_device()

	remove_memory(info->start_addr, info->length)
		offline_pages()

Later Fujitsu's patch rename this function and the BUG_ON() is unnecessary.

Thanks,
Xishi Qiu

Quoted 5 lines

In any case, it looks fairly safe to me:
Reviewed-by Dave Hansen <[email protected]>
.