[PATCH 2/5] debugobjects: Remove redundant checks in fill_pool()

Zhen Lei posted 5 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH 2/5] debugobjects: Remove redundant checks in fill_pool()
Posted by Zhen Lei 1 year, 3 months ago
The conditions for the inner and outer loops are exactly the same, so the
outer 'while' should be changed to 'if'. Then we'll see that the second
condition of the new 'if' is already guaranteed above and can be removed.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 lib/debugobjects.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 1ea8af72849cdb1..aba3e62a4315f51 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -142,8 +142,7 @@ static void fill_pool(void)
 	 * READ_ONCE()s pair with the WRITE_ONCE()s in pool_lock critical
 	 * sections.
 	 */
-	while (READ_ONCE(obj_nr_tofree) &&
-	       READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
+	if (READ_ONCE(obj_nr_tofree)) {
 		raw_spin_lock_irqsave(&pool_lock, flags);
 		/*
 		 * Recheck with the lock held as the worker thread might have
-- 
2.34.1
Re: [PATCH 2/5] debugobjects: Remove redundant checks in fill_pool()
Posted by Thomas Gleixner 1 year, 3 months ago
On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
> The conditions for the inner and outer loops are exactly the same, so the
> outer 'while' should be changed to 'if'. Then we'll see that the
> second

We'll see nothing. Please write change logs in passive voice and do not
try to impersonate code.

> condition of the new 'if' is already guaranteed above and can be
> removed.

Yes, the conditions are the same. But a 'if' is not the same as a 'while'.

So you need to explain why the outer loop is not required and why it
does not make a difference in terms of functionality.

> @@ -142,8 +142,7 @@ static void fill_pool(void)
>  	 * READ_ONCE()s pair with the WRITE_ONCE()s in pool_lock critical
>  	 * sections.
>  	 */

The comment does not make sense anymore. Please fixup comments when
changing the code. It's a pain to read a comment and then see that the
code does something different.

> -	while (READ_ONCE(obj_nr_tofree) &&
> -	       READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
> +	if (READ_ONCE(obj_nr_tofree)) {
>  		raw_spin_lock_irqsave(&pool_lock, flags);
>  		/*
>  		 * Recheck with the lock held as the worker thread might have

Thanks,

        tglx
Re: [PATCH 2/5] debugobjects: Remove redundant checks in fill_pool()
Posted by Leizhen (ThunderTown) 1 year, 3 months ago

On 2024/9/3 17:44, Thomas Gleixner wrote:
> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
>> The conditions for the inner and outer loops are exactly the same, so the
>> outer 'while' should be changed to 'if'. Then we'll see that the
>> second
> 
> We'll see nothing. Please write change logs in passive voice and do not
> try to impersonate code.

OK

> 
>> condition of the new 'if' is already guaranteed above and can be
>> removed.
> 
> Yes, the conditions are the same. But a 'if' is not the same as a 'while'.
> 
> So you need to explain why the outer loop is not required and why it
> does not make a difference in terms of functionality.

OK, I'll write a good description in V2.

> 
>> @@ -142,8 +142,7 @@ static void fill_pool(void)
>>  	 * READ_ONCE()s pair with the WRITE_ONCE()s in pool_lock critical
>>  	 * sections.
>>  	 */
> 
> The comment does not make sense anymore. Please fixup comments when
> changing the code. It's a pain to read a comment and then see that the
> code does something different.

OK

> 
>> -	while (READ_ONCE(obj_nr_tofree) &&
>> -	       READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
>> +	if (READ_ONCE(obj_nr_tofree)) {
>>  		raw_spin_lock_irqsave(&pool_lock, flags);
>>  		/*
>>  		 * Recheck with the lock held as the worker thread might have
> 
> Thanks,
> 
>         tglx
> 
> .
> 

-- 
Regards,
  Zhen Lei