lkml.org 
[lkml]   [2018]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2 v4] lib: debugobjects: handle objects free in a batch outside the loop
From
Date


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

\
 
 \ /
  Last update: 2018-01-31 19:52    [W:0.046 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site