[PATCH] split xfs_ioc_xattr

[PATCH] split xfs_ioc_xattr

[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 <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-03-04 18:14:57.000000000 +0100
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c	2008-03-04 18:25:51.000000000 +0100
@@ -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;
-		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 (likely(!error))
-			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;
-		}
-
-		attr_flags = 0;
-		if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
-			attr_flags |= ATTR_NONBLOCK;
-
-		vattr->va_mask = XFS_AT_XFLAGS;
-		vattr->va_xflags = xfs_merge_ioc_xflags(flags,
-							xfs_ip2xflags(ip));
-
-		error = xfs_setattr(ip, vattr, attr_flags, NULL);
-		if (likely(!error))
-			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;
+
+	if (copy_from_user(&flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \
+		      FS_NOATIME_FL | FS_NODUMP_FL | \
+		      FS_SYNC_FL))
+		return -EOPNOTSUPP;
+
+	vattr = kmalloc(sizeof(*vattr), GFP_KERNEL);
+	if (unlikely(!vattr))
+		return -ENOMEM;
+
+	attr_flags = 0;
+	if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
+		attr_flags |= ATTR_NONBLOCK;
+
+	vattr->va_mask = XFS_AT_XFLAGS;
+	vattr->va_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
+
+	error = -xfs_setattr(ip, vattr, attr_flags, NULL);
+	if (likely(!error))
+		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;

Re: [PATCH] split xfs_ioc_xattr

[Niv Sardi]

Christoph Hellwig <hch@lst.de> writes:
> 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 <hch@lst.de>

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

Cheers,

> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-03-04 18:14:57.000000000 +0100
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c	2008-03-04 18:25:51.000000000 +0100
> @@ -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;
> -		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 (likely(!error))

*HERE* 

> -			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;
> -		}
> -
> -		attr_flags = 0;
> -		if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
> -			attr_flags |= ATTR_NONBLOCK;
> -
> -		vattr->va_mask = XFS_AT_XFLAGS;
> -		vattr->va_xflags = xfs_merge_ioc_xflags(flags,
> -							xfs_ip2xflags(ip));
> -
> -		error = xfs_setattr(ip, vattr, attr_flags, NULL);
> -		if (likely(!error))

*HERE* 

> -			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;
> +
> +	if (copy_from_user(&flags, arg, sizeof(flags)))
> +		return -EFAULT;
> +
> +	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \
> +		      FS_NOATIME_FL | FS_NODUMP_FL | \
> +		      FS_SYNC_FL))
> +		return -EOPNOTSUPP;
> +
> +	vattr = kmalloc(sizeof(*vattr), GFP_KERNEL);
> +	if (unlikely(!vattr))

*HERE* 

> +		return -ENOMEM;
> +
> +	attr_flags = 0;
> +	if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
> +		attr_flags |= ATTR_NONBLOCK;
> +
> +	vattr->va_mask = XFS_AT_XFLAGS;
> +	vattr->va_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip));
> +
> +	error = -xfs_setattr(ip, vattr, attr_flags, NULL);
> +	if (likely(!error))

*HERE*

> +		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;

-- 
Niv Sardi

Re: [PATCH] split xfs_ioc_xattr

[Christoph Hellwig]

On Mon, Apr 14, 2008 at 01:14:47PM +1000, Niv Sardi wrote:
> 
> 
> Christoph Hellwig <hch@lst.de> writes:
> > 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 <hch@lst.de>
> 
> Looks good to me, aren't the likely() unlinkely() deprecated ? shouldn't
> they be killed ?

Why would they be deprecated?

Re: [PATCH] split xfs_ioc_xattr

[Niv Sardi]

Christoph Hellwig <hch@lst.de> writes:

> On Mon, Apr 14, 2008 at 01:14:47PM +1000, Niv Sardi wrote:
>> 
>> 
>> Christoph Hellwig <hch@lst.de> writes:
>> > 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 <hch@lst.de>
>> 
>> 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. »

But it looks like a matter of taste…

I'll push it in.
-- 
Niv Sardi

Re: [PATCH] split xfs_ioc_xattr

[David Chinner]

On Wed, Apr 16, 2008 at 01:47:13PM +1000, Niv Sardi wrote:
> Christoph Hellwig <hch@lst.de> writes:
> 
> > On Mon, Apr 14, 2008 at 01:14:47PM +1000, Niv Sardi wrote:
> >> 
> >> 
> >> Christoph Hellwig <hch@lst.de> writes:
> >> > 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 <hch@lst.de>
> >> 
> >> 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.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] split xfs_ioc_xattr

[Timothy Shimmin]

David Chinner wrote:
> On Wed, Apr 16, 2008 at 01:47:13PM +1000, Niv Sardi wrote:
>> Christoph Hellwig <hch@lst.de> writes:
>>
>>> On Mon, Apr 14, 2008 at 01:14:47PM +1000, Niv Sardi wrote:
>>>>
>>>> Christoph Hellwig <hch@lst.de> writes:
>>>>> 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 <hch@lst.de>
>>>> 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?

--Tim

Re: [PATCH] split xfs_ioc_xattr

[Andi Kleen]

Timothy Shimmin <tes@sgi.com> writes:
>
> 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
 

likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[Timothy Shimmin]

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

Andi Kleen wrote:
> Timothy Shimmin <tes@sgi.com> writes:
>> 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
>  

Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[Andi Kleen]

Timothy Shimmin wrote:
> 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 :)

Ideal would be to not bother by default, but occasionally
run oprofile with icache and branch misprediction profiling for macro
benchmarks (significant user space code running) and if you see
any icache miss/mispredict hot spots in your code  add the annotations
there and then double check with the unlikely profiler.

-Andi

Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[Eric Sandeen]

Timothy Shimmin wrote:
> 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?

-Eric

Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[David Chinner]

On Fri, Apr 18, 2008 at 09:05:25AM -0500, Eric Sandeen wrote:
> 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. i.e. a "never" block gets moved out of line,
while a "frequent" block is left inline. Realistically, the way
this is used in the Irix dir2 code is completely useless - code like
this:

        if (namelen >= MAXNAMELEN) {
#pragma mips_frequency_hint NEVER
                return XFS_ERROR(EINVAL);
        }
        if (rval = xfs_dir_ino_validate(tp->t_mountp, inum)) {
#pragma mips_frequency_hint NEVER
                return rval;
        }

Never needed those hints in the first place, it's damn messy, and
the places where it might actually be useful it doesn't get used.
I'd prefer to avoid the hints unless we really have a performance
critical path that we want to have as much straight line code as
possible. That will require profiling to get right....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[Andi Kleen]

On Mon, Apr 21, 2008 at 10:33:43AM +1000, David Chinner wrote:
> On Fri, Apr 18, 2008 at 09:05:25AM -0500, Eric Sandeen wrote:
> > 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,
> 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 ...

-Andi

Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

[David Chinner]

On Mon, Apr 21, 2008 at 09:55:58AM +0200, Andi Kleen wrote:
> On Mon, Apr 21, 2008 at 10:33:43AM +1000, David Chinner wrote:
> > On Fri, Apr 18, 2008 at 09:05:25AM -0500, Eric Sandeen wrote:
> > > 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,
> > 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 ...

Different heuristics. IIRC the "true" branch of the if was typically
put inline so you could implicitly code your logic to get the
compiler to do the right thing....

And with the branch delay slot, things like:

	if (error)
		return error;

translate into three instructions:

	cmp	error, r0
	jne	return_prelude
	mov	v0, error	<<< BDS

Seeing as the BDS is only executed when the jump occurs and v0 holds
the function return value, there's no out of line code to bother
hinting about....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group