[PATCH 0/5] debugobjects: Do some minor optimizations, fixes and cleaups

[PATCH 0/5] debugobjects: Do some minor optimizations, fixes and cleaups

[Zhen Lei]

The summary changes of the first two patches is as follows:
 if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
	return;

- while (READ_ONCE(obj_nr_tofree) && (READ_ONCE(obj_pool_free) < obj_pool_min_free)) {
+ if (READ_ONCE(obj_nr_tofree)) {
	raw_spin_lock_irqsave(&pool_lock, flags);
-	while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
+	while (obj_nr_tofree && (obj_pool_free < debug_objects_pool_min_level)) {
		... ...
	}
	raw_spin_unlock_irqrestore(&pool_lock, flags);


Zhen Lei (5):
  debugobjects: Fix the misuse of global variables in fill_pool()
  debugobjects: Remove redundant checks in fill_pool()
  debugobjects: Don't start fill if there are remaining nodes locally
  debugobjects: Use hlist_splice_init() to reduce lock conflicts
  debugobjects: Delete a piece of redundant code

 lib/debugobjects.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Zhen Lei]

The global variable 'obj_pool_min_free' records the lowest historical
value of the number of nodes in the global list 'obj_pool', instead of
being used as the lowest threshold value. This may be a mistake and
should be replaced with variable 'debug_objects_pool_min_level'.

Fixes: d26bf5056fc0 ("debugobjects: Reduce number of pool_lock acquisitions in fill_pool()")
Fixes: 36c4ead6f6df ("debugobjects: Add global free list and the counter")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 lib/debugobjects.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 7cea91e193a8f04..1ea8af72849cdb1 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -142,13 +142,14 @@ 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) < obj_pool_min_free)) {
+	while (READ_ONCE(obj_nr_tofree) &&
+	       READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
 		raw_spin_lock_irqsave(&pool_lock, flags);
 		/*
 		 * Recheck with the lock held as the worker thread might have
 		 * won the race and freed the global free list already.
 		 */
-		while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
+		while (obj_nr_tofree && (obj_pool_free < debug_objects_pool_min_level)) {
 			obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
 			hlist_del(&obj->node);
 			WRITE_ONCE(obj_nr_tofree, obj_nr_tofree - 1);
-- 
2.34.1

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Thomas Gleixner]

On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:

> The global variable 'obj_pool_min_free' records the lowest historical
> value of the number of nodes in the global list 'obj_pool', instead of
> being used as the lowest threshold value. This may be a mistake and

Maybe? It's either a bug or not.

> should be replaced with variable 'debug_objects_pool_min_level'.

And if it's a bug then it has to be replaced.

This misses another minor issue:

static int			obj_pool_min_free = ODEBUG_POOL_SIZE;

static int  __data_racy		debug_objects_pool_min_level __read_mostly
				= ODEBUG_POOL_MIN_LEVEL;

As debug_objects_pool_min_level is the minimum level to keep around and
obj_pool_min_free is a statistics mechanism, __data_racy is misplaced
too. The variables should swap their position, because
debug_objects_pool_min_level is functional, but obj_pool_min_free is
pure stats.

Also debug_objects_pool_min_level and debug_objects_pool_size should
be __ro_after_init.

> Fixes: d26bf5056fc0 ("debugobjects: Reduce number of pool_lock acquisitions in fill_pool()")
> Fixes: 36c4ead6f6df ("debugobjects: Add global free list and the counter")

Nice detective work!

Thanks,

        tglx

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Leizhen (ThunderTown)]

On 2024/9/3 0:22, Thomas Gleixner wrote:
> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
> 
>> The global variable 'obj_pool_min_free' records the lowest historical
>> value of the number of nodes in the global list 'obj_pool', instead of
>> being used as the lowest threshold value. This may be a mistake and
> 
> Maybe? It's either a bug or not.

Yes, it's a bug, but I'm just learning this module, so I'm not confident.

> 
>> should be replaced with variable 'debug_objects_pool_min_level'.
> 
> And if it's a bug then it has to be replaced.

OK, I will update the commit message in V2.

> 
> This misses another minor issue:
> 
> static int			obj_pool_min_free = ODEBUG_POOL_SIZE;
> 
> static int  __data_racy		debug_objects_pool_min_level __read_mostly
> 				= ODEBUG_POOL_MIN_LEVEL;
> 
> As debug_objects_pool_min_level is the minimum level to keep around and
> obj_pool_min_free is a statistics mechanism, __data_racy is misplaced
> too. The variables should swap their position, because
> debug_objects_pool_min_level is functional, but obj_pool_min_free is
> pure stats.
> 
> Also debug_objects_pool_min_level and debug_objects_pool_size should
> be __ro_after_init.

OK, How about modifying it like this? I further removed the __data_racy from
debug_objects_pool_min_level and debug_objects_pool_size, because __ro_after_init.


diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index d3845705db955fa..816d3d968cd9f14 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -70,7 +70,8 @@ static HLIST_HEAD(obj_to_free);
  * made at debug_stats_show(). Both obj_pool_min_free and obj_pool_max_used
  * can be off.
  */
-static int                     obj_pool_min_free = ODEBUG_POOL_SIZE;
+static int __ro_after_init     debug_objects_pool_size = ODEBUG_POOL_SIZE;
+static int __ro_after_init     debug_objects_pool_min_level = ODEBUG_POOL_MIN_LEVEL;
 static int                     obj_pool_free = ODEBUG_POOL_SIZE;
 static int                     obj_pool_used;
 static int                     obj_pool_max_used;
@@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
 static int __data_racy                 debug_objects_warnings __read_mostly;
 static int __data_racy                 debug_objects_enabled __read_mostly
                                        = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
-static int __data_racy                 debug_objects_pool_size __read_mostly
-                                       = ODEBUG_POOL_SIZE;
-static int __data_racy                 debug_objects_pool_min_level __read_mostly
-                                       = ODEBUG_POOL_MIN_LEVEL;
+static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;

 static const struct debug_obj_descr *descr_test  __read_mostly;
 static struct kmem_cache       *obj_cache __ro_after_init;


> 
>> Fixes: d26bf5056fc0 ("debugobjects: Reduce number of pool_lock acquisitions in fill_pool()")
>> Fixes: 36c4ead6f6df ("debugobjects: Add global free list and the counter")
> 
> Nice detective work!
> 
> Thanks,
> 
>         tglx
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Leizhen (ThunderTown)]

On 2024/9/3 10:16, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/9/3 0:22, Thomas Gleixner wrote:
>> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
>>
>>> The global variable 'obj_pool_min_free' records the lowest historical
>>> value of the number of nodes in the global list 'obj_pool', instead of
>>> being used as the lowest threshold value. This may be a mistake and
>>
>> Maybe? It's either a bug or not.
> 
> Yes, it's a bug, but I'm just learning this module, so I'm not confident.
> 
>>
>>> should be replaced with variable 'debug_objects_pool_min_level'.
>>
>> And if it's a bug then it has to be replaced.
> 
> OK, I will update the commit message in V2.
> 
>>
>> This misses another minor issue:
>>
>> static int			obj_pool_min_free = ODEBUG_POOL_SIZE;
>>
>> static int  __data_racy		debug_objects_pool_min_level __read_mostly
>> 				= ODEBUG_POOL_MIN_LEVEL;
>>
>> As debug_objects_pool_min_level is the minimum level to keep around and
>> obj_pool_min_free is a statistics mechanism, __data_racy is misplaced
>> too. The variables should swap their position, because
>> debug_objects_pool_min_level is functional, but obj_pool_min_free is
>> pure stats.

This principle covers a large number of variables. It should be sorted
by the variable name before. It's better not to change the position this time.

>>
>> Also debug_objects_pool_min_level and debug_objects_pool_size should
>> be __ro_after_init.
> 
> OK, How about modifying it like this? I further removed the __data_racy from
> debug_objects_pool_min_level and debug_objects_pool_size, because __ro_after_init.
> 
> 
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index d3845705db955fa..816d3d968cd9f14 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -70,7 +70,8 @@ static HLIST_HEAD(obj_to_free);
>   * made at debug_stats_show(). Both obj_pool_min_free and obj_pool_max_used
>   * can be off.
>   */
> -static int                     obj_pool_min_free = ODEBUG_POOL_SIZE;
> +static int __ro_after_init     debug_objects_pool_size = ODEBUG_POOL_SIZE;
> +static int __ro_after_init     debug_objects_pool_min_level = ODEBUG_POOL_MIN_LEVEL;
>  static int                     obj_pool_free = ODEBUG_POOL_SIZE;
>  static int                     obj_pool_used;
>  static int                     obj_pool_max_used;
> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>  static int __data_racy                 debug_objects_warnings __read_mostly;
>  static int __data_racy                 debug_objects_enabled __read_mostly
>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
> -static int __data_racy                 debug_objects_pool_size __read_mostly
> -                                       = ODEBUG_POOL_SIZE;
> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
> -                                       = ODEBUG_POOL_MIN_LEVEL;
> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;
> 
>  static const struct debug_obj_descr *descr_test  __read_mostly;
>  static struct kmem_cache       *obj_cache __ro_after_init;
> 
> 
>>
>>> Fixes: d26bf5056fc0 ("debugobjects: Reduce number of pool_lock acquisitions in fill_pool()")
>>> Fixes: 36c4ead6f6df ("debugobjects: Add global free list and the counter")
>>
>> Nice detective work!
>>
>> Thanks,
>>
>>         tglx
>> .
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Leizhen (ThunderTown)]

On 2024/9/3 11:22, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/9/3 10:16, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2024/9/3 0:22, Thomas Gleixner wrote:
>>> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
>>>
>>>> The global variable 'obj_pool_min_free' records the lowest historical
>>>> value of the number of nodes in the global list 'obj_pool', instead of
>>>> being used as the lowest threshold value. This may be a mistake and
>>>
>>> Maybe? It's either a bug or not.
>>
>> Yes, it's a bug, but I'm just learning this module, so I'm not confident.
>>
>>>
>>>> should be replaced with variable 'debug_objects_pool_min_level'.
>>>
>>> And if it's a bug then it has to be replaced.
>>
>> OK, I will update the commit message in V2.
>>
>>>
>>> This misses another minor issue:
>>>
>>> static int			obj_pool_min_free = ODEBUG_POOL_SIZE;
>>>
>>> static int  __data_racy		debug_objects_pool_min_level __read_mostly
>>> 				= ODEBUG_POOL_MIN_LEVEL;
>>>
>>> As debug_objects_pool_min_level is the minimum level to keep around and
>>> obj_pool_min_free is a statistics mechanism, __data_racy is misplaced
>>> too. The variables should swap their position, because
>>> debug_objects_pool_min_level is functional, but obj_pool_min_free is
>>> pure stats.
> 
> This principle covers a large number of variables. It should be sorted
> by the variable name before. It's better not to change the position this time.
> 
>>>
>>> Also debug_objects_pool_min_level and debug_objects_pool_size should
>>> be __ro_after_init.
>>
>> OK, How about modifying it like this? I further removed the __data_racy from
>> debug_objects_pool_min_level and debug_objects_pool_size, because __ro_after_init.
>>
>>
>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index d3845705db955fa..816d3d968cd9f14 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -70,7 +70,8 @@ static HLIST_HEAD(obj_to_free);
>>   * made at debug_stats_show(). Both obj_pool_min_free and obj_pool_max_used
>>   * can be off.
>>   */
>> -static int                     obj_pool_min_free = ODEBUG_POOL_SIZE;
>> +static int __ro_after_init     debug_objects_pool_size = ODEBUG_POOL_SIZE;
>> +static int __ro_after_init     debug_objects_pool_min_level = ODEBUG_POOL_MIN_LEVEL;
>>  static int                     obj_pool_free = ODEBUG_POOL_SIZE;
>>  static int                     obj_pool_used;
>>  static int                     obj_pool_max_used;
>> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>>  static int __data_racy                 debug_objects_warnings __read_mostly;
>>  static int __data_racy                 debug_objects_enabled __read_mostly
>>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
>> -static int __data_racy                 debug_objects_pool_size __read_mostly
>> -                                       = ODEBUG_POOL_SIZE;
>> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
>> -                                       = ODEBUG_POOL_MIN_LEVEL;
>> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;

Sorry, I rechecked it again. After this patch, obj_pool_min_free is referenced in the
same way as obj_pool_max_used. The only race point is located in debug_stats_show().
However, this reference point does not need to be included in the race analysis. So
there is no need to add __data_racy for obj_pool_min_free.

The final modification is as follows:

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index d3845705db955fa..721e207f85b6be7 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -84,9 +84,9 @@ static int __data_racy                        debug_objects_fixups __read_mostly;
 static int __data_racy                 debug_objects_warnings __read_mostly;
 static int __data_racy                 debug_objects_enabled __read_mostly
                                        = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
-static int __data_racy                 debug_objects_pool_size __read_mostly
+static int                             debug_objects_pool_size __ro_after_init
                                        = ODEBUG_POOL_SIZE;
-static int __data_racy                 debug_objects_pool_min_level __read_mostly
+static int                             debug_objects_pool_min_level __ro_after_init
                                        = ODEBUG_POOL_MIN_LEVEL;

 static const struct debug_obj_descr *descr_test  __read_mostly;


>>
>>  static const struct debug_obj_descr *descr_test  __read_mostly;
>>  static struct kmem_cache       *obj_cache __ro_after_init;
>>
>>
>>>
>>>> Fixes: d26bf5056fc0 ("debugobjects: Reduce number of pool_lock acquisitions in fill_pool()")
>>>> Fixes: 36c4ead6f6df ("debugobjects: Add global free list and the counter")
>>>
>>> Nice detective work!
>>>
>>> Thanks,
>>>
>>>         tglx
>>> .
>>>
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Thomas Gleixner]

On Tue, Sep 03 2024 at 15:00, Leizhen wrote:
>>> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>>>  static int __data_racy                 debug_objects_warnings __read_mostly;
>>>  static int __data_racy                 debug_objects_enabled __read_mostly
>>>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
>>> -static int __data_racy                 debug_objects_pool_size __read_mostly
>>> -                                       = ODEBUG_POOL_SIZE;
>>> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
>>> -                                       = ODEBUG_POOL_MIN_LEVEL;
>>> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;
>
> Sorry, I rechecked it again. After this patch, obj_pool_min_free is referenced in the
> same way as obj_pool_max_used. The only race point is located in debug_stats_show().
> However, this reference point does not need to be included in the race analysis. So
> there is no need to add __data_racy for obj_pool_min_free.

The read races against the write, so KCSAN can detect it and complain, no?

Thanks,

        tglx

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Leizhen (ThunderTown)]

On 2024/9/3 17:37, Thomas Gleixner wrote:
> On Tue, Sep 03 2024 at 15:00, Leizhen wrote:
>>>> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>>>>  static int __data_racy                 debug_objects_warnings __read_mostly;
>>>>  static int __data_racy                 debug_objects_enabled __read_mostly
>>>>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
>>>> -static int __data_racy                 debug_objects_pool_size __read_mostly
>>>> -                                       = ODEBUG_POOL_SIZE;
>>>> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
>>>> -                                       = ODEBUG_POOL_MIN_LEVEL;
>>>> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;
>>
>> Sorry, I rechecked it again. After this patch, obj_pool_min_free is referenced in the
>> same way as obj_pool_max_used. The only race point is located in debug_stats_show().
>> However, this reference point does not need to be included in the race analysis. So
>> there is no need to add __data_racy for obj_pool_min_free.
> 
> The read races against the write, so KCSAN can detect it and complain, no?

Oh, I just saw that there were a lot of other global variables in that function
that didn't mask KCSAN's detection. So I'll recheck each global variable.
However, for obj_pool_min_free, it seems that it would be better to just add
READ_ONCE() in debug_stats_show(). This does not prevent the compiler from
optimizing variable references in the lock.

# define __data_racy volatile

> 
> Thanks,
> 
>         tglx
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Thomas Gleixner]

On Tue, Sep 03 2024 at 19:14, Leizhen wrote:
> On 2024/9/3 17:37, Thomas Gleixner wrote:
>> On Tue, Sep 03 2024 at 15:00, Leizhen wrote:
>>>>> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>>>>>  static int __data_racy                 debug_objects_warnings __read_mostly;
>>>>>  static int __data_racy                 debug_objects_enabled __read_mostly
>>>>>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
>>>>> -static int __data_racy                 debug_objects_pool_size __read_mostly
>>>>> -                                       = ODEBUG_POOL_SIZE;
>>>>> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
>>>>> -                                       = ODEBUG_POOL_MIN_LEVEL;
>>>>> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;
>>>
>>> Sorry, I rechecked it again. After this patch, obj_pool_min_free is referenced in the
>>> same way as obj_pool_max_used. The only race point is located in debug_stats_show().
>>> However, this reference point does not need to be included in the race analysis. So
>>> there is no need to add __data_racy for obj_pool_min_free.
>> 
>> The read races against the write, so KCSAN can detect it and complain, no?
>
> Oh, I just saw that there were a lot of other global variables in that function
> that didn't mask KCSAN's detection. So I'll recheck each global variable.
> However, for obj_pool_min_free, it seems that it would be better to just add
> READ_ONCE() in debug_stats_show(). This does not prevent the compiler from
> optimizing variable references in the lock.
>
> # define __data_racy volatile

This is only when KCSAN is enabled. Otherwise it's empty.

And if you do a READ_ONCE() then you need a corresponding WRITE_ONCE()
to make sense. __data_racy is much simpler for that.

Thanks,

        tglx

Re: [PATCH 1/5] debugobjects: Fix the misuse of global variables in fill_pool()

[Leizhen (ThunderTown)]

On 2024/9/3 19:43, Thomas Gleixner wrote:
> On Tue, Sep 03 2024 at 19:14, Leizhen wrote:
>> On 2024/9/3 17:37, Thomas Gleixner wrote:
>>> On Tue, Sep 03 2024 at 15:00, Leizhen wrote:
>>>>>> @@ -84,10 +85,7 @@ static int __data_racy                       debug_objects_fixups __read_mostly;
>>>>>>  static int __data_racy                 debug_objects_warnings __read_mostly;
>>>>>>  static int __data_racy                 debug_objects_enabled __read_mostly
>>>>>>                                         = CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
>>>>>> -static int __data_racy                 debug_objects_pool_size __read_mostly
>>>>>> -                                       = ODEBUG_POOL_SIZE;
>>>>>> -static int __data_racy                 debug_objects_pool_min_level __read_mostly
>>>>>> -                                       = ODEBUG_POOL_MIN_LEVEL;
>>>>>> +static int __data_racy                 obj_pool_min_free = ODEBUG_POOL_SIZE;
>>>>
>>>> Sorry, I rechecked it again. After this patch, obj_pool_min_free is referenced in the
>>>> same way as obj_pool_max_used. The only race point is located in debug_stats_show().
>>>> However, this reference point does not need to be included in the race analysis. So
>>>> there is no need to add __data_racy for obj_pool_min_free.
>>>
>>> The read races against the write, so KCSAN can detect it and complain, no?
>>
>> Oh, I just saw that there were a lot of other global variables in that function
>> that didn't mask KCSAN's detection. So I'll recheck each global variable.
>> However, for obj_pool_min_free, it seems that it would be better to just add
>> READ_ONCE() in debug_stats_show(). This does not prevent the compiler from
>> optimizing variable references in the lock.
>>
>> # define __data_racy volatile
> 
> This is only when KCSAN is enabled. Otherwise it's empty.
> 
> And if you do a READ_ONCE() then you need a corresponding WRITE_ONCE()
> to make sense. __data_racy is much simpler for that.

OK, I will use __data_racy, thanks

> 
> Thanks,
> 
>         tglx
> .
> 

-- 
Regards,
  Zhen Lei

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

[Zhen Lei]

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()

[Thomas Gleixner]

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()

[Leizhen (ThunderTown)]

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

[PATCH 3/5] debugobjects: Don't start fill if there are remaining nodes locally

[Zhen Lei]

If the conditions for starting fill are met, it means that all cores that
call fill() later are blocked until the first core completes the fill
operation. But obviously, for a core that has free nodes locally, it does
not need to be blocked. This is good in stress situations.

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

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index aba3e62a4315f51..fc8224f9f0eda8f 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -130,10 +130,15 @@ static void fill_pool(void)
 	gfp_t gfp = __GFP_HIGH | __GFP_NOWARN;
 	struct debug_obj *obj;
 	unsigned long flags;
+	struct debug_percpu_free *percpu_pool;
 
 	if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
 		return;
 
+	percpu_pool = this_cpu_ptr(&percpu_obj_pool);
+	if (likely(obj_cache) && percpu_pool->obj_free > 0)
+		return;
+
 	/*
 	 * Reuse objs from the global free list; they will be reinitialized
 	 * when allocating.
-- 
2.34.1

Re: [PATCH 3/5] debugobjects: Don't start fill if there are remaining nodes locally

[Thomas Gleixner]

On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:

> If the conditions for starting fill are met, it means that all cores that
> call fill() later are blocked until the first core completes the fill
> operation. But obviously, for a core that has free nodes locally, it does
> not need to be blocked. This is good in stress situations.

Sure it's good, but is it correct? You need to explain why this can't
cause a pool depletion. The pool is filled opportunistically.

Aside of that the lock contention in fill_pool() is minimal. The heavy
lifting is the allocation of objects.

> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index aba3e62a4315f51..fc8224f9f0eda8f 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -130,10 +130,15 @@ static void fill_pool(void)
>  	gfp_t gfp = __GFP_HIGH | __GFP_NOWARN;
>  	struct debug_obj *obj;
>  	unsigned long flags;
> +	struct debug_percpu_free *percpu_pool;

Please keep variables in reverse fir tree order.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
  
>  	if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
>  		return;
>  
> +	percpu_pool = this_cpu_ptr(&percpu_obj_pool);

You don't need the pointer

> +	if (likely(obj_cache) && percpu_pool->obj_free > 0)

	if (likely(obj_cache) && this_cpu_read(percpu_pool.obj_free) > 0)

This lacks a comment explaining the rationale of this check.

Thanks,

        tglx

Re: [PATCH 3/5] debugobjects: Don't start fill if there are remaining nodes locally

[Leizhen (ThunderTown)]

On 2024/9/3 17:52, Thomas Gleixner wrote:
> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
> 
>> If the conditions for starting fill are met, it means that all cores that
>> call fill() later are blocked until the first core completes the fill
>> operation. But obviously, for a core that has free nodes locally, it does
>> not need to be blocked. This is good in stress situations.
> 
> Sure it's good, but is it correct? You need to explain why this can't> cause a pool depletion. The pool is filled opportunistically.
In the case of no nesting, a core uses only one node at a time.
Even if nesting occurs and there is only one local node,
256 / (16 + 1) = 15, the current parameter definition tolerates
15 cores, which should be sufficient. In fact, many cores may
see just >= 256 at the same time without filling. Therefore,
to eliminate the probability problem completely, an additional
mechanism is needed.

#define ODEBUG_POOL_MIN_LEVEL	256
#define ODEBUG_BATCH_SIZE	16

> 
> Aside of that the lock contention in fill_pool() is minimal. The heavy
> lifting is the allocation of objects.

I'm optimizing this, too. However, a new hlist helper function need to
be added. Now that you've mentioned it, I'll send it in V2 too!

> 
>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index aba3e62a4315f51..fc8224f9f0eda8f 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -130,10 +130,15 @@ static void fill_pool(void)
>>  	gfp_t gfp = __GFP_HIGH | __GFP_NOWARN;
>>  	struct debug_obj *obj;
>>  	unsigned long flags;
>> +	struct debug_percpu_free *percpu_pool;
> 
> Please keep variables in reverse fir tree order.
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

OK, I will correct it.

>   
>>  	if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
>>  		return;
>>  
>> +	percpu_pool = this_cpu_ptr(&percpu_obj_pool);
> 
> You don't need the pointer
> 
>> +	if (likely(obj_cache) && percpu_pool->obj_free > 0)
> 
> 	if (likely(obj_cache) && this_cpu_read(percpu_pool.obj_free) > 0)

Nice, thanks

> 
> This lacks a comment explaining the rationale of this check.

OK, I'll add.

> 
> Thanks,
> 
>         tglx
> 
> 
> 
> 
> .
> 

-- 
Regards,
  Zhen Lei

[PATCH 4/5] debugobjects: Use hlist_splice_init() to reduce lock conflicts

[Zhen Lei]

The sub list can be prepared in advance outside the lock, so that the
operation time inside the lock can be reduced and the possibility of
lock conflict can be reduced.

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

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index fc8224f9f0eda8f..998724e9dee526b 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -167,23 +167,25 @@ static void fill_pool(void)
 		return;
 
 	while (READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
-		struct debug_obj *new[ODEBUG_BATCH_SIZE];
+		HLIST_HEAD(batch_list);
+		struct debug_obj *new, *last;
 		int cnt;
 
 		for (cnt = 0; cnt < ODEBUG_BATCH_SIZE; cnt++) {
-			new[cnt] = kmem_cache_zalloc(obj_cache, gfp);
-			if (!new[cnt])
+			new = kmem_cache_zalloc(obj_cache, gfp);
+			if (!new)
 				break;
+			hlist_add_head(&new->node, &batch_list);
+			if (cnt == 0)
+				last = new;
 		}
 		if (!cnt)
 			return;
 
 		raw_spin_lock_irqsave(&pool_lock, flags);
-		while (cnt) {
-			hlist_add_head(&new[--cnt]->node, &obj_pool);
-			debug_objects_allocated++;
-			WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
-		}
+		hlist_splice_init(&batch_list, &last->node, &obj_pool);
+		debug_objects_allocated += cnt;
+		WRITE_ONCE(obj_pool_free, obj_pool_free + cnt);
 		raw_spin_unlock_irqrestore(&pool_lock, flags);
 	}
 }
-- 
2.34.1

Re: [PATCH 4/5] debugobjects: Use hlist_splice_init() to reduce lock conflicts

[Thomas Gleixner]

On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:

> The sub list can be prepared in advance outside the lock, so that the
> operation time inside the lock can be reduced and the possibility of
> lock conflict can be reduced.
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  lib/debugobjects.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index fc8224f9f0eda8f..998724e9dee526b 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -167,23 +167,25 @@ static void fill_pool(void)
>  		return;
>  
>  	while (READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
> -		struct debug_obj *new[ODEBUG_BATCH_SIZE];
> +		HLIST_HEAD(batch_list);
> +		struct debug_obj *new, *last;

Variable ordering please.

>  		int cnt;
>  
>  		for (cnt = 0; cnt < ODEBUG_BATCH_SIZE; cnt++) {
> -			new[cnt] = kmem_cache_zalloc(obj_cache, gfp);
> -			if (!new[cnt])
> +			new = kmem_cache_zalloc(obj_cache, gfp);
> +			if (!new)
>  				break;
> +			hlist_add_head(&new->node, &batch_list);
> +			if (cnt == 0)
                        
                        if (!cnt)

but it would be more self explaining if you have:

		struct debug_obj *new, *last = NULL;

and then
                if (!last)

> +				last = new;
>  		}

Thanks,

        tglx

Re: [PATCH 4/5] debugobjects: Use hlist_splice_init() to reduce lock conflicts

[Leizhen (ThunderTown)]

On 2024/9/3 18:09, Thomas Gleixner wrote:
> On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
> 
>> The sub list can be prepared in advance outside the lock, so that the
>> operation time inside the lock can be reduced and the possibility of
>> lock conflict can be reduced.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  lib/debugobjects.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index fc8224f9f0eda8f..998724e9dee526b 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -167,23 +167,25 @@ static void fill_pool(void)
>>  		return;
>>  
>>  	while (READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
>> -		struct debug_obj *new[ODEBUG_BATCH_SIZE];
>> +		HLIST_HEAD(batch_list);
>> +		struct debug_obj *new, *last;
> 
> Variable ordering please.

OK

> 
>>  		int cnt;
>>  
>>  		for (cnt = 0; cnt < ODEBUG_BATCH_SIZE; cnt++) {
>> -			new[cnt] = kmem_cache_zalloc(obj_cache, gfp);
>> -			if (!new[cnt])
>> +			new = kmem_cache_zalloc(obj_cache, gfp);
>> +			if (!new)
>>  				break;
>> +			hlist_add_head(&new->node, &batch_list);
>> +			if (cnt == 0)
>                         
>                         if (!cnt)
> 
> but it would be more self explaining if you have:
> 
> 		struct debug_obj *new, *last = NULL;
> 
> and then
>                 if (!last)

OK, I will change it. I thought so, but I changed it to reduce
the null initialization. Ha ha, wrong choice.

> 
>> +				last = new;
>>  		}
> 
> Thanks,
> 
>         tglx
> .
> 

-- 
Regards,
  Zhen Lei

[PATCH 5/5] debugobjects: Delete a piece of redundant code

[Zhen Lei]

The statically allocated objects are all located in obj_static_pool[],
no one will use them anymore, the whole memory of obj_static_pool[] will
be reclaimed later. Therefore, there is no need to split the remaining
statically nodes in list obj_pool into isolated ones. Just write
INIT_HLIST_HEAD(&obj_pool) is enough. Since hlist_move_list() directly
discards the old list, even this can be omitted.

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

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 998724e9dee526b..d3845705db955fa 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -1333,10 +1333,7 @@ static int __init debug_objects_replace_static_objects(void)
 	 * active object references.
 	 */
 
-	/* Remove the statically allocated objects from the pool */
-	hlist_for_each_entry_safe(obj, tmp, &obj_pool, node)
-		hlist_del(&obj->node);
-	/* Move the allocated objects to the pool */
+	/* Replace the statically allocated objects list with the allocated objects list */
 	hlist_move_list(&objects, &obj_pool);
 
 	/* Replace the active object references */
-- 
2.34.1

Re: [PATCH 5/5] debugobjects: Delete a piece of redundant code

[Thomas Gleixner]

On Mon, Sep 02 2024 at 22:05, Zhen Lei wrote:
> The statically allocated objects are all located in obj_static_pool[],
> no one will use them anymore, the whole memory of obj_static_pool[] will

No one used them ever: nothing will use them

> be reclaimed later. Therefore, there is no need to split the remaining
> statically nodes in list obj_pool into isolated ones. Just write
> INIT_HLIST_HEAD(&obj_pool) is enough. Since hlist_move_list() directly
> discards the old list, even this can be omitted.

Nice!

Thanks,

        tglx