LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Paul McKenney" <paulmck@linux.vnet.ibm.com>,
	"Ingo Molnar" <mingo@kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Lai Jiangshan" <jiangshanlai@gmail.com>,
	"Dipankar Sarma" <dipankar@in.ibm.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>,
	"Josh Triplett" <josh@joshtriplett.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"David Howells" <dhowells@redhat.com>,
	"Eric Dumazet" <edumazet@google.com>,
	"Darren Hart" <dvhart@linux.intel.com>,
	"Frédéric Weisbecker" <fweisbec@gmail.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"pranith kumar" <bobby.prani@gmail.com>,
	"Patrick Marlier" <patrick.marlier@gmail.com>
Subject: Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()
Date: Tue, 27 Oct 2015 14:19:39 +0900
Message-ID: <20151027051939.GA19355@mtj.duckdns.org> (raw)
In-Reply-To: <CA+55aFxHeM2vceDGC7YnDMfN4qu0DYVN=4iRGuC-naj1VnDw_w@mail.gmail.com>

Hello,

On Tue, Oct 27, 2015 at 12:37:16PM +0900, Linus Torvalds wrote:
> > I believe that the above should instead be:
> >
> >         struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

I should have just used list_entry() here.  It's just offseting the
pointer to set up the initial iteration point.

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

Yeah, just releasing and regrabbing should work too as the iterator
doesn't depend on anything other than the current entry (e.g. as
opposed to imaginary list_for_each_entry_safe_rcu()).  It's slightly
icky to meddle with locking behind the iterator's back tho.  Either
way should be fine but how about something like the following?

Subject: writeback: don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

bdi_split_work_to_wbs() uses list_for_each_entry_rcu_continue() to
walk @bdi->wb_list.  To set up the initial iteration condition, it
uses list_entry_rcu() to calculate the entry pointer corresponding to
the list head; however, this isn't an actual RCU dereference and using
list_entry_rcu() for it ended up breaking a proposed list_entry_rcu()
change because it was feeding an non-lvalue pointer into the macro.

Don't use the RCU variant for simple pointer offsetting.  Use
list_entry() instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fs-writeback.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 29e4599..7378169 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -779,8 +779,8 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
 				  bool skip_if_busy)
 {
 	struct bdi_writeback *last_wb = NULL;
-	struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
-						struct bdi_writeback, bdi_node);
+	struct bdi_writeback *wb = list_entry(&bdi->wb_list,
+					      struct bdi_writeback, bdi_node);
 
 	might_sleep();
 restart:

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-06 16:13 [PATCH tip/core/rcu 0/13] Miscellaneous fixes for 4.4 Paul E. McKenney
2015-10-06 16:13 ` [PATCH tip/core/rcu 01/13] sched: Export sched_setscheduler_nocheck Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 02/13] rcu: Use rcu_callback_t in call_rcu*() and friends Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 03/13] rcu: Use call_rcu_func_t to replace explicit type equivalents Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 04/13] rcu: Don't disable preemption for Tiny and Tree RCU readers Paul E. McKenney
2015-10-06 16:20     ` [Kernel networking modules.] OSI levels 2 & 3, Assistance - If anyone knows anyone in the US. North West region John D Allen, Leveridge Systems INC
2015-10-06 16:44     ` [PATCH tip/core/rcu 04/13] rcu: Don't disable preemption for Tiny and Tree RCU readers Josh Triplett
2015-10-06 17:01       ` Paul E. McKenney
2015-10-06 17:16         ` Josh Triplett
2015-10-06 17:42           ` Paul E. McKenney
2015-10-06 17:46             ` Josh Triplett
2015-10-06 20:05             ` Peter Zijlstra
2015-10-06 20:18               ` Paul E. McKenney
2015-10-06 20:52                 ` Peter Zijlstra
2015-10-06 21:05                   ` Paul E. McKenney
2015-10-07  7:19                     ` Peter Zijlstra
2015-10-06 16:13   ` [PATCH tip/core/rcu 05/13] rcu: Eliminate panic when silly boot-time fanout specified Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 06/13] rcu: Add online/offline info to stall warning message Paul E. McKenney
2015-10-06 17:15     ` Josh Triplett
2015-10-06 16:13   ` [PATCH tip/core/rcu 07/13] rcu: Move preemption disabling out of __srcu_read_lock() Paul E. McKenney
2015-10-06 17:18     ` Josh Triplett
2015-10-06 17:36       ` Paul E. McKenney
2015-10-06 17:43         ` Josh Triplett
2015-10-06 18:07           ` Paul E. McKenney
2015-10-06 20:07     ` Peter Zijlstra
2015-10-06 20:19       ` Paul E. McKenney
2015-10-06 20:32         ` Peter Zijlstra
2015-10-06 21:03           ` Paul E. McKenney
2015-10-07  7:20             ` Peter Zijlstra
2015-10-07 14:18               ` Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 08/13] rcu: Finish folding ->fqs_state into ->gp_state Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 09/13] rcu: Correct comment for values of ->gp_state field Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 10/13] rcu: Add rcu_pointer_handoff() Paul E. McKenney
2015-10-06 17:21     ` Josh Triplett
2015-10-06 17:31       ` Paul E. McKenney
2015-10-06 17:36         ` Josh Triplett
2015-10-06 20:27     ` Peter Zijlstra
2015-10-06 21:02       ` Paul E. McKenney
2015-10-07  7:22         ` Peter Zijlstra
2015-10-07 14:20           ` Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference() Paul E. McKenney
2015-10-26  8:45     ` Ingo Molnar
2015-10-26 14:55       ` Paul E. McKenney
2015-10-26 18:02         ` Ingo Molnar
2015-10-27  3:37         ` Linus Torvalds
2015-10-27  5:19           ` Tejun Heo [this message]
2015-10-27  5:33             ` Paul E. McKenney
2015-10-28  8:33             ` Ingo Molnar
2015-10-28 20:35               ` Patrick Marlier
2015-10-29  0:00                 ` Paul E. McKenney
2015-10-29  2:13                   ` Tejun Heo
2015-10-28 20:58             ` [tip:core/rcu] fs/writeback, rcu: Don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs() tip-bot for Tejun Heo
2015-10-27  5:32           ` [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference() Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 12/13] rcu: Remove deprecated rcu_lockdep_assert() Paul E. McKenney
2015-10-06 16:13   ` [PATCH tip/core/rcu 13/13] rculist: Use WRITE_ONCE() when deleting from reader-visible list Paul E. McKenney
2015-10-06 17:23 ` [PATCH tip/core/rcu 0/13] Miscellaneous fixes for 4.4 Josh Triplett
2015-10-06 17:38   ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151027051939.GA19355@mtj.duckdns.org \
    --to=tj@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=bobby.prani@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=dipankar@in.ibm.com \
    --cc=dvhart@linux.intel.com \
    --cc=edumazet@google.com \
    --cc=fweisbec@gmail.com \
    --cc=jiangshanlai@gmail.com \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=patrick.marlier@gmail.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git