From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH] VFS: Cut down inode->i_op->xyz accesses in path walking Date: Tue, 26 Jul 2011 04:05:31 +0100 Message-ID: <20110726030531.GE22133@ZenIV.linux.org.uk> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Christoph Hellwig , linux-fsdevel To: Linus Torvalds Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:55093 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751364Ab1GZDFe (ORCPT ); Mon, 25 Jul 2011 23:05:34 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Jul 25, 2011 at 07:04:11PM -0700, Linus Torvalds wrote: > > > One of the biggest remaining unnecessary costs in path walking is the > pointer chasing in inode operations. We already avoided the > dentry->d_op derferences with the DCACHE_OP_xyz flags, this just starts > doing the same thing for the i_op->xyz cases and new IOP_xyz flags in > the new inode->i_opflag field. > > To make sure the i_opflag field is consistent with i_op, we introduce a > simple wrapper function to do assignments to i_op. That makes the patch > big, but it was almost entirely mechanical and very straightforward. > > Signed-off-by: Linus Torvalds > --- > > Ok, so this patch is pretty big, but the bulk if it is an entirely > mechanical replacement of > > inode->i_op = xyz; > > with > > set_inode_ops(inode, xyz); > > using a very stupid sed-script. It was followed by some manual editing > (a couple of fixups for my sed-script being stupid, and a couple of > whitespace cleanups for pre-existing whitespace problems that 'git diff' > just highlights). > > This removes the 'inode->i_op->xyz' chain from the critical pathname walk, > although the *single* hottest instruction in link_path_walk on my Westmere > system remains the test of 'inode->i_opflag' for IOP_FOLLOW_LINK: > > 14.70%: testb $0x2,(%rax) > > because it turns out that that instruction is what brings in the inode > into the L1 cache. But hey, it's certainly better than getting that i_op > and then following the pointer there and checking it for NULL. > > And it's better to the tune of roughly 0.1s for my empty "make -j" test > (which is actually dominated by the user-space costs in 'make', but has a > largish pathname component to it too). That's out of about 4.3s, so it is > about 2%. Not huge, but it seems quite measurable. > > Of course, those kinds of costs will depend very much on cache sizes and > microarchitectural details, and the "empty kernel make -j" may not be the > most relevant benchmark around, but it's more or a real load than some. > > Hmm? Too painful? We could actually cheat a bit. *All* inodes that ever reach inode_permission() have at some point been pointed to by ->d_inode of some dentry. It's inconvenient as hell (and inviting abuse) to pass such dentry instead of inode; moreover, there would be nasty questions of ->d_inode stability on RCU path. However, we can greatly reduce that amount of changes if we do the following: * one bit in your new field set when inode is put in ->d_inode. Checked at inode_permission() time, BUG_ON() if not set. * other bit(s) set by checking ->i_op contents at the same time, if bit hadn't been already set (->i_op can't change afterwards); checked by inode_permission() and whatever else might want to (e.g. checks for non-NULL ->lookup(); for those we also know that inode went through some ->d_inode at some point). * we need to play with setting these flags only in two places - __d_instantiate() and d_obtain_alias(). IOW, add /* called with inode->i_lock held */ static inline void set_inode_flags(struct inode *inode) { if (!(inode->i_opflag & Iop_valid)) { struct inode_operations *op = inode->i_op; int flags = Iop_valid; if (op->permission) flags |= Iop_permission; if (op->lookup) flags |= Iop_lookup; inode->i_opflag = flags; } } and turn __d_instantiate() into static void __d_instantiate(struct dentry *dentry, struct inode *inode) { spin_lock(&dentry->d_lock); if (inode) { if (unlikely(IS_AUTOMOUNT(inode))) dentry->d_flags |= DCACHE_NEED_AUTOMOUNT; set_inode_flags(inode); list_add(&dentry->d_alias, &inode->i_dentry); } dentry->d_inode = inode; dentry_rcuwalk_barrier(dentry); spin_unlock(&dentry->d_lock); fsnotify_d_instantiate(dentry, inode); } with set_inode_flags(inode); also added right after tmp->d_inode = inode; in d_obtain_alias() Voila - inode_permission() would do int flags; ... flags = inode->i_opflag; BUG_ON(!(flags & Iop_valid)); if (unlikely(flags & Iop_permission)) res = inode->i_op->permission(...); else res = generic_permission(...) and we are all set. Individual fs code is not affected at all... Sure, it's a bit of cheating, but it avoids a lot of churn *and* a nasty class of bugs in the future - somebody assigning ->i_op directly. Dunno...