* [PATCH] split xfs_ioc_xattr
@ 2008-03-19 20:40 Christoph Hellwig
2008-04-14 3:14 ` Niv Sardi
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2008-03-19 20:40 UTC (permalink / raw
To: xfs
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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-03-19 20:40 [PATCH] split xfs_ioc_xattr Christoph Hellwig
@ 2008-04-14 3:14 ` Niv Sardi
2008-04-14 3:29 ` Christoph Hellwig
0 siblings, 1 reply; 13+ messages in thread
From: Niv Sardi @ 2008-04-14 3:14 UTC (permalink / raw
To: Christoph Hellwig; +Cc: xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-04-14 3:14 ` Niv Sardi
@ 2008-04-14 3:29 ` Christoph Hellwig
2008-04-16 3:47 ` Niv Sardi
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2008-04-14 3:29 UTC (permalink / raw
To: Niv Sardi; +Cc: Christoph Hellwig, xfs
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-04-14 3:29 ` Christoph Hellwig
@ 2008-04-16 3:47 ` Niv Sardi
2008-04-16 6:37 ` David Chinner
0 siblings, 1 reply; 13+ messages in thread
From: Niv Sardi @ 2008-04-16 3:47 UTC (permalink / raw
To: Christoph Hellwig; +Cc: xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-04-16 3:47 ` Niv Sardi
@ 2008-04-16 6:37 ` David Chinner
2008-04-16 7:06 ` Timothy Shimmin
0 siblings, 1 reply; 13+ messages in thread
From: David Chinner @ 2008-04-16 6:37 UTC (permalink / raw
To: Niv Sardi; +Cc: Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-04-16 6:37 ` David Chinner
@ 2008-04-16 7:06 ` Timothy Shimmin
2008-04-16 7:29 ` Andi Kleen
0 siblings, 1 reply; 13+ messages in thread
From: Timothy Shimmin @ 2008-04-16 7:06 UTC (permalink / raw
To: David Chinner; +Cc: Niv Sardi, Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] split xfs_ioc_xattr
2008-04-16 7:06 ` Timothy Shimmin
@ 2008-04-16 7:29 ` Andi Kleen
2008-04-18 7:06 ` likely and unlikely was: " Timothy Shimmin
0 siblings, 1 reply; 13+ messages in thread
From: Andi Kleen @ 2008-04-16 7:29 UTC (permalink / raw
To: Timothy Shimmin; +Cc: David Chinner, Niv Sardi, Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-16 7:29 ` Andi Kleen
@ 2008-04-18 7:06 ` Timothy Shimmin
2008-04-18 13:34 ` Andi Kleen
2008-04-18 14:05 ` Eric Sandeen
0 siblings, 2 replies; 13+ messages in thread
From: Timothy Shimmin @ 2008-04-18 7:06 UTC (permalink / raw
To: Andi Kleen; +Cc: David Chinner, Niv Sardi, Christoph Hellwig, xfs
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-18 7:06 ` likely and unlikely was: " Timothy Shimmin
@ 2008-04-18 13:34 ` Andi Kleen
2008-04-18 14:05 ` Eric Sandeen
1 sibling, 0 replies; 13+ messages in thread
From: Andi Kleen @ 2008-04-18 13:34 UTC (permalink / raw
To: Timothy Shimmin; +Cc: David Chinner, Niv Sardi, Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-18 7:06 ` likely and unlikely was: " Timothy Shimmin
2008-04-18 13:34 ` Andi Kleen
@ 2008-04-18 14:05 ` Eric Sandeen
2008-04-21 0:33 ` David Chinner
1 sibling, 1 reply; 13+ messages in thread
From: Eric Sandeen @ 2008-04-18 14:05 UTC (permalink / raw
To: Timothy Shimmin
Cc: Andi Kleen, David Chinner, Niv Sardi, Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-18 14:05 ` Eric Sandeen
@ 2008-04-21 0:33 ` David Chinner
2008-04-21 7:55 ` Andi Kleen
0 siblings, 1 reply; 13+ messages in thread
From: David Chinner @ 2008-04-21 0:33 UTC (permalink / raw
To: Eric Sandeen
Cc: Timothy Shimmin, Andi Kleen, David Chinner, Niv Sardi,
Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-21 0:33 ` David Chinner
@ 2008-04-21 7:55 ` Andi Kleen
2008-04-21 21:41 ` David Chinner
0 siblings, 1 reply; 13+ messages in thread
From: Andi Kleen @ 2008-04-21 7:55 UTC (permalink / raw
To: David Chinner
Cc: Eric Sandeen, Timothy Shimmin, Andi Kleen, Niv Sardi,
Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
2008-04-21 7:55 ` Andi Kleen
@ 2008-04-21 21:41 ` David Chinner
0 siblings, 0 replies; 13+ messages in thread
From: David Chinner @ 2008-04-21 21:41 UTC (permalink / raw
To: Andi Kleen
Cc: David Chinner, Eric Sandeen, Timothy Shimmin, Niv Sardi,
Christoph Hellwig, xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-04-21 21:41 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-19 20:40 [PATCH] split xfs_ioc_xattr Christoph Hellwig
2008-04-14 3:14 ` Niv Sardi
2008-04-14 3:29 ` Christoph Hellwig
2008-04-16 3:47 ` Niv Sardi
2008-04-16 6:37 ` David Chinner
2008-04-16 7:06 ` Timothy Shimmin
2008-04-16 7:29 ` Andi Kleen
2008-04-18 7:06 ` likely and unlikely was: " Timothy Shimmin
2008-04-18 13:34 ` Andi Kleen
2008-04-18 14:05 ` Eric Sandeen
2008-04-21 0:33 ` David Chinner
2008-04-21 7:55 ` Andi Kleen
2008-04-21 21:41 ` David Chinner
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.