On 08/07/2013 06:13 AM, Eric W. Biederman wrote: > Andrew Morton <a...@linux-foundation.org> writes: > >> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <gang.c...@asianux.com> wrote: >> >>> Improve the usage of return value 'result', so not only can make code >>> clearer to readers, but also can improve the performance. >> >> It used to be pervasive kernel style do to >> >> ret = -ENOMEM; >> foo = alloc(...); >> if (!foo) >> goto out; >> >> whereas nowadays people usually do the more straightforward >> >> foo = alloc(...); >> if (!foo) { >> ret = -ENOMEM; >> goto out; >> } >> >> The thinking was that the old style generated better code, but for the >> life of me I can't remember why :( > > Because doing the assignment outside of the if() goto . Allows the > compiler to emit the if() goto as a single branch. > > While a smart compiler may perform the code motion across the branch, > it is much easier for the compiler to branch to somewhere else perform > the assignment and then branch out. >
For my opinion, for assembly code, the old style is clearer than the new style. And commonly, the old style will be faster than new style. Thanks. > Eric > > >> Your patch switches from old-style to new-style. And it appears to >> have increased the text size. I did this, to switch three sites back >> to old-style: >> >> --- >> a/kernel/sysctl_binary.c~kernel-sysctl_binaryc-improve-the-usage-of-return-value-result-fix >> +++ a/kernel/sysctl_binary.c >> @@ -941,17 +941,15 @@ static ssize_t bin_string(struct file *f >> copied = result; >> lastp = oldval + copied - 1; >> >> - if (get_user(ch, lastp)) { >> - result = -EFAULT; >> + result = -EFAULT; >> + if (get_user(ch, lastp)) >> goto out; >> - } >> >> /* Trim off the trailing newline */ >> if (ch == '\n') { >> - if (put_user('\0', lastp)) { >> - result = -EFAULT; >> + result = -EFAULT; >> + if (put_user('\0', lastp)) >> goto out; >> - } >> copied -= 1; >> } >> } >> @@ -976,11 +974,10 @@ static ssize_t bin_intvec(struct file *f >> char *buffer; >> ssize_t result; >> >> + result = -ENOMEM; >> buffer = kmalloc(BUFSZ, GFP_KERNEL); >> - if (!buffer) { >> - result = -ENOMEM; >> + if (!buffer) >> goto out; >> - } >> >> if (oldval && oldlen) { >> unsigned __user *vec = oldval; >> _ >> >> and kernel/sysctl_binary.o's .text got six bytes smaller. >> >> Now, smaller text doesn't mean faster code. But it probably means >> larger cache footprint, which can mean slower code. >> >> IOW, it isn't obvious that this was an improvement. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Chen Gang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/