From: ebiederm@xmission.com (Eric W. Biederman)
To: Chen Gang <gang.chen@asianux.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
xi.wang@gmail.com, nicolas.dichtel@6wind.com,
Andrew Morton <akpm@linux-foundation.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kernel/sysctl_binary.c: improve the usage of return value 'result'
Date: Tue, 06 Aug 2013 14:46:03 -0700 [thread overview]
Message-ID: <8761vibihw.fsf@xmission.com> (raw)
In-Reply-To: <5200A5E6.9020803@asianux.com> (Chen Gang's message of "Tue, 06 Aug 2013 15:29:42 +0800")
Chen Gang <gang.chen@asianux.com> writes:
Have you tested this code? Do you have anything that actually the
uses sysctl binary interface?
If you do have code that actually uses this interface please fix it not
to use it. This code is fundamentally a stop gap measure and will
bit-rot in time and then we will remove it.
> Improve the usage of return value 'result', so not only can make code
> clearer to readers, but also can improve the performance.
>
> Assign the return error code only when error occurs, so can save some
> instructions (some of them are inside looping).
That actually is a code pessimization, not an optimization. As are most
of your proposed changes. Only assigning the error code simply can not
generate better assembly code.
> Rewrite the sysctl_getname() to make code clearer to readers, and also
> can save some instructions (it is mainly related with the usage of the
> variable 'result').
Again you are introducing branches and pessimizing the code in the name
of optimization.
> Remove useless variable 'result' for sysctl() and compat sysctl(), when
> do_sysctl() fails, it is meaningless to assign 'oldlenp ' with original
> same value.
> Also modify the related code to pass "./scripts/checkpatch.pl" checking.
I really don't care about checpatch.pl at this point.
The right answer to the code is to config it out and then you don't have
to worry about it one way or another.
The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.
Eric
next prev parent reply other threads:[~2013-08-06 21:46 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-06 7:29 [PATCH] kernel/sysctl_binary.c: improve the usage of return value 'result' Chen Gang
2013-08-06 21:43 ` Andrew Morton
2013-08-06 22:11 ` Joe Perches
2013-08-07 3:53 ` Chen Gang
2013-08-06 22:13 ` Eric W. Biederman
2013-08-07 5:28 ` Chen Gang F T
2013-08-07 5:11 ` Chen Gang
2013-08-06 21:46 ` Eric W. Biederman [this message]
2013-08-07 5:07 ` Chen Gang
2013-08-07 5:56 ` Li Zefan
2013-08-07 6:10 ` Joe Perches
2013-08-07 6:29 ` Chen Gang
2013-08-07 6:42 ` Li Zefan
2013-08-07 6:42 ` Li Zefan
2013-08-07 6:57 ` Chen Gang
2013-08-07 6:24 ` Chen Gang
2013-08-07 6:29 ` Andrew Morton
2013-08-07 6:34 ` Chen Gang
2013-08-07 7:02 ` Li Zefan
2013-08-07 8:03 ` Chen Gang
2013-08-07 8:44 ` Li Zefan
2013-08-07 9:13 ` Chen Gang
2013-08-07 7:45 ` Eric W. Biederman
2013-08-07 10:25 ` Chen Gang
2013-08-07 18:38 ` Eric W. Biederman
2013-08-08 3:19 ` Chen Gang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8761vibihw.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=akpm@linux-foundation.org \
--cc=gang.chen@asianux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.dichtel@6wind.com \
--cc=viro@zeniv.linux.org.uk \
--cc=xi.wang@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox