Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
On Wed, Sep 26, 2007 at 09:40:20AM -0400, Erez Zadok wrote: >... > Also, Auke, if indeed compilers are [sic] likely to do better than > programmers adding un/likely wrappers, then why do we still support that in > the kernel? (Working for a company tat produces high-quality compilers, you > may know the answer better.) > > Personally I'm not too fond of what those wrappers do the code: they make it > a bit harder to read the code (yet another extra set of parentheses); and > they cause the code to be indented further to the right, which you sometimes > have to split to multiple lines to avoid going over 80 chars. There are some places in generic code where it makes sense, e.g.: #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0) If you run into a BUG() it's anyway game over. And there are some rare hotpaths in the kernel where it might make sense, and many other places where the likely/unlikely usage that might be present doesn't make sense. Unless you know you need it you simply shouldn't use likely/unlikely. > Cheers, > Erez. cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
In message <[EMAIL PROTECTED]>, Jan Engelhardt writes: > > On Sep 26 2007 11:43, Erez Zadok wrote: > > > >*That's* the information I was looking for, Kyle: what's the estimated > >probability I should be using as my guideline. I used 95% (20/1 ratio), and > > ;-) > > 19:1 <=> 95:5 <=> 95% <=> ratio=0.95 != 20.0 (=20/1) > > >you're telling me I should use 99% (100/1 ratio). The difference between > > 99:1 <=> 99% <=> ratio=0.99 != 100.0 (=100/1) > > >the number of cycles saved/added is very compelling. Given that I certainly > >agree with you that I'm using un/likely too much. I'll re-evaluate and > >update my patch series then. Yeah, close enough. :-) The important issue is that I'm probably using about five times too many un/likely wrappers. Erez. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
On Sep 26 2007 11:43, Erez Zadok wrote: > >*That's* the information I was looking for, Kyle: what's the estimated >probability I should be using as my guideline. I used 95% (20/1 ratio), and ;-) 19:1 <=> 95:5 <=> 95% <=> ratio=0.95 != 20.0 (=20/1) >you're telling me I should use 99% (100/1 ratio). The difference between 99:1 <=> 99% <=> ratio=0.99 != 100.0 (=100/1) >the number of cycles saved/added is very compelling. Given that I certainly >agree with you that I'm using un/likely too much. I'll re-evaluate and >update my patch series then. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
In message <[EMAIL PROTECTED]>, Kyle Moffett writes: > On Sep 26, 2007, at 09:40:20, Erez Zadok wrote: [...] > > Recently we've done a full audit of the entire code, and added un/ > > likely where we felt that the chance of succeeding is 95% or better > > (e.g., error conditions that should rarely happen, and such). > > Actually due to the performance penalty on some systems I think you > only want to use it if the chance of succeeding is 99% or better, as > the benefit if predicted is a cycle or two and the harm if > mispredicted can be more than 50 cycles, depending on the CPU. *That's* the information I was looking for, Kyle: what's the estimated probability I should be using as my guideline. I used 95% (20/1 ratio), and you're telling me I should use 99% (100/1 ratio). The difference between the number of cycles saved/added is very compelling. Given that I certainly agree with you that I'm using un/likely too much. I'll re-evaluate and update my patch series then. Thanks, Erez. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
On Sep 26, 2007, at 09:40:20, Erez Zadok wrote: In message <[EMAIL PROTECTED]>, "Kok, Auke" writes: I've been told several times that adding these is almost always bogus - either it messes up the CPU branch prediction or the compiler/CPU just does a lot better at finding the right way without these hints. Adding them as a blanket seems rather strange. Have you got any numbers that this really improves performance? Auke, that's a good question, but I found it hard to find any info about it. There's no discussion on it in Documentation/, and very little I could find elsewhere. I did see one url explaining what un/likely does precisely, but no guidelines. My understanding is that it can improve performance, as long as it's used carefully (otherwise it may hurt performance). Hmm, even still I agree with Auke, you probably use it too much. Recently we've done a full audit of the entire code, and added un/ likely where we felt that the chance of succeeding is 95% or better (e.g., error conditions that should rarely happen, and such). Actually due to the performance penalty on some systems I think you only want to use it if the chance of succeeding is 99% or better, as the benefit if predicted is a cycle or two and the harm if mispredicted can be more than 50 cycles, depending on the CPU. You should also remember than in filesystems many "failures" are triggered by things like the ld.so library searches, where it literally calls access() 20 different times on various possible paths for library files, failing the first 19. It does this once for each necessary library. Typically you only want to add unlikely() or likely() for about 2 reasons: (A) It's a hot path and the unlikely case is just going to burn a bunch of CPU anyways (B) It really is extremely unlikely that it fails (Think physical hardware failure) Anything else is just bogus. Cheers, Kyle Moffett - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
In message <[EMAIL PROTECTED]>, "Kok, Auke" writes: > Erez Zadok wrote: > > Signed-off-by: Erez Zadok <[EMAIL PROTECTED]> > > --- > > fs/unionfs/copyup.c | 102 > > +- > > 1 files changed, 51 insertions(+), 51 deletions(-) > > > > diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c > > index 23ac4c8..e3c5f15 100644 > > --- a/fs/unionfs/copyup.c > > +++ b/fs/unionfs/copyup.c > > @@ -36,14 +36,14 @@ static int copyup_xattrs(struct dentry > > *old_lower_dentry, > > > > /* query the actual size of the xattr list */ > > list_size = vfs_listxattr(old_lower_dentry, NULL, 0); > > - if (list_size <= 0) { > > + if (unlikely(list_size <= 0)) { > > > I've been told several times that adding these is almost always bogus - > either it > messes up the CPU branch prediction or the compiler/CPU just does a lot > better at > finding the right way without these hints. > > Adding them as a blanket seems rather strange. Have you got any numbers that > this > really improves performance? > > Auke Auke, that's a good question, but I found it hard to find any info about it. There's no discussion on it in Documentation/, and very little I could find elsewhere. I did see one url explaining what un/likely does precisely, but no guidelines. My understanding is that it can improve performance, as long as it's used carefully (otherwise it may hurt performance). Background: we used un/likely in Unionfs only sparingly until now. We looked at what other filesystems and kernel components do, and it seems that it varies a lot: some subsystems use it religiously wherever they can, and some use it just a little here and there. We looked at what other filesystems do in particular and tried to follow a similar model under what cases other filesystems use un/likely. Recently we've done a full audit of the entire code, and added un/likely where we felt that the chance of succeeding is 95% or better (e.g., error conditions that should rarely happen, and such). So while it looks like we've added many of those in this series of patches, I can assure you we didn't just wrap every conditional in an un/likely just for the sake of using it. :-) There are plenty of conditionals (over 250) left untouched b/c it didn't make sense to force the branch prediction on them one way or another. So my questions are, for everyone, what's the policy on using un/likely? Any common conventions/wisdom? I think we need something added to Documentation/. Also, Auke, if indeed compilers are [sic] likely to do better than programmers adding un/likely wrappers, then why do we still support that in the kernel? (Working for a company tat produces high-quality compilers, you may know the answer better.) Personally I'm not too fond of what those wrappers do the code: they make it a bit harder to read the code (yet another extra set of parentheses); and they cause the code to be indented further to the right, which you sometimes have to split to multiple lines to avoid going over 80 chars. Cheers, Erez. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
Erez Zadok wrote: > Signed-off-by: Erez Zadok <[EMAIL PROTECTED]> > --- > fs/unionfs/copyup.c | 102 +- > 1 files changed, 51 insertions(+), 51 deletions(-) > > diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c > index 23ac4c8..e3c5f15 100644 > --- a/fs/unionfs/copyup.c > +++ b/fs/unionfs/copyup.c > @@ -36,14 +36,14 @@ static int copyup_xattrs(struct dentry *old_lower_dentry, > > /* query the actual size of the xattr list */ > list_size = vfs_listxattr(old_lower_dentry, NULL, 0); > - if (list_size <= 0) { > + if (unlikely(list_size <= 0)) { I've been told several times that adding these is almost always bogus - either it messes up the CPU branch prediction or the compiler/CPU just does a lot better at finding the right way without these hints. Adding them as a blanket seems rather strange. Have you got any numbers that this really improves performance? Auke - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 10/25] Unionfs: add un/likely conditionals on copyup ops
Signed-off-by: Erez Zadok <[EMAIL PROTECTED]> --- fs/unionfs/copyup.c | 102 +- 1 files changed, 51 insertions(+), 51 deletions(-) diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c index 23ac4c8..e3c5f15 100644 --- a/fs/unionfs/copyup.c +++ b/fs/unionfs/copyup.c @@ -36,14 +36,14 @@ static int copyup_xattrs(struct dentry *old_lower_dentry, /* query the actual size of the xattr list */ list_size = vfs_listxattr(old_lower_dentry, NULL, 0); - if (list_size <= 0) { + if (unlikely(list_size <= 0)) { err = list_size; goto out; } /* allocate space for the actual list */ name_list = unionfs_xattr_alloc(list_size + 1, XATTR_LIST_MAX); - if (!name_list || IS_ERR(name_list)) { + if (unlikely(!name_list || IS_ERR(name_list))) { err = PTR_ERR(name_list); goto out; } @@ -52,14 +52,14 @@ static int copyup_xattrs(struct dentry *old_lower_dentry, /* now get the actual xattr list of the source file */ list_size = vfs_listxattr(old_lower_dentry, name_list, list_size); - if (list_size <= 0) { + if (unlikely(list_size <= 0)) { err = list_size; goto out; } /* allocate space to hold each xattr's value */ attr_value = unionfs_xattr_alloc(XATTR_SIZE_MAX, XATTR_SIZE_MAX); - if (!attr_value || IS_ERR(attr_value)) { + if (unlikely(!attr_value || IS_ERR(attr_value))) { err = PTR_ERR(name_list); goto out; } @@ -73,11 +73,11 @@ static int copyup_xattrs(struct dentry *old_lower_dentry, size = vfs_getxattr(old_lower_dentry, name_list, attr_value, XATTR_SIZE_MAX); mutex_unlock(&old_lower_dentry->d_inode->i_mutex); - if (size < 0) { + if (unlikely(size < 0)) { err = size; goto out; } - if (size > XATTR_SIZE_MAX) { + if (unlikely(size > XATTR_SIZE_MAX)) { err = -E2BIG; goto out; } @@ -91,13 +91,13 @@ static int copyup_xattrs(struct dentry *old_lower_dentry, * temporarily get FOWNER privileges. * XXX: move entire copyup code to SIOQ. */ - if (err == -EPERM && !capable(CAP_FOWNER)) { + if (unlikely(err == -EPERM && !capable(CAP_FOWNER))) { cap_raise(current->cap_effective, CAP_FOWNER); err = vfs_setxattr(new_lower_dentry, name_list, attr_value, size, 0); cap_lower(current->cap_effective, CAP_FOWNER); } - if (err < 0) + if (unlikely(err < 0)) goto out; name_list += strlen(name_list) + 1; } @@ -105,7 +105,7 @@ out: unionfs_xattr_kfree(name_list_buf); unionfs_xattr_kfree(attr_value); /* Ignore if xattr isn't supported */ - if (err == -ENOTSUPP || err == -EOPNOTSUPP) + if (unlikely(err == -ENOTSUPP || err == -EOPNOTSUPP)) err = 0; return err; } @@ -136,15 +136,15 @@ static int copyup_permissions(struct super_block *sb, ATTR_ATIME_SET | ATTR_MTIME_SET | ATTR_FORCE | ATTR_GID | ATTR_UID; err = notify_change(new_lower_dentry, &newattrs); - if (err) + if (unlikely(err)) goto out; /* now try to change the mode and ignore EOPNOTSUPP on symlinks */ newattrs.ia_mode = i->i_mode; newattrs.ia_valid = ATTR_MODE | ATTR_FORCE; err = notify_change(new_lower_dentry, &newattrs); - if (err == -EOPNOTSUPP && - S_ISLNK(new_lower_dentry->d_inode->i_mode)) { + if (unlikely(err == -EOPNOTSUPP && +S_ISLNK(new_lower_dentry->d_inode->i_mode))) { printk(KERN_WARNING "unionfs: changing \"%s\" symlink mode unsupported\n", new_lower_dentry->d_name.name); @@ -178,7 +178,7 @@ static int __copyup_ndentry(struct dentry *old_lower_dentry, run_sioq(__unionfs_mkdir, &args); err = args.err; - } else if (S_ISLNK(old_mode)) { + } else if (unlikely(S_ISLNK(old_mode))) { args.symlink.parent = new_lower_parent_dentry->d_inode; args.symlink.dentry = new_lower_dentry; args.symlink.symbuf = symbuf; @@ -186,8 +186,8 @@ static int __copyup_ndentry(struct dentry *old_lower_dentry, run_sioq(__unionfs_symlink, &args); err = args.err; - } else if (S_ISBLK(old_mode) || S_ISCHR(old_mode) || - S_ISFIFO(old_mode) || S_ISSOCK(old_mode))