linux-kernel - Re: [PATCH] writeback: call writeback tracepoints withoud holding list_lock in wb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 25 Feb 2016 18:31:07 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	"Shi, Yang" <yang.shi@...aro.org>
Cc:	tj@...nel.org, jack@...e.cz, axboe@...com, fengguang.wu@...el.com,
	tglx@...utronix.de, bigeasy@...utronix.de,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, linux-rt-users@...r.kernel.org,
	linaro-kernel@...ts.linaro.org
Subject: Re: [PATCH] writeback: call writeback tracepoints withoud holding
 list_lock in wb_writeback()

On Thu, 25 Feb 2016 15:16:54 -0800
"Shi, Yang" <yang.shi@...aro.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.

> 
> >  
> >>  
> >>>
> >>>  
> >>>> 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.

Is there a way to not use the kernfs_* function? At least for -rt?

-- Steve