Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255749 > unrolled thread

Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()

Started byIngo Molnar <mingo@kernel.org>
First post2015-10-26 09:50 +0100
Last post2015-10-27 06:40 +0100
Articles 12 — 6 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Ingo Molnar <mingo@kernel.org> - 2015-10-26 09:50 +0100
    Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-26 16:00 +0100
      Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Ingo Molnar <mingo@kernel.org> - 2015-10-26 19:10 +0100
      Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference() Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-27 04:40 +0100
        Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Tejun Heo <tj@kernel.org> - 2015-10-27 06:20 +0100
          Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-27 06:40 +0100
          Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Ingo Molnar <mingo@kernel.org> - 2015-10-28 09:40 +0100
            Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Patrick Marlier <patrick.marlier@gmail.com> - 2015-10-28 21:40 +0100
              Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-29 01:10 +0100
                Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() Tejun Heo <tj@kernel.org> - 2015-10-29 03:20 +0100
          [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 <tipbot@zytor.com> - 2015-10-28 22:00 +0100
        Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use  lockless_dereference() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-27 06:40 +0100

#1255749 — Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()

FromIngo Molnar <mingo@kernel.org>
Date2015-10-26 09:50 +0100
SubjectRe: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()
Message-ID<qnLDz-60G-1@gated-at.bofh.it>
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> From: Patrick Marlier <patrick.marlier@gmail.com>
> 
> The current list_entry_rcu() implementation copies the pointer to a stack
> variable, then invokes rcu_dereference_raw() on it.  This results in an
> additional store-load pair.  Now, most compilers will emit normal store
> and load instructions, which might seem to be of negligible overhead,
> but this results in a load-hit-store situation that can cause surprisingly
> long pipeline stalls, even on modern microprocessors.  The problem is
> that it takes time for the store to get the store buffer updated, which
> can delay the subsequent load, which immediately follows.
> 
> This commit therefore switches to the lockless_dereference() primitive,
> which does not expect the __rcu annotations (that are anyway not present
> in the list_head structure) and which, like rcu_dereference_raw(),
> does not check for an enclosing RCU read-side critical section.
> Most importantly, it does not copy the pointer, thus avoiding the
> load-hit-store overhead.
> 
> Signed-off-by: Patrick Marlier <patrick.marlier@gmail.com>
> [ paulmck: Switched to lockless_dereference() to suppress sparse warnings. ]
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/rculist.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 17c6b1f84a77..5ed540986019 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
>   * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
>   */
>  #define list_entry_rcu(ptr, type, member) \
> -({ \
> -	typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
> -	container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
> -})
> +	container_of(lockless_dereference(ptr), type, member)

So this commit:

  8db70b132dd5 ("rculist: Make list_entry_rcu() use lockless_dereference()")

when merged with Linus's latest tree, triggers the following build failure on 
allyesconfig/allmodconfig x86:

triton:~/tip> make fs/fs-writeback.o
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      fs/fs-writeback.o
In file included from fs/fs-writeback.c:16:0:
fs/fs-writeback.c: In function ‘bdi_split_work_to_wbs’:
include/linux/compiler.h:281:20: error: lvalue required as unary ‘&’ operand
   __read_once_size(&(x), __u.__c, sizeof(x));  \
                    ^
include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                 ^
include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
 #define READ_ONCE(x) __READ_ONCE(x, 1)
                      ^
include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
  typeof(p) _________p1 = READ_ONCE(p); \
                          ^
include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
  container_of(lockless_dereference(ptr), type, member)
               ^
fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
  struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
                             ^
include/linux/compiler.h:283:28: error: lvalue required as unary ‘&’ operand
   __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
                            ^
include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                 ^
include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
 #define READ_ONCE(x) __READ_ONCE(x, 1)
                      ^
include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
  typeof(p) _________p1 = READ_ONCE(p); \
                          ^
include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
  container_of(lockless_dereference(ptr), type, member)
               ^
fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
  struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
                             ^
scripts/Makefile.build:258: recipe for target 'fs/fs-writeback.o' failed
make[1]: *** [fs/fs-writeback.o] Error 1
Makefile:1526: recipe for target 'fs/fs-writeback.o' failed
make: *** [fs/fs-writeback.o] Error 2

It's this new usage in fs/fs-writeback.c:

static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
                                  struct wb_writeback_work *base_work,
                                  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);

so AFAICS the problem is lockless_dereference() not being able to take this valid 
but non-lvalue list head, because READ_ONCE() does not take non-lvalues. All other 
existing uses of list_entry_rcu() happen to use lvalues, so this bug didn't get 
triggered before.

For now I've reverted this commit locally, to make the kernel build and so, but we 
need a cleaner solution I suspect.

Thanks,

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

[toc] | [next] | [standalone]

#1256088

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2015-10-26 16:00 +0100
Message-ID<qnRpD-143-5@gated-at.bofh.it>
In reply to#1255749
On Mon, Oct 26, 2015 at 09:45:06AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > From: Patrick Marlier <patrick.marlier@gmail.com>
> > 
> > The current list_entry_rcu() implementation copies the pointer to a stack
> > variable, then invokes rcu_dereference_raw() on it.  This results in an
> > additional store-load pair.  Now, most compilers will emit normal store
> > and load instructions, which might seem to be of negligible overhead,
> > but this results in a load-hit-store situation that can cause surprisingly
> > long pipeline stalls, even on modern microprocessors.  The problem is
> > that it takes time for the store to get the store buffer updated, which
> > can delay the subsequent load, which immediately follows.
> > 
> > This commit therefore switches to the lockless_dereference() primitive,
> > which does not expect the __rcu annotations (that are anyway not present
> > in the list_head structure) and which, like rcu_dereference_raw(),
> > does not check for an enclosing RCU read-side critical section.
> > Most importantly, it does not copy the pointer, thus avoiding the
> > load-hit-store overhead.
> > 
> > Signed-off-by: Patrick Marlier <patrick.marlier@gmail.com>
> > [ paulmck: Switched to lockless_dereference() to suppress sparse warnings. ]
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  include/linux/rculist.h | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index 17c6b1f84a77..5ed540986019 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
> >   * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
> >   */
> >  #define list_entry_rcu(ptr, type, member) \
> > -({ \
> > -	typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
> > -	container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
> > -})
> > +	container_of(lockless_dereference(ptr), type, member)
> 
> So this commit:
> 
>   8db70b132dd5 ("rculist: Make list_entry_rcu() use lockless_dereference()")
> 
> when merged with Linus's latest tree, triggers the following build failure on 
> allyesconfig/allmodconfig x86:
> 
> triton:~/tip> make fs/fs-writeback.o
>   CHK     include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CHK     include/generated/bounds.h
>   CHK     include/generated/timeconst.h
>   CHK     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      fs/fs-writeback.o
> In file included from fs/fs-writeback.c:16:0:
> fs/fs-writeback.c: In function ‘bdi_split_work_to_wbs’:
> include/linux/compiler.h:281:20: error: lvalue required as unary ‘&’ operand
>    __read_once_size(&(x), __u.__c, sizeof(x));  \
>                     ^
> include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
>   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>                                                  ^
> include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
>  #define READ_ONCE(x) __READ_ONCE(x, 1)
>                       ^
> include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
>   typeof(p) _________p1 = READ_ONCE(p); \
>                           ^
> include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
>   container_of(lockless_dereference(ptr), type, member)
>                ^
> fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
>   struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
>                              ^
> include/linux/compiler.h:283:28: error: lvalue required as unary ‘&’ operand
>    __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
>                             ^
> include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
>   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>                                                  ^
> include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
>  #define READ_ONCE(x) __READ_ONCE(x, 1)
>                       ^
> include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
>   typeof(p) _________p1 = READ_ONCE(p); \
>                           ^
> include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
>   container_of(lockless_dereference(ptr), type, member)
>                ^
> fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
>   struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
>                              ^
> scripts/Makefile.build:258: recipe for target 'fs/fs-writeback.o' failed
> make[1]: *** [fs/fs-writeback.o] Error 1
> Makefile:1526: recipe for target 'fs/fs-writeback.o' failed
> make: *** [fs/fs-writeback.o] Error 2
> 
> It's this new usage in fs/fs-writeback.c:
> 
> static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
>                                   struct wb_writeback_work *base_work,
>                                   bool skip_if_busy)
> {
>         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,

After all, RCU read-side list primitives need to fetch pointers in order
to traverse those pointers in an RCU-safe manner.  The patch below clears
this up for me, does it also work for you?

							Thanx, Paul

------------------------------------------------------------------------

commit 9221c4e78cc3511cd15b1433617ae2548ad8f631
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Oct 26 07:47:20 2015 -0700

    fs: Fix argument to list_entry_rcu()
    
    The list_entry_rcu() macro requires that its first argument be the
    pointer to the list_head contained in the structure whose pointer is to
    be returned, but fetched in an RCU-safe manner.  This commit therefore
    fixes the use in bdi_split_work_to_wbs() to follow this convention.
    
    Reported-by: Ingo Molnar <mingo@kernel.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 29e4599f6fc1..126d4de8faad 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -779,7 +779,7 @@ 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 *wb = list_entry_rcu(bdi->wb_list.next,
 						struct bdi_writeback, bdi_node);
 
 	might_sleep();

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

[toc] | [prev] | [next] | [standalone]

#1256197

FromIngo Molnar <mingo@kernel.org>
Date2015-10-26 19:10 +0100
Message-ID<qnUnw-35z-7@gated-at.bofh.it>
In reply to#1256088
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> > It's this new usage in fs/fs-writeback.c:
> > 
> > static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
> >                                   struct wb_writeback_work *base_work,
> >                                   bool skip_if_busy)
> > {
> >         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,
> 
> After all, RCU read-side list primitives need to fetch pointers in order to 
> traverse those pointers in an RCU-safe manner.  The patch below clears this up 
> for me, does it also work for you?

Are you sure about that?

I considered this solution too, but the code goes like this:

static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
                                  struct wb_writeback_work *base_work,
                                  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);

        might_sleep();
restart:
        rcu_read_lock();
        list_for_each_entry_continue_rcu(wb, &bdi->wb_list, bdi_node) {

and list_for_each_entry_continue_rcu() will start the iteration with the next 
entry. So if you initialize the head with .next, then we'll start with 
.next->next, i.e. we skip the first entry.

That seems to change behavior and break the logic.

Another solution I considered is to use bd->wb_list.next->prev, but that, beyond 
being ugly, causes actual extra runtime overhead - for something that seems 
academical.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1256445 — Re: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-10-27 04:40 +0100
SubjectRe: [PATCH tip/core/rcu 11/13] rculist: Make list_entry_rcu() use lockless_dereference()
Message-ID<qo3h7-7h-3@gated-at.bofh.it>
In reply to#1256088
On Mon, Oct 26, 2015 at 11:55 PM, Paul E. McKenney
<paulmck@linux.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256463

FromTejun Heo <tj@kernel.org>
Date2015-10-27 06:20 +0100
Message-ID<qo4PT-18r-9@gated-at.bofh.it>
In reply to#1256445
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:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256470

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2015-10-27 06:40 +0100
Message-ID<qo59g-1gU-21@gated-at.bofh.it>
In reply to#1256463
On Tue, Oct 27, 2015 at 02:19:39PM +0900, Tejun Heo wrote:
> 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.

OK, that sounds much better!

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

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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

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

[toc] | [prev] | [next] | [standalone]

#1257794

FromIngo Molnar <mingo@kernel.org>
Date2015-10-28 09:40 +0100
Message-ID<qouqZ-dw-13@gated-at.bofh.it>
In reply to#1256463
* Tejun Heo <tj@kernel.org> wrote:

> 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();

Any objections against me applying this fix to tip:core/rcu so that I can push the 
recent RCU changes towards linux-next without triggering a build failure?

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1258403

FromPatrick Marlier <patrick.marlier@gmail.com>
Date2015-10-28 21:40 +0100
Message-ID<qoFFM-7u4-3@gated-at.bofh.it>
In reply to#1257794

On 10/28/2015 09:33 AM, Ingo Molnar wrote:
>
> * Tejun Heo <tj@kernel.org> wrote:
>
>> 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();
>
> Any objections against me applying this fix to tip:core/rcu so that I can push the
> recent RCU changes towards linux-next without triggering a build failure?

No objection on my side but probably you are waiting for an ack from 
somebody else.
--
Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258492

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2015-10-29 01:10 +0100
Message-ID<qoIWZ-1dO-1@gated-at.bofh.it>
In reply to#1258403
On Wed, Oct 28, 2015 at 09:35:42PM +0100, Patrick Marlier wrote:
> 
> 
> On 10/28/2015 09:33 AM, Ingo Molnar wrote:
> >
> >* Tejun Heo <tj@kernel.org> wrote:
> >
> >>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();
> >
> >Any objections against me applying this fix to tip:core/rcu so that I can push the
> >recent RCU changes towards linux-next without triggering a build failure?
> 
> No objection on my side but probably you are waiting for an ack from
> somebody else.

I am guessing that he was asking Tejun, but just for the record, I am
OK with it as well:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

							Thanx, Paul

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

[toc] | [prev] | [next] | [standalone]

#1258533

FromTejun Heo <tj@kernel.org>
Date2015-10-29 03:20 +0100
Message-ID<qoKYO-2ww-1@gated-at.bofh.it>
In reply to#1258492
On Wed, Oct 28, 2015 at 05:00:30PM -0700, Paul E. McKenney wrote:
> > >Any objections against me applying this fix to tip:core/rcu so that I can push the
> > >recent RCU changes towards linux-next without triggering a build failure?
> > 
> > No objection on my side but probably you are waiting for an ack from
> > somebody else.
> 
> I am guessing that he was asking Tejun, but just for the record, I am
> OK with it as well:
> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Oops, sorry, for some reason I thought Ingo was asking someone else.
The patch already got applied but yes routing it through -tip sounds
good.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1258417 — [tip:core/rcu] fs/writeback, rcu: Don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

Fromtip-bot for Tejun Heo <tipbot@zytor.com>
Date2015-10-28 22:00 +0100
Subject[tip:core/rcu] fs/writeback, rcu: Don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()
Message-ID<qoFZ7-7Bh-11@gated-at.bofh.it>
In reply to#1256463
Commit-ID:  b33e18f61bd18227a456016a77b1a968f5bc1d65
Gitweb:     http://git.kernel.org/tip/b33e18f61bd18227a456016a77b1a968f5bc1d65
Author:     Tejun Heo <tj@kernel.org>
AuthorDate: Tue, 27 Oct 2015 14:19:39 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 28 Oct 2015 13:17:30 +0100

fs/writeback, rcu: 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.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Darren Hart <dvhart@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dipankar Sarma <dipankar@in.ibm.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Patrick Marlier <patrick.marlier@gmail.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: pranith kumar <bobby.prani@gmail.com>
Link: http://lkml.kernel.org/r/20151027051939.GA19355@mtj.duckdns.org
Signed-off-by: Ingo Molnar <mingo@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:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256471

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2015-10-27 06:40 +0100
Message-ID<qo59g-1gU-23@gated-at.bofh.it>
In reply to#1256445
On Tue, Oct 27, 2015 at 12:37:16PM +0900, Linus Torvalds wrote:
> On Mon, Oct 26, 2015 at 11:55 PM, Paul E. McKenney
> <paulmck@linux.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.

Good point, color me dazed and confused.  :-/

							Thanx, Paul

> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web