lkml.org 
[lkml]   [2016]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] writeback: call writeback tracepoints withoud holding list_lock in wb_writeback()
From
Date
On 2/25/2016 3:47 PM, Shi, Yang wrote:
> On 2/25/2016 3:31 PM, Steven Rostedt wrote:
>> On Thu, 25 Feb 2016 15:16:54 -0800
>> "Shi, Yang" <yang.shi@linaro.org> wrote:
>>
>>
>>> Actually, regardless whether this is the right fix for the splat, it
>>> makes me be wondering if the spin lock which protects the whole for loop
>>> is really necessary. It sounds feasible to move it into the for loop and
>>> just protect the necessary area.
>>
>> That's a separate issue, which may have its own merits that should be
>> decided by the writeback folks.
>
> Yes, definitely. I will rework my commit log for this part.
>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> INFO: lockdep is turned off.
>>>>>>> Preemption disabled at:[<ffffffc000374a5c>] wb_writeback+0xec/0x830
>>>>
>>>> Can you disassemble the vmlinux file to see exactly where that call is.
>>>> I use gdb to find the right locations.
>>>>
>>>> gdb> li *0xffffffc000374a5c
>>>> gdb> disass 0xffffffc000374a5c
>>>
>>> I use gdb to get the code too.
>>>
>>> It does point to the spin_lock.
>>>
>>> (gdb) list *0xffffffc000374a5c
>>> 0xffffffc000374a5c is in wb_writeback (fs/fs-writeback.c:1621).
>>> 1616
>>> 1617 oldest_jif = jiffies;
>>> 1618 work->older_than_this = &oldest_jif;
>>> 1619
>>> 1620 blk_start_plug(&plug);
>>> 1621 spin_lock(&wb->list_lock);
>>> 1622 for (;;) {
>>> 1623 /*
>>> 1624 * Stop writeback when nr_pages has been
>>> consumed
>>> 1625 */
>>>
>>>
>>> The disassemble:
>>> 0xffffffc000374a58 <+232>: bl 0xffffffc0001300b0
>>
>> The above is the place it recorded. But I just realized, this isn't the
>> issue. I know where the problem is.
>>
>>
>>> <migrate_disable>
>>> 0xffffffc000374a5c <+236>: mov x0, x22
>>> 0xffffffc000374a60 <+240>: bl 0xffffffc000d5d518
>>> <rt_spin_lock>
>>>
>>>>
>>
>>
>>
>>>>> DECLARE_EVENT_CLASS(writeback_work_class,
>>>>> TP_PROTO(struct bdi_writeback *wb, struct
>>>>> wb_writeback_work *work),
>>>>> TP_ARGS(wb, work),
>>>>> TP_STRUCT__entry(
>>>>> __array(char, name, 32)
>>>>> __field(long, nr_pages)
>>>>> __field(dev_t, sb_dev)
>>>>> __field(int, sync_mode)
>>>>> __field(int, for_kupdate)
>>>>> __field(int, range_cyclic)
>>>>> __field(int, for_background)
>>>>> __field(int, reason)
>>>>> __dynamic_array(char, cgroup,
>>>>> __trace_wb_cgroup_size(wb))
>>>>>
>>>>
>>>> Ah, thanks for pointing that out. I missed that.
>>>
>>> It sounds not correct if tracepoint doesn't allow sleep.
>>>
>>> I considered to change sleeping lock to raw lock in kernfs_* functions,
>>> but it sounds not reasonable since they are used heavily by cgroup.
>>
>> It is the kernfs_* that can't sleep. Tracepoints use
>> rcu_read_lock_sched_notrace(), which disables preemption, and not only
>> that, hides itself from lockdep as the last place to disable preemption.
>
> Ah, thanks for pointing out this.
>
>>
>> Is there a way to not use the kernfs_* function? At least for -rt?
>
> I'm not quite sure if there is straightforward replacement. However, I'm
> wondering if lock free version could be used by tracing.
>
> For example, create __kernfs_path_len which doesn't acquire any lock for
> writeback tracing as long as there is not any race condition.
>
> At least we could rule out preemption.

Can we disable irqs in tracepoints since spin_lock_irqsave is used by
kernfs_* functions.

Thanks,
Yang

>
> Thanks,
> Yang
>
>>
>> -- Steve
>>
>

\
 
 \ /
  Last update: 2016-02-26 01:21    [W:0.045 / U:1.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site