Linux-Next Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: linux-next@vger.kernel.org, linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Dave Chinner <dchinner@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: linux-next: manual merge of the akpm tree with Linus' tree
Date: Tue, 10 Sep 2013 15:27:53 -0700
Message-ID: <20130910152753.662599171456233c5f91edb4@linux-foundation.org> (raw)
In-Reply-To: <20130910143807.4c32d548e08d2184061f52cb@canb.auug.org.au>

On Tue, 10 Sep 2013 14:38:07 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Andrew,
> 
> Today's linux-next merge of the akpm tree got a conflict in fs/dcache.c
> between commit 8aab6a27332b ("vfs: reorganize dput() memory accesses")
> from Linus' tree and commit "dcache: convert to use new lru list
> infrastructure" from the akpm tree.
> 
> /me mutters about development happening during the merge window -
> especially when Andrew is absent.
> 
> I have no idea if this will be correct, but I just used the version from
> the akpm tree (effectively reverting parts of commit 8aab6a27332b) and
> can carry the fix as necessary (no action is required).

This is rather a fiasco.  "vfs: reorganize dput() memory accesses" made
rather a mess of a 46 patch series which has been under development and
test for two cycles so far.

I reverted it so I could get it all to apply and build with some
confidence that I didn't break anything.  Then I went to re-apply "vfs:
reorganize dput() memory accesses" on top but I'm unsure that it's the
right thing to do.  ->d_lru is now reused for s_dentry_lru and for the
shrink list and for the dispose list, so simply testing its
list_emptiness doesn't work any more.  And it's unobvious that the
problem which "vfs: reorganize dput() memory accesses" addresses still
exists, or whether it was worsened or whatever.

And given that "vfs: reorganize dput() memory accesses" was driven by
careful profiling work, there's not a lot of point in going any further
until the new code is profiled and comparisons are performed.

Am moderately frustrated by all of this.  It would be nice if the vfs
maintainer could have, you know, paid some attention to a massive great
vfs patchset instead of blithely wrecking it :(


Dave, can you please eyeball the below and have a think about its
applicability under the new regime?  Thanks.

Right now I'm not very confident in all of this.  I think what I'll do
is send the patches out for re-re-re-review right now and I'll ask Al
and Linus to please find some time to think them over.




commit 8aab6a27332bbf2abfcb35224738394e784d940b
Author:     Linus Torvalds <torvalds@linux-foundation.org>
AuthorDate: Sun Sep 8 13:26:18 2013 -0700
Commit:     Linus Torvalds <torvalds@linux-foundation.org>
CommitDate: Sun Sep 8 13:26:18 2013 -0700

    vfs: reorganize dput() memory accesses
    
    This is me being a bit OCD after all the dentry optimization work this
    merge window: profiles end up showing 'dput()' as a rather expensive
    operation, and there were two unrelated bad reasons for that.
    
    The first reason was reading d_lockref.count for debugging purposes,
    which touches the lockref cacheline (for reads) before really need to.
    More importantly, the debugging test in question is _wrong_, and has
    hidden bugs.  It's true that we can only sleep when the count goes down
    to zero, but the test as-is hides the much more subtle bug that happens
    if we race with somebody else deleting the file.
    
    Anyway we _will_ touch that cacheline, but let's do it for a write and
    in the right routine (ie in "lockref_put_or_lock()") which annotates the
    costs better.  So remove the misleading debug code.
    
    The other was an unnecessary access to the cacheline that contains the
    d_lru list, just to check whether we already were on the LRU list or
    not.  This is exactly what we have d_flags for, so that we can avoid
    touching extra cache lines for the common case.  So just add another bit
    for "is this dentry on the LRU".
    
    Finally, mark the tests properly likely/unlikely, so that the common
    fast-paths are dense in the instruction stream.
    
    This makes the profiles look much saner.
    
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/dcache.c b/fs/dcache.c
index 761e31b..bf3c4f9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -308,8 +308,9 @@ static void dentry_unlink_inode(struct dentry * dentry)
  */
 static void dentry_lru_add(struct dentry *dentry)
 {
-	if (list_empty(&dentry->d_lru)) {
+	if (unlikely(!(dentry->d_flags & DCACHE_LRU_LIST))) {
 		spin_lock(&dcache_lru_lock);
+		dentry->d_flags |= DCACHE_LRU_LIST;
 		list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
 		dentry->d_sb->s_nr_dentry_unused++;
 		dentry_stat.nr_unused++;
@@ -320,7 +321,7 @@ static void dentry_lru_add(struct dentry *dentry)
 static void __dentry_lru_del(struct dentry *dentry)
 {
 	list_del_init(&dentry->d_lru);
-	dentry->d_flags &= ~DCACHE_SHRINK_LIST;
+	dentry->d_flags &= ~(DCACHE_SHRINK_LIST | DCACHE_LRU_LIST);
 	dentry->d_sb->s_nr_dentry_unused--;
 	dentry_stat.nr_unused--;
 }
@@ -341,6 +342,7 @@ static void dentry_lru_move_list(struct dentry *dentry, struct list_head *list)
 {
 	spin_lock(&dcache_lru_lock);
 	if (list_empty(&dentry->d_lru)) {
+		dentry->d_flags |= DCACHE_LRU_LIST;
 		list_add_tail(&dentry->d_lru, list);
 		dentry->d_sb->s_nr_dentry_unused++;
 		dentry_stat.nr_unused++;
@@ -509,24 +511,22 @@ relock:
  */
 void dput(struct dentry *dentry)
 {
-	if (!dentry)
+	if (unlikely(!dentry))
 		return;
 
 repeat:
-	if (dentry->d_lockref.count == 1)
-		might_sleep();
 	if (lockref_put_or_lock(&dentry->d_lockref))
 		return;
 
-	if (dentry->d_flags & DCACHE_OP_DELETE) {
+	/* Unreachable? Get rid of it */
+	if (unlikely(d_unhashed(dentry)))
+		goto kill_it;
+
+	if (unlikely(dentry->d_flags & DCACHE_OP_DELETE)) {
 		if (dentry->d_op->d_delete(dentry))
 			goto kill_it;
 	}
 
-	/* Unreachable? Get rid of it */
- 	if (d_unhashed(dentry))
-		goto kill_it;
-
 	dentry->d_flags |= DCACHE_REFERENCED;
 	dentry_lru_add(dentry);
 
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index fe50f3d..feaa8d8 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -208,6 +208,7 @@ struct dentry_operations {
 #define DCACHE_MANAGED_DENTRY \
 	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
 
+#define DCACHE_LRU_LIST		0x80000
 #define DCACHE_DENTRY_KILLED	0x100000
 
 extern seqlock_t rename_lock;

  reply index

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-10  4:38 Stephen Rothwell
2013-09-10 22:27 ` Andrew Morton [this message]
2013-09-10 22:29   ` Al Viro
2013-09-10 22:35     ` Andrew Morton
2013-09-10 22:36       ` Al Viro
2013-09-10 22:39         ` Al Viro
2013-09-10 22:41         ` Andrew Morton
2013-09-10 22:48           ` Al Viro
2013-09-10 22:59             ` Al Viro
2013-09-10 23:13               ` Andrew Morton
2013-09-10 23:55                 ` Al Viro
2013-09-11  4:30                 ` Stephen Rothwell
2013-09-10 23:37               ` Linus Torvalds
2013-09-10 23:53                 ` Al Viro
2013-09-11  0:01                   ` Linus Torvalds
2013-09-11  0:39                     ` Dave Chinner
2013-09-13  0:56                 ` Linus Torvalds
2013-09-13  1:12                   ` Linus Torvalds
2013-09-13  1:35                     ` Al Viro
2013-09-13 19:12                     ` Linus Torvalds
2013-09-13 19:28                       ` Linus Torvalds
2013-09-13 19:54                       ` Linus Torvalds
2013-09-13 20:00                       ` Al Viro
2013-09-13 20:18                         ` Al Viro
2013-09-13 20:23                           ` Al Viro
2013-09-13 20:25                         ` Linus Torvalds
2013-09-13 20:31                           ` Linus Torvalds
2013-09-13 20:31                           ` Al Viro
2013-09-13 20:34                             ` Linus Torvalds
2013-09-10 22:35   ` Linus Torvalds
2013-09-10 22:44     ` Andrew Morton
2013-09-11  0:30       ` Stephen Rothwell
2013-09-11  0:41         ` Linus Torvalds
  -- strict thread matches above, loose matches on Subject: below --
2019-12-02  2:17 Stephen Rothwell
2019-12-02  2:08 Stephen Rothwell
2018-10-15  7:22 Stephen Rothwell
2018-10-15  7:04 Stephen Rothwell
2018-03-26  8:56 Stephen Rothwell
2017-01-09  2:51 Stephen Rothwell
2016-12-12  5:49 Stephen Rothwell
2016-01-19  2:42 Stephen Rothwell
2016-01-19  2:36 Stephen Rothwell
2015-07-27  5:26 Stephen Rothwell
2013-09-10  4:21 Stephen Rothwell
2013-09-10  4:09 Stephen Rothwell
2013-09-10  4:12 ` Stephen Rothwell
2013-09-09  5:38 Stephen Rothwell
2013-09-09  5:22 Stephen Rothwell
2013-09-09  5:16 Stephen Rothwell
2013-08-30  8:44 Stephen Rothwell
2013-05-27  6:20 Stephen Rothwell
2013-05-20  4:04 Stephen Rothwell
2013-05-20 12:19 ` Chris Mason
2013-05-13  5:20 Eric Paris
2013-05-13  2:07 Stephen Rothwell
2013-05-13  2:11 ` Eric Paris
2013-05-13  4:16   ` Stephen Rothwell
2013-05-13  4:49   ` Kees Cook
2013-05-13  5:14     ` Eric Paris
2013-05-02  6:01 Stephen Rothwell
2013-04-29  8:38 Stephen Rothwell
2013-04-19  7:40 Stephen Rothwell
2013-04-16  7:25 Stephen Rothwell
2013-04-03  6:10 Stephen Rothwell
2013-03-25  4:22 Stephen Rothwell
2013-03-04  2:21 Stephen Rothwell
2013-01-04  3:27 Stephen Rothwell
2012-12-11  5:25 Stephen Rothwell
2012-12-11  5:22 Stephen Rothwell
2012-12-11  7:58 ` Glauber Costa
2012-12-07  6:39 Stephen Rothwell
2012-11-30  6:24 Stephen Rothwell
2012-11-26 12:52 Stephen Rothwell
2012-11-26 12:48 Stephen Rothwell
2012-11-26 13:25 ` Xiaotian Feng
2012-11-26 12:34 Stephen Rothwell
2012-10-15  2:07 Stephen Rothwell
2012-10-15 22:14 ` Catalin Marinas
2012-10-01 14:15 Stephen Rothwell
2012-08-22  5:59 Stephen Rothwell
2012-08-22  8:58 ` Mel Gorman
2012-07-27  3:57 Stephen Rothwell
2012-07-02  6:39 Stephen Rothwell
2012-06-04  4:58 Stephen Rothwell
2012-05-31  4:24 Stephen Rothwell
2012-05-31  4:13 Stephen Rothwell
2012-05-31  7:25 ` Johannes Weiner
2012-05-31  8:24   ` Stephen Rothwell
2012-05-31  8:27     ` Stephen Rothwell
2012-05-21  8:23 Stephen Rothwell
2012-05-21  8:13 Stephen Rothwell
2012-05-21  8:16 ` Cyrill Gorcunov
2012-03-08  6:53 Stephen Rothwell
2012-03-08  7:32 ` Andrew Morton
2012-03-08  7:41   ` Stephen Rothwell
2012-03-08  7:50     ` Andrew Morton
2012-03-08  7:50       ` Xiao Guangrong
2012-03-08  9:59       ` Xiao Guangrong
2012-03-08 21:24         ` Andrew Morton
2012-03-08 23:42           ` Linus Torvalds
2011-12-28  7:54 Stephen Rothwell
2011-11-08  3:24 Stephen Rothwell
2011-11-01  8:16 Stephen Rothwell
2011-11-01 10:47 ` Tao Ma
2011-09-16  6:09 Stephen Rothwell
2011-08-15  4:52 Stephen Rothwell

Reply instructions:

You may reply publically 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=20130910152753.662599171456233c5f91edb4@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=dchinner@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=sfr@canb.auug.org.au \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /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

Linux-Next Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-next/0 linux-next/git/0.git

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

Example config snippet for mirrors

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


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