All of lore.kernel.org
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Jaegeuk Kim <jaegeuk.kim@samsung.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	"Linux Kernel, Mailing List" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [BUG] kmemleak on __radix_tree_preload
Date: Thu, 8 May 2014 17:52:17 +0100	[thread overview]
Message-ID: <20140508165217.GI17344@arm.com> (raw)
In-Reply-To: <20140508155330.GE8754@linux.vnet.ibm.com>

On Thu, May 08, 2014 at 04:53:30PM +0100, Paul E. McKenney wrote:
> On Thu, May 08, 2014 at 04:29:48PM +0100, Catalin Marinas wrote:
> > BTW, is it safe to have a union overlapping node->parent and
> > node->rcu_head.next? I'm still staring at the radix-tree code but a
> > scenario I have in mind is that call_rcu() has been raised for a few
> > nodes, other CPU may have some reference to one of them and set
> > node->parent to NULL (e.g. concurrent calls to radix_tree_shrink()),
> > breaking the RCU linking. I can't confirm this theory yet ;)
> 
> If this were reproducible, I would suggest retrying with non-overlapping
> node->parent and node->rcu_head.next, but you knew that already.  ;-)

Reading the code, I'm less convinced about this scenario (though it's
worth checking without the union).

> But the usual practice would be to make node removal exclude shrinking.
> And the radix-tree code seems to delegate locking to the caller.
> 
> So, is the correct locking present in the page cache?  The radix-tree
> code seems to assume that all update operations for a given tree are
> protected by a lock global to that tree.

The calling code in mm/filemap.c holds mapping->tree_lock when deleting
radix-tree nodes, so no concurrent calls.

> Another diagnosis approach would be to build with
> CONFIG_DEBUG_OBJECTS_RCU_HEAD=y, which would complain about double
> call_rcu() invocations.  Rumor has it that is is necessary to turn off
> other kmem debugging for this to tell you anything -- I have seen cases
> where the kmem debugging obscures the debug-objects diagnostics.

Another test Jaegeuk could run (hopefully he has some time to look into
this).

Thanks for suggestions.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Jaegeuk Kim <jaegeuk.kim@samsung.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	"Linux Kernel, Mailing List" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [BUG] kmemleak on __radix_tree_preload
Date: Thu, 8 May 2014 17:52:17 +0100	[thread overview]
Message-ID: <20140508165217.GI17344@arm.com> (raw)
In-Reply-To: <20140508155330.GE8754@linux.vnet.ibm.com>

On Thu, May 08, 2014 at 04:53:30PM +0100, Paul E. McKenney wrote:
> On Thu, May 08, 2014 at 04:29:48PM +0100, Catalin Marinas wrote:
> > BTW, is it safe to have a union overlapping node->parent and
> > node->rcu_head.next? I'm still staring at the radix-tree code but a
> > scenario I have in mind is that call_rcu() has been raised for a few
> > nodes, other CPU may have some reference to one of them and set
> > node->parent to NULL (e.g. concurrent calls to radix_tree_shrink()),
> > breaking the RCU linking. I can't confirm this theory yet ;)
> 
> If this were reproducible, I would suggest retrying with non-overlapping
> node->parent and node->rcu_head.next, but you knew that already.  ;-)

Reading the code, I'm less convinced about this scenario (though it's
worth checking without the union).

> But the usual practice would be to make node removal exclude shrinking.
> And the radix-tree code seems to delegate locking to the caller.
> 
> So, is the correct locking present in the page cache?  The radix-tree
> code seems to assume that all update operations for a given tree are
> protected by a lock global to that tree.

The calling code in mm/filemap.c holds mapping->tree_lock when deleting
radix-tree nodes, so no concurrent calls.

> Another diagnosis approach would be to build with
> CONFIG_DEBUG_OBJECTS_RCU_HEAD=y, which would complain about double
> call_rcu() invocations.  Rumor has it that is is necessary to turn off
> other kmem debugging for this to tell you anything -- I have seen cases
> where the kmem debugging obscures the debug-objects diagnostics.

Another test Jaegeuk could run (hopefully he has some time to look into
this).

Thanks for suggestions.

-- 
Catalin

  reply	other threads:[~2014-05-08 16:53 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-25  1:45 [BUG] kmemleak on __radix_tree_preload Jaegeuk Kim
2014-04-25  1:45 ` Jaegeuk Kim
2014-05-01 17:06 ` Catalin Marinas
2014-05-01 17:06   ` Catalin Marinas
2014-05-01 18:41   ` Johannes Weiner
2014-05-01 18:41     ` Johannes Weiner
2014-05-07  2:58     ` Jaegeuk Kim
2014-05-07  2:58       ` Jaegeuk Kim
2014-05-07 11:39       ` Catalin Marinas
2014-05-07 11:39         ` Catalin Marinas
2014-05-08  9:16         ` Jaegeuk Kim
2014-05-08  9:16           ` Jaegeuk Kim
2014-05-08  9:26           ` Catalin Marinas
2014-05-08  9:26             ` Catalin Marinas
2014-05-08  9:37             ` Jaegeuk Kim
2014-05-08  9:37               ` Jaegeuk Kim
2014-05-08 10:24               ` Catalin Marinas
2014-05-08 10:24                 ` Catalin Marinas
2014-05-08 15:00                 ` Paul E. McKenney
2014-05-08 15:00                   ` Paul E. McKenney
2014-05-08 15:29                   ` Catalin Marinas
2014-05-08 15:29                     ` Catalin Marinas
2014-05-08 15:53                     ` Paul E. McKenney
2014-05-08 15:53                       ` Paul E. McKenney
2014-05-08 16:52                       ` Catalin Marinas [this message]
2014-05-08 16:52                         ` Catalin Marinas
2014-05-09  0:06                         ` Jaegeuk Kim
2014-05-09  0:06                           ` Jaegeuk Kim
2014-05-08 17:52                       ` Johannes Weiner
2014-05-08 17:52                         ` Johannes Weiner
2014-05-08 21:40                         ` Catalin Marinas
2014-05-08 21:40                           ` Catalin Marinas
2014-05-09  0:01                     ` Jaegeuk Kim
2014-05-09  0:01                       ` Jaegeuk Kim
2014-05-09  9:45                       ` Catalin Marinas
2014-05-09  9:45                         ` Catalin Marinas

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=20140508165217.GI17344@arm.com \
    --to=catalin.marinas@arm.com \
    --cc=hannes@cmpxchg.org \
    --cc=jaegeuk.kim@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=paulmck@linux.vnet.ibm.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.