linux-kernel - Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless

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:	Tue, 27 Oct 2015 12:37:16 +0900
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Tejun Heo <tj@...nel.org>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Dipankar Sarma <dipankar@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Josh Triplett <josh@...htriplett.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	David Howells <dhowells@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Darren Hart <dvhart@...ux.intel.com>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Oleg Nesterov <oleg@...hat.com>,
	pranith kumar <bobby.prani@...il.com>,
	Patrick Marlier <patrick.marlier@...il.com>
Subject: Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()

On Mon, Oct 26, 2015 at 11:55 PM, Paul E. McKenney
<paulmck@...ux.vnet.ibm.com> wrote:
>>         struct bdi_writeback *last_wb = NULL;
>>         struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
>
> I believe that the above should instead be:
>
>         struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

I don't think you can do that.

You haven't even taken the RCU read lock yet at this point.

What the code seems to try to do is to get the "head pointer" of the
list before taking the read lock (since _that_ is stable), and then
follow the list under the lock.

You're making it actually follow the first RCU pointer too early.

That said, I'm not sure why it doesn't just do the normal

    rcu_read_lock();
    list_for_each_entry_rcu(wb, &bdi->wb_list, bdi_node) {
        ....
    }
    rcu_read_unlock();

like the other places do. It looks like it wants that
"list_for_each_entry_continue_rcu()" because it does that odd "pin
entry and drop rcu lock and retake it and continue where you left
off", but I'm not sure why the continue version would be so
different.. It's going to do that "follow next entry" regardless, and
the "goto restart" doesn't look like it actually adds anything. If
following the next pointer is ok even after having released the RCU
read lock, then I'm not seeing why the end of the loop couldn't just
do

                rcu_read_unlock();
                wb_wait_for_completion(bdi, &fallback_work_done);
                rcu_read_lock();

and just continue the loop (and the pinning of "wb" and releasing the
"last_wb" thing in the *next* iteration should make it all work the
same).

Adding Tejun to the cc, because this is his code and there's probably
something subtle I'm missing. Tejun, can you take a look? It's
bdi_split_work_to_wbs() in fs/fs-writeback.c.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/