Messages in this thread | | | Subject | Re: [PATCH 2/2 v4] lib: debugobjects: handle objects free in a batch outside the loop | From | Yang Shi <> | Date | Wed, 31 Jan 2018 10:51:13 -0800 |
| |
On 1/31/18 8:01 AM, Thomas Gleixner wrote: > On Thu, 25 Jan 2018, Yang Shi wrote: > >> 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 >> task: ffff884cbb0d0000 task.stack: ffff884cabc70000 >> RIP: 0010:[<ffffffff817d647b>] [<ffffffff817d647b>] >> _raw_spin_unlock_irqrestore+0x3b/0x60 >> RSP: 0018:ffff884cabc77b78 EFLAGS: 00000292 >> RAX: ffff884cbb0d0000 RBX: 0000000000000292 RCX: 0000000000000000 >> RDX: ffff884cbb0d0000 RSI: 0000000000000001 RDI: 0000000000000292 >> RBP: ffff884cabc77b88 R08: 0000000000000000 R09: 0000000000000000 >> R10: 0000000000000001 R11: 0000000000000001 R12: ffffffff8357a0d8 >> R13: ffff884cabc77bc8 R14: ffffffff8357a0d0 R15: 00000000000000fc >> FS: 00002aee845fd2c0(0000) GS:ffff8852bd400000(0000) >> knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 0000000002991808 CR3: 0000005123abf000 CR4: 00000000000406e0 >> Stack: >> ffff884ff4fe0000 ffff884ff4fd8000 ffff884cabc77c00 ffffffff8141177e >> 0000000000000202 ffff884cbb0d0000 ffff884cabc77bc8 0000000000000006 >> ffff884ff4fda000 ffffffff8357a0d8 0000000000000000 91f5d976f6020b6c > Please trim that. The register content is not really interesting
Yes, sure.
> >> @@ -186,11 +199,25 @@ 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; >> >> if (!raw_spin_trylock_irqsave(&pool_lock, flags)) >> return; >> + >> + /* Move free obj to pool list from global free list */ > If the pool is full, why would you shuffle those objects to the pool list > first just to free them 10 lines further down?
Yes, thanks for pointing out this. I should checked if pool list is full or not before moving free objects to pool list.
> >> + if (obj_free > 0) { > obj_nr_tofree might be a more descriptive name for this variable
OK.
> >> + hlist_for_each_entry_safe(obj, tmp, &obj_to_free, node) { >> + hlist_del(&obj->node); >> + hlist_add_head(&obj->node, &obj_pool); >> + obj_pool_free++; >> + obj_pool_used--; >> + obj_free--; >> + } >> + } > The other thing here is that this whole list walk operation happens with > the pool lock held and interrupts disabled. That's suboptimal at best. > > So the right thing to do here is: > > HLIST_HEAD(tofree); > > if (!raw_spin_trylock_irqsave(&pool_lock, flags)) > return; > > while (obj_pool_free < debug_objects_pool_size) { > if (!obj_nr_tofree) > break: > hlist_del(...) > hlist_add(...) > } > > if (obj_nr_tofree) { > hlist_move_list(&obj_to_free, &freelist); > .... > } > > while (obj_pool_free >= debug_objects_pool_size + ODEBUG_FREE_BATCH) { > .... > } > > raw_spin_unlock_irqrestore(&pool_lock, flags); > > hlist_for_each_entry_safe(obj, tmp, &tofree, node) { > hlist_del(); > kmem_cache_free(....); > } > > That way you minimize the lock held times and spare pointless list walks > and shuffling.
Thanks a lot for the suggestion, will fix these in new version.
Regards, Yang
> > Thanks, > > tglx
| |