split xfs_ioc_xattr

The three subcases of xfs_ioc_xattr don't share any semantics and almost no code, so split it into three separate helpers.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +5 -12

@@ -871,85 +871,85 @@ xfs_ioc_fsgetxattr(
 }
 
 STATIC int
-xfs_ioc_xattr(
+xfs_ioc_fssetxattr(
 	xfs_inode_t		*ip,
 	struct file		*filp,
-	unsigned int		cmd,
 	void			__user *arg)
 {
 	struct fsxattr		fa;
 	struct bhv_vattr	*vattr;
-	int			error = 0;
+	int			error;
 	int			attr_flags;
-	unsigned int		flags;
+
+	if (copy_from_user(&fa, arg, sizeof(fa)))
+		return -EFAULT;
 
 	vattr = kmalloc(sizeof(*vattr), GFP_KERNEL);
 	if (unlikely(!vattr))
 		return -ENOMEM;
 
-	switch (cmd) {
-	case XFS_IOC_FSSETXATTR: {
-		if (copy_from_user(&fa, arg, sizeof(fa))) {
-			error = -EFAULT;
-			break;
-		}
-
-		attr_flags = 0;

fs/xfs/linux-2.6/xfs_ioctl.c +0 -25

-			vn_revalidate(XFS_ITOV(ip));	/* update flags */
-		error = -error;
-		break;
-	}
-
-	case XFS_IOC_GETXFLAGS: {
-		flags = xfs_di2lxflags(ip->i_d.di_flags);
-		if (copy_to_user(arg, &flags, sizeof(flags)))
-			error = -EFAULT;
-		break;
-	}
-
-	case XFS_IOC_SETXFLAGS: {
-		if (copy_from_user(&flags, arg, sizeof(flags))) {
-			error = -EFAULT;
-			break;
-		}
-
-		if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \
-			      FS_NOATIME_FL | FS_NODUMP_FL | \
-			      FS_SYNC_FL)) {
-			error = -EOPNOTSUPP;
-			break;
-		}
-

fs/xfs/linux-2.6/xfs_ioctl.c +38 -8

-			vn_revalidate(XFS_ITOV(ip));	/* update flags */
-		error = -error;
-		break;
-	}
+	attr_flags = 0;
+	if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
+		attr_flags |= ATTR_NONBLOCK;
+
+	vattr->va_mask = XFS_AT_XFLAGS | XFS_AT_EXTSIZE | XFS_AT_PROJID;
+	vattr->va_xflags  = fa.fsx_xflags;
+	vattr->va_extsize = fa.fsx_extsize;
+	vattr->va_projid  = fa.fsx_projid;
+
+	error = -xfs_setattr(ip, vattr, attr_flags, NULL);
+	if (!error)
+		vn_revalidate(XFS_ITOV(ip));	/* update flags */
+	kfree(vattr);
+	return 0;
+}
 
-	default:
-		error = -ENOTTY;
-		break;
-	}
+STATIC int
+xfs_ioc_getxflags(
+	xfs_inode_t		*ip,
+	void			__user *arg)
+{
+	unsigned int		flags;
+
+	flags = xfs_di2lxflags(ip->i_d.di_flags);
+	if (copy_to_user(arg, &flags, sizeof(flags)))
+		return -EFAULT;
+	return 0;
+}
 
+STATIC int
+xfs_ioc_setxflags(
+	xfs_inode_t		*ip,
+	struct file		*filp,
+	void			__user *arg)
+{
+	struct bhv_vattr	*vattr;
+	unsigned int		flags;
+	int			attr_flags;
+	int			error;
+

fs/xfs/linux-2.6/xfs_ioctl.c +5 -2

+		vn_revalidate(XFS_ITOV(ip));	/* update flags */
 	kfree(vattr);
 	return error;
 }

@@ -1090,10 +1090,12 @@ xfs_ioctl(
 		return xfs_ioc_fsgetxattr(ip, 0, arg);
 	case XFS_IOC_FSGETXATTRA:
 		return xfs_ioc_fsgetxattr(ip, 1, arg);
+	case XFS_IOC_FSSETXATTR:
+		return xfs_ioc_fssetxattr(ip, filp, arg);
 	case XFS_IOC_GETXFLAGS:
+		return xfs_ioc_getxflags(ip, arg);
 	case XFS_IOC_SETXFLAGS:
-	case XFS_IOC_FSSETXATTR:
-		return xfs_ioc_xattr(ip, filp, cmd, arg);
+		return xfs_ioc_setxflags(ip, filp, arg);
 
 	case XFS_IOC_FSSETDM: {
 		struct fsdmidata	dmi;

Quoted 4 lines · Christoph Hellwig

The three subcases of xfs_ioc_xattr don't share any semantics and almost no code, so split it into three separate helpers.

Signed-off-by Christoph Hellwig <[email protected]>

Looks good to me, aren't the likely() unlinkely() deprecated ? shouldn't they be killed ?

Quoted 6 lines · Niv Sardi

Quoted 3 lines · Christoph Hellwig

The three subcases of xfs_ioc_xattr don't share any semantics and almost no code, so split it into three separate helpers.

Quoted 4 lines

Quoted 1 line

Signed-off-by Christoph Hellwig <[email protected]>

Looks good to me, aren't the likely() unlinkely() deprecated ? shouldn't they be killed ?

Why would they be deprecated?

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · Niv Sardi

Quoted 4 lines · Christoph Hellwig

The three subcases of xfs_ioc_xattr don't share any semantics and almost no code, so split it into three separate helpers.

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 4 lines

Quoted 2 lines

Looks good to me, aren't the likely() unlinkely() deprecated ? shouldn't they be killed ?

Why would they be deprecated?

just an impression I had from on of Dave's comment to one of my patches:
« Can we kill all the likely() crap out of here? Modern hardware
branch predictors are far better than static prediction hints. »

Quoted 8 lines · Niv Sardi

Quoted 6 lines · Christoph Hellwig

Quoted 5 lines · Niv Sardi

Quoted 3 lines · Christoph Hellwig

The three subcases of xfs_ioc_xattr don't share any semantics and almost no code, so split it into three separate helpers.

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Christoph Hellwig <[email protected]>

Looks good to me, aren't the likely() unlinkely() deprecated ? shouldn't they be killed ?

Why would they be deprecated?

just an impression I had from on of Dave's comment to one of my patches:
« Can we kill all the likely() crap out of here? Modern hardware
branch predictors are far better than static prediction hints. »

And the context which you haven't quoted? A repugnant hunk of code with one broken use of likely() in two unnecessary 'if (likely(!error) ...' branches, and 20 lines of my comment after the above quote demonstrating of how to restructure it so it was neater, faster and didn't need the prediction hints at all.

I'm still wondering if likely() and unlikely() should ever be used or not?

Quoted 1 line · Timothy Shimmin

I'm still wondering if likely() and unlikely() should ever be used or not?

It's more than just branch predictors. unlikely also moves unlikely code out of line and keeps it out of the icache (the obvious drawback is that it makes the asm code much harder to read during debugging though -- that is why it used to be turned off on x86-64)

Then CPUs have two types of branch predictors: dynamic ones with history and static ones. Dynamic branch predictors tend to only work when the code has been recently executed several times and is still cached in their history buffers

Now the nature of the kernel is that it is a library serving much more code running in user space. This leads to often the user space clearing out the history buffers and caches so kernel code has to often deal with running cache cold and also branch predictor cold.

Then there are the static branch predictors in the CPU and unlikely() actually rearranges code to make them predict correctly.

Personally I would say the cache effects (moving code out of line) are more important than the branch prediction because cache misses are more costly than branch misprediction.

That all said it might make sense in some really performance critical code, especially if it's a in a loop and the gcc static branch predictors (gcc has a large range of builtin heuristics that say e.g. (x < 0) or (x == NULL) is unlikely). Most code is probably not performance critical enough to justify the ugliness of the code annotations. And again for many situations the builtin predictors of gcc (and the CPU) do fine without help anyways.

Also if you add them you should at some point run with the unlikely profiler patch from mm just to make sure that your guesses about which paths are likely are actually correct. Humans are unfortunately often wrong on such guesses.

Ideally (but that might ask for too much for normal code writing) you would only add them to code where you have oprofile data for branch mispredictions or icache misses.

-Andi

Hi,

Thanks for the explanation, Andi.
So I guess the upshot is, that it can make a difference but in many cases (where the perf difference isn't an issue) it is probably not worth the ugliness.
And in performance cases, it would be best to test the hypothesis with the unlikely profiler patch
=> it will be _unlikely_ we will bother ;-)
So I don't think I'll be bothering with them then unless an issue comes up :)

Hi,

Thanks for the explanation, Andi.
So I guess the upshot is, that it can make a difference but in many cases (where the perf difference isn't an issue) it is probably not worth the ugliness.
And in performance cases, it would be best to test the hypothesis with the unlikely profiler patch
=> it will be _unlikely_ we will bother ;-)
So I don't think I'll be bothering with them then unless an issue comes up :)

--Tim

ISTR that the dir2 code on Irix had tons of compiler pragmas for likely and unlikely paths, and that it actually was well-profiled and tested. Did that ever get translated into Linux hints?

Quoted 3 lines · Eric Sandeen

ISTR that the dir2 code on Irix had tons of compiler pragmas for likely and unlikely paths, and that it actually was well-profiled and tested. Did that ever get translated into Linux hints?

The Irix code (#pragma mips_frequency_hint [FREQUENT|NEVER|INIT]) only controllered physical placement of code, it never issued branch hints.

while a "frequent" block is left inline. Realistically, the way this is used in the Irix dir2 code is completely useless - code like this:

Quoted 8 lines · David Chinner

Quoted 3 lines · Eric Sandeen

ISTR that the dir2 code on Irix had tons of compiler pragmas for likely and unlikely paths, and that it actually was well-profiled and tested. Did that ever get translated into Linux hints?

The Irix code (#pragma mips_frequency_hint [FREQUENT|NEVER|INIT]) only controllered physical placement of code, it never issued branch hints.

likely/unlikely control placement too and I think it is actually more important for kernel code than branch hints (which x86 doesn't have explictely unlike ia64, they only way to do a branch hint is to reorder the code) due to icache effects.

i.e. a "never" block gets moved out of line,

Quoted 3 lines

while a "frequent" block is left inline. Realistically, the way this is used in the Irix dir2 code is completely useless - code like this:

Indeed because gcc has a heuristic that early returns are considered unlikely:

/* Branch causing function to terminate is probably not taken. */ DEF_PREDICTOR (PRED_TREE_EARLY_RETURN, "early return (on trees)", HITRATE (61),
0)

It's only 61%, but that should be enough.

I'm somewhat surprised that MipsPro didn't have such a default heuristic though ...