[PATCH 1/2 v5] lib: debugobjects: export max loops counter

[PATCH 1/2 v5] lib: debugobjects: export max loops counter

[Yang Shi]

Currently max chain counter is exported to debugfs, it just record the
counter of inner loop, however, there might be significant iterations of
external loop then it may take significant amount of time to finish all
of the checks. This may cause lockup on !CONFIG_PREEMPT kernel build
occasionally.

Record the counter of the max loops then export to sysfs so that the
user can be aware of the real overhead.

Then the output of /sys/kernel/debug/debug_objects/stats looks like:

max_chain     :121
max_loops     :543267
warnings      :0
fixups        :0
pool_free     :1764
pool_min_free :341
pool_used     :86438
pool_max_used :268887
objs_allocated:6068254
objs_freed    :5981076

Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
v1 --> v2:
 * Correct the typo in the commit log

 lib/debugobjects.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 2f5349c..166488d 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -50,6 +50,7 @@ struct debug_bucket {
 static struct kmem_cache	*obj_cache;
 
 static int			debug_objects_maxchain __read_mostly;
+static int			debug_objects_maxloops __read_mostly;
 static int			debug_objects_fixups __read_mostly;
 static int			debug_objects_warnings __read_mostly;
 static int			debug_objects_enabled __read_mostly
@@ -720,7 +721,7 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 	enum debug_obj_state state;
 	struct debug_bucket *db;
 	struct debug_obj *obj;
-	int cnt;
+	int cnt, max_loops = 0;
 
 	saddr = (unsigned long) address;
 	eaddr = saddr + size;
@@ -765,7 +766,12 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 
 		if (cnt > debug_objects_maxchain)
 			debug_objects_maxchain = cnt;
+
+		max_loops += cnt;
 	}
+
+	if (max_loops > debug_objects_maxloops)
+		debug_objects_maxloops = max_loops;
 }
 
 void debug_check_no_obj_freed(const void *address, unsigned long size)
@@ -780,6 +786,7 @@ void debug_check_no_obj_freed(const void *address, unsigned long size)
 static int debug_stats_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "max_chain     :%d\n", debug_objects_maxchain);
+	seq_printf(m, "max_loops     :%d\n", debug_objects_maxloops);
 	seq_printf(m, "warnings      :%d\n", debug_objects_warnings);
 	seq_printf(m, "fixups        :%d\n", debug_objects_fixups);
 	seq_printf(m, "pool_free     :%d\n", obj_pool_free);
-- 
1.8.3.1

[PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

[Yang Shi]

There are nested loops on debug objects free path, sometimes it may take
over hundred thousands of loops, then cause soft lockup with
!CONFIG_PREEMPT occasionally, like below:

NMI watchdog: BUG: soft lockup - CPU#15 stuck for 22s! [stress-ng-getde:110342]

 CPU: 15 PID: 110342 Comm: stress-ng-getde Tainted: G
E   4.9.44-003.ali3000.alios7.x86_64.debug #1
 Hardware name: Dell Inc. PowerEdge R720xd/0X6FFV, BIOS
1.6.0 03/07/2013

Call Trace:
  [<ffffffff8141177e>] debug_check_no_obj_freed+0x13e/0x220
  [<ffffffff811f8751>] __free_pages_ok+0x1f1/0x5c0
  [<ffffffff811fa785>] __free_pages+0x25/0x40
  [<ffffffff812638db>] __free_slab+0x19b/0x270
  [<ffffffff812639e9>] discard_slab+0x39/0x50
  [<ffffffff812679f7>] __slab_free+0x207/0x270
  [<ffffffff81269966>] ___cache_free+0xa6/0xb0
  [<ffffffff8126c267>] qlist_free_all+0x47/0x80
  [<ffffffff8126c5a9>] quarantine_reduce+0x159/0x190
  [<ffffffff8126b3bf>] kasan_kmalloc+0xaf/0xc0
  [<ffffffff8126b8a2>] kasan_slab_alloc+0x12/0x20
  [<ffffffff81265e8a>] kmem_cache_alloc+0xfa/0x360
  [<ffffffff812abc8f>] ? getname_flags+0x4f/0x1f0
  [<ffffffff812abc8f>] getname_flags+0x4f/0x1f0
  [<ffffffff812abe42>] getname+0x12/0x20
  [<ffffffff81298da9>] do_sys_open+0xf9/0x210
  [<ffffffff81298ede>] SyS_open+0x1e/0x20
  [<ffffffff817d6e01>] entry_SYSCALL_64_fastpath+0x1f/0xc2

The code path might be called in either atomic or non-atomic context,
and in_atomic() can't tell if current context is atomic or not on
!PREEMPT kernel, so cond_resched() can't be used to prevent from the
softlockup.

Defer objects free outside of the loop in a batch to save some cycles
in the loop.
The objects will be added to a global free list, then put them back to
pool list in a work if the pool list is not full. If the pool list is
already full, the objects will stay on the global free list, then will
be freed later.
When allocating objects, check if there are any objects available on
the global free list and just reuse the objects if the global free list
is not empty. Reuse pool lock to protect the free list. Using a new lock
sounds overkilling.

Export the number of objects on the global freelist to sysfs, which
looks like:

max_chain     :79
max_loops     :8147
warnings      :0
fixups        :0
pool_free     :1697
pool_min_free :346
pool_used     :15356
pool_max_used :23933
on_free_list  :39
objs_allocated:32617
objs_freed    :16588

Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Waiman Long <longman@redhat.com>
---
v5: Trimmed commit log to just keep call stack and process info.
    Per tglx's comment, just move free objs to the pool list when it is not
    full. If the pool list is full, free the memory of objs on the free list.
v4: Dropped touching softlockup watchdog approach, and defer objects free
    outside the for loop per the suggestion from tglx.

 lib/debugobjects.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 166488d..943882b 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -42,11 +42,14 @@ struct debug_bucket {
 static DEFINE_RAW_SPINLOCK(pool_lock);
 
 static HLIST_HEAD(obj_pool);
+static HLIST_HEAD(obj_to_free);
 
 static int			obj_pool_min_free = ODEBUG_POOL_SIZE;
 static int			obj_pool_free = ODEBUG_POOL_SIZE;
 static int			obj_pool_used;
 static int			obj_pool_max_used;
+/* The number of objs on the global free list */
+static int			obj_nr_tofree;
 static struct kmem_cache	*obj_cache;
 
 static int			debug_objects_maxchain __read_mostly;
@@ -141,7 +144,8 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 }
 
 /*
- * Allocate a new object. If the pool is empty, switch off the debugger.
+ * Allocate a new object. Retrieve from global freelist first. If the pool is
+ * empty, switch off the debugger.
  * Must be called with interrupts disabled.
  */
 static struct debug_obj *
@@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 	struct debug_obj *obj = NULL;
 
 	raw_spin_lock(&pool_lock);
+	if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
+		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
+		obj_nr_tofree--;
+		hlist_del(&obj->node);
+		goto out;
+	}
+
 	if (obj_pool.first) {
 		obj	    = hlist_entry(obj_pool.first, typeof(*obj), node);
 
@@ -169,6 +180,8 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 		if (obj_pool_free < obj_pool_min_free)
 			obj_pool_min_free = obj_pool_free;
 	}
+
+out:
 	raw_spin_unlock(&pool_lock);
 
 	return obj;
@@ -186,11 +199,37 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 static void free_obj_work(struct work_struct *work)
 {
 	struct debug_obj *objs[ODEBUG_FREE_BATCH];
+	struct hlist_node *tmp;
+	struct debug_obj *obj;
 	unsigned long flags;
 	int i;
+	HLIST_HEAD(tofree);
 
 	if (!raw_spin_trylock_irqsave(&pool_lock, flags))
 		return;
+
+	/* When pool list is not full move free objs to pool list */
+	while (obj_pool_free < debug_objects_pool_size) {
+		if (obj_nr_tofree <= 0)
+			break;
+
+		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
+		hlist_del(&obj->node);
+		hlist_add_head(&obj->node, &obj_pool);
+		obj_pool_free++;
+		obj_pool_used--;
+		obj_nr_tofree--;
+	}
+
+	/*
+	 * pool list is already full, and there are still objs on the free list,
+	 * move remaining free objs to a separate list to free the memory later.
+	 */
+	if (obj_nr_tofree > 0) {
+		hlist_move_list(&obj_to_free, &tofree);
+		obj_nr_tofree = 0;
+	}
+
 	while (obj_pool_free >= debug_objects_pool_size + ODEBUG_FREE_BATCH) {
 		for (i = 0; i < ODEBUG_FREE_BATCH; i++) {
 			objs[i] = hlist_entry(obj_pool.first,
@@ -211,6 +250,13 @@ static void free_obj_work(struct work_struct *work)
 			return;
 	}
 	raw_spin_unlock_irqrestore(&pool_lock, flags);
+
+	if (!hlist_empty(&tofree)) {
+		hlist_for_each_entry_safe(obj, tmp, &tofree, node) {
+			hlist_del(&obj->node);
+			kmem_cache_free(obj_cache, obj);
+		}
+	}
 }
 
 /*
@@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 {
 	unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
 	struct hlist_node *tmp;
-	HLIST_HEAD(freelist);
 	struct debug_obj_descr *descr;
 	enum debug_obj_state state;
 	struct debug_bucket *db;
@@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 				goto repeat;
 			default:
 				hlist_del(&obj->node);
-				hlist_add_head(&obj->node, &freelist);
+				/* Put obj on the global free list */
+				raw_spin_lock(&pool_lock);
+				hlist_add_head(&obj->node, &obj_to_free);
+				/* Update the counter of objs on the global freelist */
+				obj_nr_tofree++;
+				raw_spin_unlock(&pool_lock);
 				break;
 			}
 		}
 		raw_spin_unlock_irqrestore(&db->lock, flags);
 
-		/* Now free them */
-		hlist_for_each_entry_safe(obj, tmp, &freelist, node) {
-			hlist_del(&obj->node);
-			free_object(obj);
-		}
-
 		if (cnt > debug_objects_maxchain)
 			debug_objects_maxchain = cnt;
 
@@ -772,6 +816,10 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 
 	if (max_loops > debug_objects_maxloops)
 		debug_objects_maxloops = max_loops;
+
+	/* Schedule work to move free objs to pool list */
+	if (obj_nr_tofree > 0)
+		schedule_work(&debug_obj_work);
 }
 
 void debug_check_no_obj_freed(const void *address, unsigned long size)
@@ -793,6 +841,7 @@ static int debug_stats_show(struct seq_file *m, void *v)
 	seq_printf(m, "pool_min_free :%d\n", obj_pool_min_free);
 	seq_printf(m, "pool_used     :%d\n", obj_pool_used);
 	seq_printf(m, "pool_max_used :%d\n", obj_pool_max_used);
+	seq_printf(m, "on_free_list  :%d\n", obj_nr_tofree);
 	seq_printf(m, "objs_allocated:%d\n", debug_objects_allocated);
 	seq_printf(m, "objs_freed    :%d\n", debug_objects_freed);
 	return 0;
-- 
1.8.3.1

Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

[Thomas Gleixner]

On Fri, 2 Feb 2018, Yang Shi wrote:

>  /*
> - * Allocate a new object. If the pool is empty, switch off the debugger.
> + * Allocate a new object. Retrieve from global freelist first. If the pool is
> + * empty, switch off the debugger.
>   * Must be called with interrupts disabled.
>   */
>  static struct debug_obj *
> @@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
>  	struct debug_obj *obj = NULL;
>  
>  	raw_spin_lock(&pool_lock);

Why in alloc_object() and not in fill_pool()?

> +	if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
> +		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
> +		obj_nr_tofree--;
> +		hlist_del(&obj->node);
> +		goto out;
> +	}

Errm. This is wrong. It does not reinitialize the object. Please do that
shuffling in fill_pool().

>  	if (obj_pool.first) {
>  		obj	    = hlist_entry(obj_pool.first, typeof(*obj), node);

....

> +	/* When pool list is not full move free objs to pool list */
> +	while (obj_pool_free < debug_objects_pool_size) {
> +		if (obj_nr_tofree <= 0)
> +			break;
> +
> +		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
> +		hlist_del(&obj->node);
> +		hlist_add_head(&obj->node, &obj_pool);
> +		obj_pool_free++;
> +		obj_pool_used--;
> +		obj_nr_tofree--;
> +	}
> +
> +	/*
> +	 * pool list is already full, and there are still objs on the free list,
> +	 * move remaining free objs to a separate list to free the memory later.
> +	 */
> +	if (obj_nr_tofree > 0) {
> +		hlist_move_list(&obj_to_free, &tofree);
> +		obj_nr_tofree = 0;
> +	}

The accounting is inconsistent. You leak obj_pool_used. But that's wrong
anyway because an object should not be accounted for in two places. It's
only on _ONE_ list....

> @@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>  {
>  	unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
>  	struct hlist_node *tmp;
> -	HLIST_HEAD(freelist);
>  	struct debug_obj_descr *descr;
>  	enum debug_obj_state state;
>  	struct debug_bucket *db;
> @@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>  				goto repeat;
>  			default:
>  				hlist_del(&obj->node);
> -				hlist_add_head(&obj->node, &freelist);
> +				/* Put obj on the global free list */
> +				raw_spin_lock(&pool_lock);
> +				hlist_add_head(&obj->node, &obj_to_free);
> +				/* Update the counter of objs on the global freelist */
> +				obj_nr_tofree++;
> +				raw_spin_unlock(&pool_lock);

As we have to take pool_lock anyway, we simply can change free_object() to:

static bool __free_object(obj)
{
	bool work;

	lock(pool);
	work = obj_pool_free > debug_objects_pool_size && obj_cache;
	obj_pool_used++;
	if (work) {
		obj_nr_tofree++;
		hlist_add_head(&obj->node, &obj_to_free);
	] else {
		obj_pool_free++;
		hlist_add_head(&obj->node, &obj_pool);
	}
	unlock(pool);
	return work;
}

static void free_object(obj)
{
	if (__free_object(obj))
		schedule_work(&debug_obj_work);
}

and then use __free_object() in __debug_check_no_obj_freed()

    	 bool work = false;

	 ...
 	    	 work |= __free_object(obj);
	 ...

	 if (work)
		schedule_work(&debug_obj_work);

That makes the whole thing simpler and the accounting is matching the place
where the object is:

      obj_pool_free counts the number of objects enqueued in obj_pool
      obj_nr_tofree counts the number of objects enqueued in obj_to_free
      obr_pool_used counts the number of objects enqueued in the hash lists

Ideally you split that patch into pieces:

1) Introduce obj_to_free/obj_nr_tofree and add the removing/freeing from it
   in fill_pool() and free_obj_work(). Nothing adds an object at this point
   to obj_to_free.

2) Change free_object() to use obj_to_free and split it apart

3) Change __debug_check_no_obj_freed() to use __free_object()

That makes it simpler to review and to follow.

Hmm?

Thanks,

	tglx

Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

[Yang Shi]

On 2/1/18 1:36 PM, Thomas Gleixner wrote:
> On Fri, 2 Feb 2018, Yang Shi wrote:
>
>>   /*
>> - * Allocate a new object. If the pool is empty, switch off the debugger.
>> + * Allocate a new object. Retrieve from global freelist first. If the pool is
>> + * empty, switch off the debugger.
>>    * Must be called with interrupts disabled.
>>    */
>>   static struct debug_obj *
>> @@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
>>   	struct debug_obj *obj = NULL;
>>   
>>   	raw_spin_lock(&pool_lock);
> Why in alloc_object() and not in fill_pool()?
>
>> +	if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
>> +		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>> +		obj_nr_tofree--;
>> +		hlist_del(&obj->node);
>> +		goto out;
>> +	}
> Errm. This is wrong. It does not reinitialize the object. Please do that
> shuffling in fill_pool().

OK, will move the reuse logic into fill_pool().

>
>>   	if (obj_pool.first) {
>>   		obj	    = hlist_entry(obj_pool.first, typeof(*obj), node);
> ....
>
>> +	/* When pool list is not full move free objs to pool list */
>> +	while (obj_pool_free < debug_objects_pool_size) {
>> +		if (obj_nr_tofree <= 0)
>> +			break;
>> +
>> +		obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>> +		hlist_del(&obj->node);
>> +		hlist_add_head(&obj->node, &obj_pool);
>> +		obj_pool_free++;
>> +		obj_pool_used--;
>> +		obj_nr_tofree--;
>> +	}
>> +
>> +	/*
>> +	 * pool list is already full, and there are still objs on the free list,
>> +	 * move remaining free objs to a separate list to free the memory later.
>> +	 */
>> +	if (obj_nr_tofree > 0) {
>> +		hlist_move_list(&obj_to_free, &tofree);
>> +		obj_nr_tofree = 0;
>> +	}
> The accounting is inconsistent. You leak obj_pool_used. But that's wrong
> anyway because an object should not be accounted for in two places. It's
> only on _ONE_ list....

So I should move the accounting to where the obj is deleted from the 
list? It should look like:

if (obj_nr_tofree > 0)
         hlist_move_list(&obj_to_free, &tofree);

...

if (!hlist_empty(&tofree)) {
                 hlist_for_each_entry_safe(obj, tmp, &tofree, node) {
                         hlist_del(&obj->node);
                         obj_nr_tofree--;
                         kmem_cache_free(obj_cache, obj);
                 }
         }

>
>> @@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>>   {
>>   	unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
>>   	struct hlist_node *tmp;
>> -	HLIST_HEAD(freelist);
>>   	struct debug_obj_descr *descr;
>>   	enum debug_obj_state state;
>>   	struct debug_bucket *db;
>> @@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>>   				goto repeat;
>>   			default:
>>   				hlist_del(&obj->node);
>> -				hlist_add_head(&obj->node, &freelist);
>> +				/* Put obj on the global free list */
>> +				raw_spin_lock(&pool_lock);
>> +				hlist_add_head(&obj->node, &obj_to_free);
>> +				/* Update the counter of objs on the global freelist */
>> +				obj_nr_tofree++;
>> +				raw_spin_unlock(&pool_lock);
> As we have to take pool_lock anyway, we simply can change free_object() to:
>
> static bool __free_object(obj)
> {
> 	bool work;
>
> 	lock(pool);
> 	work = obj_pool_free > debug_objects_pool_size && obj_cache;
> 	obj_pool_used++;
> 	if (work) {
> 		obj_nr_tofree++;
> 		hlist_add_head(&obj->node, &obj_to_free);
> 	] else {
> 		obj_pool_free++;
> 		hlist_add_head(&obj->node, &obj_pool);
> 	}
> 	unlock(pool);
> 	return work;
> }
>
> static void free_object(obj)
> {
> 	if (__free_object(obj))
> 		schedule_work(&debug_obj_work);
> }
>
> and then use __free_object() in __debug_check_no_obj_freed()
>
>      	 bool work = false;
>
> 	 ...
>   	    	 work |= __free_object(obj);
> 	 ...
>
> 	 if (work)
> 		schedule_work(&debug_obj_work);
>
> That makes the whole thing simpler and the accounting is matching the place
> where the object is:
>
>        obj_pool_free counts the number of objects enqueued in obj_pool
>        obj_nr_tofree counts the number of objects enqueued in obj_to_free
>        obr_pool_used counts the number of objects enqueued in the hash lists
>
> Ideally you split that patch into pieces:
>
> 1) Introduce obj_to_free/obj_nr_tofree and add the removing/freeing from it
>     in fill_pool() and free_obj_work(). Nothing adds an object at this point
>     to obj_to_free.
>
> 2) Change free_object() to use obj_to_free and split it apart
>
> 3) Change __debug_check_no_obj_freed() to use __free_object()
>
> That makes it simpler to review and to follow.
>
> Hmm?

Sure, will refactor free_object() and split the patches in newer version.

Thanks,
Yang

>
> Thanks,
>
> 	tglx

Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

[Yang Shi]

On 2/1/18 3:01 PM, Yang Shi wrote:
>
>
> On 2/1/18 1:36 PM, Thomas Gleixner wrote:
>> On Fri, 2 Feb 2018, Yang Shi wrote:
>>
>>>   /*
>>> - * Allocate a new object. If the pool is empty, switch off the 
>>> debugger.
>>> + * Allocate a new object. Retrieve from global freelist first. If 
>>> the pool is
>>> + * empty, switch off the debugger.
>>>    * Must be called with interrupts disabled.
>>>    */
>>>   static struct debug_obj *
>>> @@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void 
>>> *addr, struct debug_bucket *b)
>>>       struct debug_obj *obj = NULL;
>>>         raw_spin_lock(&pool_lock);
>> Why in alloc_object() and not in fill_pool()?
>>
>>> +    if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
>>> +        obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>>> +        obj_nr_tofree--;
>>> +        hlist_del(&obj->node);
>>> +        goto out;
>>> +    }
>> Errm. This is wrong. It does not reinitialize the object. Please do that
>> shuffling in fill_pool().
>
> OK, will move the reuse logic into fill_pool().
>
>>
>>>       if (obj_pool.first) {
>>>           obj        = hlist_entry(obj_pool.first, typeof(*obj), node);
>> ....
>>
>>> +    /* When pool list is not full move free objs to pool list */
>>> +    while (obj_pool_free < debug_objects_pool_size) {
>>> +        if (obj_nr_tofree <= 0)
>>> +            break;
>>> +
>>> +        obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>>> +        hlist_del(&obj->node);
>>> +        hlist_add_head(&obj->node, &obj_pool);
>>> +        obj_pool_free++;
>>> +        obj_pool_used--;
>>> +        obj_nr_tofree--;
>>> +    }
>>> +
>>> +    /*
>>> +     * pool list is already full, and there are still objs on the 
>>> free list,
>>> +     * move remaining free objs to a separate list to free the 
>>> memory later.
>>> +     */
>>> +    if (obj_nr_tofree > 0) {
>>> +        hlist_move_list(&obj_to_free, &tofree);
>>> +        obj_nr_tofree = 0;
>>> +    }
>> The accounting is inconsistent. You leak obj_pool_used. But that's wrong
>> anyway because an object should not be accounted for in two places. It's
>> only on _ONE_ list....
>
> So I should move the accounting to where the obj is deleted from the 
> list? It should look like:

I got your point here. Yes, obj_pool_used should be not decreased here 
since it has not been allocated from pool list.

But, I think obj_nr_tofree counter should be cleared since all the objs 
are *NOT* on the global free list anymore. They will be freed later. 
And, we can't decrease the obj_nr_tofree counter later without acquiring 
pool lock.

>
> if (obj_nr_tofree > 0)
>         hlist_move_list(&obj_to_free, &tofree);
>
> ...
>
> if (!hlist_empty(&tofree)) {
>                 hlist_for_each_entry_safe(obj, tmp, &tofree, node) {
>                         hlist_del(&obj->node);
>                         obj_nr_tofree--;
>                         kmem_cache_free(obj_cache, obj);
>                 }
>         }
>
>>
>>> @@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const 
>>> void *address, unsigned long size)
>>>   {
>>>       unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
>>>       struct hlist_node *tmp;
>>> -    HLIST_HEAD(freelist);
>>>       struct debug_obj_descr *descr;
>>>       enum debug_obj_state state;
>>>       struct debug_bucket *db;
>>> @@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const 
>>> void *address, unsigned long size)
>>>                   goto repeat;
>>>               default:
>>>                   hlist_del(&obj->node);
>>> -                hlist_add_head(&obj->node, &freelist);
>>> +                /* Put obj on the global free list */
>>> +                raw_spin_lock(&pool_lock);
>>> +                hlist_add_head(&obj->node, &obj_to_free);
>>> +                /* Update the counter of objs on the global 
>>> freelist */
>>> +                obj_nr_tofree++;
>>> +                raw_spin_unlock(&pool_lock);
>> As we have to take pool_lock anyway, we simply can change 
>> free_object() to:
>>
>> static bool __free_object(obj)
>> {
>>     bool work;
>>
>>     lock(pool);
>>     work = obj_pool_free > debug_objects_pool_size && obj_cache;
>>     obj_pool_used++;

Should it be decreased here since the obj is being dequeued from hlist?

Thanks,
Yang

>>     if (work) {
>>         obj_nr_tofree++;
>>         hlist_add_head(&obj->node, &obj_to_free);
>>     ] else {
>>         obj_pool_free++;
>>         hlist_add_head(&obj->node, &obj_pool);
>>     }
>>     unlock(pool);
>>     return work;
>> }
>>
>> static void free_object(obj)
>> {
>>     if (__free_object(obj))
>>         schedule_work(&debug_obj_work);
>> }
>>
>> and then use __free_object() in __debug_check_no_obj_freed()
>>
>>           bool work = false;
>>
>>      ...
>>                work |= __free_object(obj);
>>      ...
>>
>>      if (work)
>>         schedule_work(&debug_obj_work);
>>
>> That makes the whole thing simpler and the accounting is matching the 
>> place
>> where the object is:
>>
>>        obj_pool_free counts the number of objects enqueued in obj_pool
>>        obj_nr_tofree counts the number of objects enqueued in 
>> obj_to_free
>>        obr_pool_used counts the number of objects enqueued in the 
>> hash lists
>>
>> Ideally you split that patch into pieces:
>>
>> 1) Introduce obj_to_free/obj_nr_tofree and add the removing/freeing 
>> from it
>>     in fill_pool() and free_obj_work(). Nothing adds an object at 
>> this point
>>     to obj_to_free.
>>
>> 2) Change free_object() to use obj_to_free and split it apart
>>
>> 3) Change __debug_check_no_obj_freed() to use __free_object()
>>
>> That makes it simpler to review and to follow.
>>
>> Hmm?
>
> Sure, will refactor free_object() and split the patches in newer version.
>
> Thanks,
> Yang
>
>>
>> Thanks,
>>
>>     tglx

Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

[Thomas Gleixner]

On Fri, 2 Feb 2018, Yang Shi wrote:
> On 2/1/18 3:01 PM, Yang Shi wrote:
> > On 2/1/18 1:36 PM, Thomas Gleixner wrote:
> > > The accounting is inconsistent. You leak obj_pool_used. But that's wrong
> > > anyway because an object should not be accounted for in two places. It's
> > > only on _ONE_ list....
> > 
> > So I should move the accounting to where the obj is deleted from the list?
> > It should look like:
> 
> I got your point here. Yes, obj_pool_used should be not decreased here since
> it has not been allocated from pool list.
> 
> But, I think obj_nr_tofree counter should be cleared since all the objs are
> *NOT* on the global free list anymore. They will be freed later. And, we can't
> decrease the obj_nr_tofree counter later without acquiring pool lock.

Right, when you split the list to the temporary tofree list head then you
need to reset obj_nr_tofree as well. And that's correct because the objects
are not longer reachable through one of the global lists.

> > > static bool __free_object(obj)
> > > {
> > >     bool work;
> > > 
> > >     lock(pool);
> > >     work = obj_pool_free > debug_objects_pool_size && obj_cache;
> > >     obj_pool_used++;
> 
> Should it be decreased here since the obj is being dequeued from hlist?

I guess so :)

Thanks,

	tglx