* [PATCH 1/2 v5] lib: debugobjects: export max loops counter @ 2018-02-01 18:15 Yang Shi 2018-02-01 18:15 ` [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop Yang Shi 0 siblings, 1 reply; 6+ messages in thread From: Yang Shi @ 2018-02-01 18:15 UTC (permalink / raw) To: tglx, longman; +Cc: yang.shi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop 2018-02-01 18:15 [PATCH 1/2 v5] lib: debugobjects: export max loops counter Yang Shi @ 2018-02-01 18:15 ` Yang Shi 2018-02-01 21:36 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Yang Shi @ 2018-02-01 18:15 UTC (permalink / raw) To: tglx, longman; +Cc: yang.shi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop 2018-02-01 18:15 ` [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop Yang Shi @ 2018-02-01 21:36 ` Thomas Gleixner 2018-02-01 23:01 ` Yang Shi 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2018-02-01 21:36 UTC (permalink / raw) To: Yang Shi; +Cc: longman, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop 2018-02-01 21:36 ` Thomas Gleixner @ 2018-02-01 23:01 ` Yang Shi 2018-02-02 19:52 ` Yang Shi 0 siblings, 1 reply; 6+ messages in thread From: Yang Shi @ 2018-02-01 23:01 UTC (permalink / raw) To: Thomas Gleixner; +Cc: longman, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop 2018-02-01 23:01 ` Yang Shi @ 2018-02-02 19:52 ` Yang Shi 2018-02-02 20:44 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Yang Shi @ 2018-02-02 19:52 UTC (permalink / raw) To: Thomas Gleixner; +Cc: longman, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop 2018-02-02 19:52 ` Yang Shi @ 2018-02-02 20:44 ` Thomas Gleixner 0 siblings, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2018-02-02 20:44 UTC (permalink / raw) To: Yang Shi; +Cc: longman, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-02 20:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-01 18:15 [PATCH 1/2 v5] lib: debugobjects: export max loops counter Yang Shi 2018-02-01 18:15 ` [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop Yang Shi 2018-02-01 21:36 ` Thomas Gleixner 2018-02-01 23:01 ` Yang Shi 2018-02-02 19:52 ` Yang Shi 2018-02-02 20:44 ` Thomas Gleixner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.