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.1On 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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.