linux-kernel - Re: [PATCH] kernel/sysctl_binary.c: improve the usage of return value 'result'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20130806144339.182beb0a2abddc0782015487@linux-foundation.org>
Date:	Tue, 6 Aug 2013 14:43:39 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Chen Gang <gang.chen@...anux.com>
Cc:	Al Viro <viro@...iv.linux.org.uk>,
	"Eric W. Biederman" <ebiederm@...ssion.com>, xi.wang@...il.com,
	nicolas.dichtel@...nd.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel/sysctl_binary.c: improve the usage of return
 value 'result'

On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <gang.chen@...anux.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 :(

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/