From: Julien Grall <jgrall@amazon.com>

free_heap_pages() has an ASSERT() checking that node is >= 0. However
node is defined as an unsigned int. So it cannot be negative.

Therefore remove the check as it will always be true.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

I have looked at the history. AFAICT, node has always be defined
as unsigned int. So the ASSERT() may have never been useful (?).
---
 xen/common/page_alloc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 46357182375a..e971bf91e0be 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1441,7 +1441,6 @@ static void free_heap_pages(
     unsigned int zone = page_to_zone(pg);
 
     ASSERT(order <= MAX_ORDER);
-    ASSERT(node >= 0);
 
     spin_lock(&heap_lock);
 
-- 
2.32.0

On 23/02/2022 18:38, Julien Grall wrote:

From: Julien Grall <jgrall@amazon.com><mailto:jgrall@amazon.com>

free_heap_pages() has an ASSERT() checking that node is >= 0. However
node is defined as an unsigned int. So it cannot be negative.

Therefore remove the check as it will always be true.


Coverity-ID: 1055631


Signed-off-by: Julien Grall <jgrall@amazon.com><mailto:jgrall@amazon.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com><mailto:andrew.cooper3@citrix.com>

Hi Andrew,

On 23/02/2022 19:30, Andrew Cooper wrote:
> On 23/02/2022 18:38, Julien Grall wrote:
>> From: Julien Grall<jgrall@amazon.com>
>>
>> free_heap_pages() has an ASSERT() checking that node is >= 0. However
>> node is defined as an unsigned int. So it cannot be negative.
>>
>> Therefore remove the check as it will always be true.
> 
> Coverity-ID: 1055631

I will add it while committing.

> 
>> Signed-off-by: Julien Grall<jgrall@amazon.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks!

Cheers,

-- 
Julien Grall

On 23.02.2022 19:38, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> free_heap_pages() has an ASSERT() checking that node is >= 0. However
> node is defined as an unsigned int. So it cannot be negative.
> 
> Therefore remove the check as it will always be true.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> I have looked at the history. AFAICT, node has always be defined
> as unsigned int. So the ASSERT() may have never been useful (?).

Commit f0738d2d3f81 introduced "node" as a local variable of type
"int". Along with this commit f1c6ac275100 introduced ia64's
paddr_to_nid() (backing phys_to_nid()), which was able to return -1.
Hence at the time the assertion fulfilled a purpose. I should have
dropped it in bd3e1195d694.

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Hi Jan,

On 24/02/2022 08:27, Jan Beulich wrote:
> On 23.02.2022 19:38, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> free_heap_pages() has an ASSERT() checking that node is >= 0. However
>> node is defined as an unsigned int. So it cannot be negative.
>>
>> Therefore remove the check as it will always be true.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>> I have looked at the history. AFAICT, node has always be defined
>> as unsigned int. So the ASSERT() may have never been useful (?).
> 
> Commit f0738d2d3f81 introduced "node" as a local variable of type
> "int". Along with this commit f1c6ac275100 introduced ia64's
> paddr_to_nid() (backing phys_to_nid()), which was able to return -1.
> Hence at the time the assertion fulfilled a purpose. I should have
> dropped it in bd3e1195d694.

Thanks for the information. It looks like I need to brush my git-blame 
skill :).

> 
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Cheers,

-- 
Julien Grall