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

[permalink] [raw]

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

Chen Gang <[email protected]> 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

2013-08-06 22:11:30

by Joe Perches

[permalink] [raw]

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

On Tue, 2013-08-06 at 14:43 -0700, Andrew Morton wrote:
> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> 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 :(

https://lkml.org/lkml/2008/12/16/383

2013-08-06 22:14:03

[permalink] [raw]

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

Andrew Morton <[email protected]> writes:

> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> 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.

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.

2013-08-07 03:54:21

[permalink] [raw]

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

On 08/07/2013 06:11 AM, Joe Perches wrote:
> On Tue, 2013-08-06 at 14:43 -0700, Andrew Morton wrote:
>> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> 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 :(
>
> https://lkml.org/lkml/2008/12/16/383
>

Thank you for your information.

--
Chen Gang

2013-08-07 05:09:00

[permalink] [raw]

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

On 08/07/2013 05:46 AM, Eric W. Biederman wrote:
> Chen Gang <[email protected]> writes:
>
> Have you tested this code? Do you have anything that actually the
> uses sysctl binary interface?
>

No, I only compile about it, not give a test. It is really better to
give a test, but it seems not quite necessary to must give a test
(since it is a simple change).

> 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.
>

OK, since these code will be removed, and this patch is not for bug
fixing, so at least, this patch is useless.

The next reply below is only for current discussing, it has no effect
with the global result above (it is a useless patch).

>> 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.
>

Excuse me, my English is not quite well, I don't know about the meaning
of 'pessimization' (I can not find it in my English-Chinese dictionary,
in Thunderbird Spell Check, it is not a correct word either).

But it seems it doesn't matter, what you want to say is only "it is not
an optimization", is it correct ?

And after check the assembly code, really it is (it is only for 2
styles which both are commonly used, not for optimization).

>> 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.
>

Hmm... it is useless for optimization.

But in my opinion, at least, it can make code more clearer for C code
readers, although it may make performance a litter lower.

Please see the 2 implementations below, I feel 2nd is clearer than 1st,
how about your feeling ?

static char *sysctl_getname(const int *name, int nlen, const struct bin_table **tablep)
{
char *tmp, *result;

result = ERR_PTR(-ENOMEM);
tmp = __getname();
if (tmp) {
const struct bin_table *table = get_sysctl(name, nlen, tmp);
result = tmp;
*tablep = table;
if (IS_ERR(table)) {
__putname(tmp);
result = ERR_CAST(table);
}
}
return result;
}

static char *sysctl_getname(const int *name, int nlen, const struct bin_table **tablep)
{
char *result;
const struct bin_table *table;

result = __getname();
if (!result)
return ERR_PTR(-ENOMEM);

table = get_sysctl(name, nlen, result);
if (IS_ERR(table)) {
__putname(result);
return ERR_CAST(table);
}
*tablep = table; /* If error occurs, need not set this value */

return result;
}

>> 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.
>

Do you mean: you don't care about "checkpatch.pl", because some of
another reasons which in fact not related with "checkpatch.pl" though ?

> 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.
>

Pardon?

Excuse me, my English is not quite well, I don't quite understand your
meaning, could you please repeat again in details or say more clearly ?

> 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.
>

I guess no one ever invited you to maintain this file (for just as you
said, this file will be removed), so don't worry about it.

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
better not use word 'seems' which is not a suitable word appeared in
the results, proofs or conclusions.

At least, one conclusion is: this patch switches from old-style to
new-style, not for optimization.

But for sysctl_getname() and "checkpatch.pl" of this patch, better to
get more discussion.

Is it OK ?

> Eric
>
>

Thanks.
--
Chen Gang

2013-08-07 05:12:37

[permalink] [raw]

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

Thank you for your reply in details, especially you are very busy.

My original opinion about optimization is incorrect.

Thanks.

On 08/07/2013 05:43 AM, Andrew Morton wrote:
> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> 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.
>
>

--
Chen Gang

2013-08-07 05:29:58

by Chen Gang F T

[permalink] [raw]

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

On 08/07/2013 06:13 AM, Eric W. Biederman wrote:
> Andrew Morton <[email protected]> writes:
>
>> On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

--
Chen Gang

2013-08-07 05:58:19

[permalink] [raw]

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

>> 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.
>>
>
> Pardon?
>
> Excuse me, my English is not quite well, I don't quite understand your
> meaning, could you please repeat again in details or say more clearly ?
>
>
>> 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.
>>
>
> I guess no one ever invited you to maintain this file (for just as you
> said, this file will be removed), so don't worry about it.
>
> Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
> better not use word 'seems' which is not a suitable word appeared in
> the results, proofs or conclusions.
>

To be honest...

You are too bad in english to do kernel development. You don't seem to
know how to communicate in english...

And people easily get frustrated or even pissed off when discussing with
you. That's why tglx descided to put your emails into /dev/null...

> At least, one conclusion is: this patch switches from old-style to
> new-style, not for optimization.
>
> But for sysctl_getname() and "checkpatch.pl" of this patch, better to
> get more discussion.
>
> Is it OK ?

2013-08-07 06:10:52

by Joe Perches

[permalink] [raw]

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

On Wed, 2013-08-07 at 13:56 +0800, Li Zefan wrote:
> >> 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.
[]
> > Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
> > better not use word 'seems' which is not a suitable word appeared in
> > the results, proofs or conclusions.
[]
> You are too bad in english to do kernel development. You don't seem to
> know how to communicate in english...

I don't think that's true.

For instance, pessimization is a pretty
obscure word. Most native English speakers
could probably guess what it meant, but I
couldn't hold any low opinion of non-native
speaker that asked "what does that mean".

The code communicates just fine.

Chen Gang could be a bit more careful when
making significant changes to core kernel
facilities.

(said the guy that broke just printk)

I think the fluent English speakers can be
tolerant reasonably well when the code is
correct and compelling (and correct).

cheers, Joe

2013-08-07 06:25:11

[permalink] [raw]

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

On 08/07/2013 01:56 PM, Li Zefan wrote:
>>> 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.
>>>
>>
>> Pardon?
>>
>> Excuse me, my English is not quite well, I don't quite understand your
>> meaning, could you please repeat again in details or say more clearly ?
>>
>>
>>> 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.
>>>
>>
>> I guess no one ever invited you to maintain this file (for just as you
>> said, this file will be removed), so don't worry about it.
>>
>> Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
>> better not use word 'seems' which is not a suitable word appeared in
>> the results, proofs or conclusions.
>>
>
> To be honest...
>
> You are too bad in english to do kernel development. You don't seem to
> know how to communicate in english...
>

So I should improve my English, and now I am just trying improving.

At least, it is not an excuse to leave upstream kernel development, is
it right ? or do you have additional ideas or suggestions ?

> And people easily get frustrated or even pissed off when discussing with
> you. That's why tglx descided to put your emails into /dev/null...
>

If any member won't discuss with me because of my bad English, and put
my e-mail into "/dev/null", I can understand, and say sorry to them.

So I really need thank the members who still want to discuss with me.

Do you still want to discuss with me ? if not, please put my e-mail
into "/dev/null", I really can understand, and say sorry to you.

Thanks.

>> At least, one conclusion is: this patch switches from old-style to
>> new-style, not for optimization.
>>
>> But for sysctl_getname() and "checkpatch.pl" of this patch, better to
>> get more discussion.
>>
>> Is it OK ?
>
>
>

--
Chen Gang

2013-08-07 06:30:14

[permalink] [raw]

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

On 08/07/2013 02:10 PM, Joe Perches wrote:
> On Wed, 2013-08-07 at 13:56 +0800, Li Zefan wrote:
>>>> 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.
> []
>>> Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
>>> better not use word 'seems' which is not a suitable word appeared in
>>> the results, proofs or conclusions.
> []
>> You are too bad in english to do kernel development. You don't seem to
>> know how to communicate in english...
>
> I don't think that's true.
>
> For instance, pessimization is a pretty
> obscure word. Most native English speakers
> could probably guess what it meant, but I
> couldn't hold any low opinion of non-native
> speaker that asked "what does that mean".
>
> The code communicates just fine.
>
> Chen Gang could be a bit more careful when
> making significant changes to core kernel
> facilities.
>

OK, thanks, I really should be more careful for making patches.

> (said the guy that broke just printk)
>
> I think the fluent English speakers can be
> tolerant reasonably well when the code is
> correct and compelling (and correct).
>
> cheers, Joe
>
>
>

Thank you very much for your understanding.

--
Chen Gang

2013-08-07 06:30:48

by Andrew Morton

[permalink] [raw]

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

On Wed, 07 Aug 2013 14:24:05 +0800 Chen Gang <[email protected]> wrote:

> > To be honest...
> >
> > You are too bad in english to do kernel development. You don't seem to
> > know how to communicate in english...
> >
>
> So I should improve my English, and now I am just trying improving.
>
> At least, it is not an excuse to leave upstream kernel development, is
> it right ? or do you have additional ideas or suggestions ?

I find your English to be adequate. There are worse ;)

Don't be afraid of writing too much text - trust me, I've never seen
a changelog which was too long!

2013-08-07 06:35:46

[permalink] [raw]

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

On 08/07/2013 02:29 PM, Andrew Morton wrote:
> On Wed, 07 Aug 2013 14:24:05 +0800 Chen Gang <[email protected]> wrote:
>
>>> To be honest...
>>>
>>> You are too bad in english to do kernel development. You don't seem to
>>> know how to communicate in english...
>>>
>>
>> So I should improve my English, and now I am just trying improving.
>>
>> At least, it is not an excuse to leave upstream kernel development, is
>> it right ? or do you have additional ideas or suggestions ?
>
> I find your English to be adequate. There are worse ;)
>
> Don't be afraid of writing too much text - trust me, I've never seen
> a changelog which was too long!
>
>

Thank you very much, I should continue trying.

Thanks.
--
Chen Gang

2013-08-07 06:44:39

[permalink] [raw]

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

On 2013/8/7 14:10, Joe Perches wrote:
> On Wed, 2013-08-07 at 13:56 +0800, Li Zefan wrote:
>>>> 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.
> []
>>> Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
>>> better not use word 'seems' which is not a suitable word appeared in
>>> the results, proofs or conclusions.
> []
>> You are too bad in english to do kernel development. You don't seem to
>> know how to communicate in english...
>
> I don't think that's true.
>
> For instance, pessimization is a pretty
> obscure word. Most native English speakers
> could probably guess what it meant, but I
> couldn't hold any low opinion of non-native
> speaker that asked "what does that mean".
>

I know, and that's why I didn't quote that part.

2013-08-07 06:45:08

[permalink] [raw]

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

2013-08-07 06:58:27

[permalink] [raw]

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

On 08/07/2013 02:42 PM, Li Zefan wrote:
> On 2013/8/7 14:10, Joe Perches wrote:
>> On Wed, 2013-08-07 at 13:56 +0800, Li Zefan wrote:
>>>>> 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.
>> []
>>>> Hmm... do you mean you spend 5 minutes to get a conclusion ? if so,
>>>> better not use word 'seems' which is not a suitable word appeared in
>>>> the results, proofs or conclusions.
>> []
>>> You are too bad in english to do kernel development. You don't seem to
>>> know how to communicate in english...
>>
>> I don't think that's true.
>>
>> For instance, pessimization is a pretty
>> obscure word. Most native English speakers
>> could probably guess what it meant, but I
>> couldn't hold any low opinion of non-native
>> speaker that asked "what does that mean".
>>
>
> I know, and that's why I didn't quote that part.
>

I guess, Joe also know about that.

He intends to explain 'pessimization' to me, so encourage me to continue
trying improving my English.

Thanks.
--
Chen Gang

2013-08-07 07:04:27

[permalink] [raw]

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

>> To be honest...
>>
>> You are too bad in english to do kernel development. You don't seem to
>> know how to communicate in english...
>>
>
> So I should improve my English, and now I am just trying improving.
>
> At least, it is not an excuse to leave upstream kernel development, is
> it right ? or do you have additional ideas or suggestions ?
>

Two sugguestions.

The first one is, if you get a reply from a maintainer (especially a top
maintainer), try harder to understand/learn from that reply, but don't
keep asking why and don't keep arguing without much thinking. I think
what's why sometimes people are annoyed in the discussion with you.

The second one is, better focus on a specific subsystem and try to do some
real work. It's not bad to start making patches like this one, but it won't
do you any good in the long term. Don't get addicted in increasing patch
contribution.

2013-08-07 07:45:25

[permalink] [raw]

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

Chen Gang <[email protected]> writes:

> On 08/07/2013 05:46 AM, Eric W. Biederman wrote:
>> Chen Gang <[email protected]> writes:
>>
>> Have you tested this code? Do you have anything that actually the
>> uses sysctl binary interface?
>>
>
> No, I only compile about it, not give a test. It is really better to
> give a test, but it seems not quite necessary to must give a test
> (since it is a simple change).

Many programs have been broken by programmers not caring enough to test
their changes. In this case it is doubly important because if you don't
test this code it is likely no one will run this code for months.

>> 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.
>>
>
> OK, since these code will be removed, and this patch is not for bug
> fixing, so at least, this patch is useless.

The point of all of this is that it really helps if you stop and learn
the context of the code you are working on. It also helps if you care
about the changes you are making.

In this case the history of sysctl(2) is that someone copied sysctl(2)
from bsd. Then someone quickly created /proc/sys/ to reflect the same
data in a friendlier format. sysctl(2) was quickly deprecated and was
effectively never used in practice. With one notable exception an old
use in glibc.

I was very nice a few years ago and instead of just deleting the code I
wrote a wrapper layer so that the binary interface would translate into
read/write operations on /proc/sys. Mostly that was about not letting
the maintenance of sysctl(2) be a drag on the maintenance of /proc/sys/.
People do care about /proc/sys and do test it and fix bugs with 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.
>>
>
> Excuse me, my English is not quite well, I don't know about the meaning
> of 'pessimization' (I can not find it in my English-Chinese dictionary,
> in Thunderbird Spell Check, it is not a correct word either).

Pessimization is the opposite of optimization. Both words are literally
ridiculous as optimization means to make something more optimum, and
pessimization means to make things more pessimal, and optimum and
pessimal refer to the best and the worst possible situtations. But
whoever let a silly dictionary meaning get in the way of a good use of a
word.

> But it seems it doesn't matter, what you want to say is only "it is not
> an optimization", is it correct ?

Correct.

>>> 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.
>>
>
> Hmm... it is useless for optimization.
>
> But in my opinion, at least, it can make code more clearer for C code
> readers, although it may make performance a litter lower.
>
> Please see the 2 implementations below, I feel 2nd is clearer than 1st,
> how about your feeling ?

My feeling is if you don't care about sysctl(2) enough to figure out the
history or compile test it, or even actually use it, it is highly
unlikely you actually care about reading the code.

I care just enough about the code to keep people who would not even
notice if it the code breaks from touching it.

> Do you mean: you don't care about "checkpatch.pl", because some of
> another reasons which in fact not related with "checkpatch.pl" though
> ?

checkpatch.pl is not the definition of good code, or good style but
meerly a tool to help point out problems before a human has to be bother
to look at it. If a style problem is real it can be explained without
reference to an arbitrary tool.

>> 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.
>>
>
> Pardon?
>
> Excuse me, my English is not quite well, I don't quite understand your
> meaning, could you please repeat again in details or say more clearly?

You can enable or disable this code with "make menuconfig". Everyone
should just turn binary sysctl support off and ignore this code. The
someday when the code has bit-rotted because no one actually cares we
can delete this code.

>> 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.
>>
>
> I guess no one ever invited you to maintain this file (for just as you
> said, this file will be removed), so don't worry about it.

We may get around to removing it. There are just enough silly people
who think it is more important to keep open the possibility of keeping
strange old binaries running than to avoid code bugs that the code has
stayed this long. But no one has ever cared enough to in practice to
maintain this code.

I use people sending patches to sysctl_binary.c as an opportunity to
educate people that their program is doing something silly and they
should change their ways. sysctl_binary.c really is effectively a honey
pot for developers and organizations who are not paying attention.

Eric

2013-08-07 08:04:32

[permalink] [raw]

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

On 08/07/2013 03:02 PM, Li Zefan wrote:
>>> To be honest...
>>>
>>> You are too bad in english to do kernel development. You don't seem to
>>> know how to communicate in english...
>>>
>>
>> So I should improve my English, and now I am just trying improving.
>>
>> At least, it is not an excuse to leave upstream kernel development, is
>> it right ? or do you have additional ideas or suggestions ?
>>
>
> Two sugguestions.
>

Firstly, thank you for your suggestions.

> The first one is, if you get a reply from a maintainer (especially a top
> maintainer), try harder to understand/learn from that reply, but don't
> keep asking why and don't keep arguing without much thinking. I think
> what's why sometimes people are annoyed in the discussion with you.
>

In my opinion, "understand/learn" means:

learn the proof which the author supplied;
understand the author's opinion;
know about what the author wants to do now (especially why he intents to send/reply mail to you).

But "understand/learn" does not mean:

familiar about the 'professional' details.
if each related member knows about the 'professional' details, it only need a work flow, not need discussing.

Do you think so too ?

Hmm... for each reply, I think it has 3 requirements:

1. match the original contents which we want to reply.
2. say opinion clearly.
3. provide proof.

I guess your suggestion is for 1st: if we can not understand/learn from
the original contents, of cause, our reply can not match it.

Since discussing is thinking process, and we may get more understanding
during thinking, so it permits to continue reply multiple times (if for
each reply is qualified with the 3 requirements above).

Have you ever seen some of my reply which misunderstand(or not learn
enough) from original contents ?

Maybe you often saw that I continue reply multiple times for a thread,
but I think, each reply matches the 3 requirements above.

> The second one is, better focus on a specific subsystem and try to do some
> real work. It's not bad to start making patches like this one, but it won't
> do you any good in the long term. Don't get addicted in increasing patch
> contribution.

Hmm... I think what you said above is about ways (or method).

For ways, it is meaningless to say which is better or bad, it depends
on one's goal, one's environments, and one's resources.

So, every member need think of it, and find their own suitable ways to
continue providing their contributions to outside with efficiency.

Thanks.
--
Chen Gang

2013-08-07 08:45:39

[permalink] [raw]

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

>> The first one is, if you get a reply from a maintainer (especially a top
>> maintainer), try harder to understand/learn from that reply, but don't
>> keep asking why and don't keep arguing without much thinking. I think
>> what's why sometimes people are annoyed in the discussion with you.
>>
>
> In my opinion, "understand/learn" means:
>
> learn the proof which the author supplied;
> understand the author's opinion;
> know about what the author wants to do now (especially why he intents to send/reply mail to you).
>
> But "understand/learn" does not mean:
>
> familiar about the 'professional' details.
> if each related member knows about the 'professional' details, it only need a work flow, not need discussing.
>
> Do you think so too ?
>
>
> Hmm... for each reply, I think it has 3 requirements:
>
> 1. match the original contents which we want to reply.
> 2. say opinion clearly.
> 3. provide proof.
>
> I guess your suggestion is for 1st: if we can not understand/learn from
> the original contents, of cause, our reply can not match it.
>
> Since discussing is thinking process, and we may get more understanding
> during thinking, so it permits to continue reply multiple times (if for
> each reply is qualified with the 3 requirements above).
>
>
> Have you ever seen some of my reply which misunderstand(or not learn
> enough) from original contents ?
>
> Maybe you often saw that I continue reply multiple times for a thread,
> but I think, each reply matches the 3 requirements above.
>

You fail to see there's a problem in you and how you frustrate people and
waste their time...

For example in this thread:

https://lkml.org/lkml/2013/7/4/405

and this therad:

https://lkml.org/lkml/2013/6/20/228

Please don't argue anymore...

Back to coding and won't reply to this thread...

2013-08-07 09:14:48

[permalink] [raw]

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

On 08/07/2013 04:44 PM, Li Zefan wrote:
>>> The first one is, if you get a reply from a maintainer (especially a top
>>> maintainer), try harder to understand/learn from that reply, but don't
>>> keep asking why and don't keep arguing without much thinking. I think
>>> what's why sometimes people are annoyed in the discussion with you.
>>>
>>
>> In my opinion, "understand/learn" means:
>>
>> learn the proof which the author supplied;
>> understand the author's opinion;
>> know about what the author wants to do now (especially why he intents to send/reply mail to you).
>>
>> But "understand/learn" does not mean:
>>
>> familiar about the 'professional' details.
>> if each related member knows about the 'professional' details, it only need a work flow, not need discussing.
>>
>> Do you think so too ?
>>
>>
>> Hmm... for each reply, I think it has 3 requirements:
>>
>> 1. match the original contents which we want to reply.
>> 2. say opinion clearly.
>> 3. provide proof.
>>
>> I guess your suggestion is for 1st: if we can not understand/learn from
>> the original contents, of cause, our reply can not match it.
>>
>> Since discussing is thinking process, and we may get more understanding
>> during thinking, so it permits to continue reply multiple times (if for
>> each reply is qualified with the 3 requirements above).
>>
>>
>> Have you ever seen some of my reply which misunderstand(or not learn
>> enough) from original contents ?
>>
>> Maybe you often saw that I continue reply multiple times for a thread,
>> but I think, each reply matches the 3 requirements above.
>>
>
> You fail to see there's a problem in you and how you frustrate people and
> waste their time...
>
> For example in this thread:
>
> https://lkml.org/lkml/2013/7/4/405
>

For this one, it is other member reply to me, so I think
https://lkml.org/lkml/2013/7/7/117 is more suitable as the last related
thread of my reply.

Isn't this thread qualified with the 3 requirements ?

1. match the original contents.
2. say opinion clearly.
3. with proof.

> and this therad:
>
> https://lkml.org/lkml/2013/6/20/228
>

For this one, at least now, I still stick to my opinion: "what he said
is not quite precise".

But since the related member wants stopping discussion, so I stop (in
fact, for the related details contents, he is the last one reply, not
me). :-)

> Please don't argue anymore...
>

Discussing does not mean arguing.

Since we are both Chinese, one Chinese sentence is "Jun Zi He Er Bu Tong".

excuse me, my English is not quite well, could you (or other Chinese
members) please help to translate it into English ?

Thanks.

> Back to coding and won't reply to this thread...
>

I can understand, if get none-reply.

Thanks.
--
Chen Gang

2013-08-07 10:26:41

[permalink] [raw]

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

Firstly, sorry for replying late, and also thank you for your detail
patient reply.

On 08/07/2013 03:45 PM, Eric W. Biederman wrote:
> Chen Gang <[email protected]> writes:
>
>> On 08/07/2013 05:46 AM, Eric W. Biederman wrote:
>>> Chen Gang <[email protected]> writes:
>>>
>>> Have you tested this code? Do you have anything that actually the
>>> uses sysctl binary interface?
>>>
>>
>> No, I only compile about it, not give a test. It is really better to
>> give a test, but it seems not quite necessary to must give a test
>> (since it is a simple change).
>
> Many programs have been broken by programmers not caring enough to test
> their changes. In this case it is doubly important because if you don't
> test this code it is likely no one will run this code for months.
>

It is only for code reconstruction, not for modifying real contents
(especially not touch much code).

So in my option, do 3 checking is enough:

when edit the code, try to get selftest (only copy/past/delete, high light selections for checking, auto sentence checking ...).
review the whole diff 2-3 times by oneself, review the whole source code 2-3 times by oneself.
let it pass compiling without error/warnings.

So I think, for an experienced programmer, it is better to give a test
for our case, but not quite necessary to must do it (especially no
related environments).

>>> 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.
>>>
>>
>> OK, since these code will be removed, and this patch is not for bug
>> fixing, so at least, this patch is useless.
>
> The point of all of this is that it really helps if you stop and learn
> the context of the code you are working on. It also helps if you care
> about the changes you are making.
>

For every programmer, reading source code is one of the important ways
for learning, I am learning kernel by reading code, and try to provide
patch to fix issue which found by reading code.

Discussion can let every member to care about the related changes which
they are making, so Discussion is helpful. ;-)

> In this case the history of sysctl(2) is that someone copied sysctl(2)
> from bsd. Then someone quickly created /proc/sys/ to reflect the same
> data in a friendlier format. sysctl(2) was quickly deprecated and was
> effectively never used in practice. With one notable exception an old
> use in glibc.
>
> I was very nice a few years ago and instead of just deleting the code I
> wrote a wrapper layer so that the binary interface would translate into
> read/write operations on /proc/sys. Mostly that was about not letting
> the maintenance of sysctl(2) be a drag on the maintenance of /proc/sys/.
> People do care about /proc/sys and do test it and fix bugs with it.
>

Thank you for your valuable information.

>>>> 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.
>>>
>>
>> Excuse me, my English is not quite well, I don't know about the meaning
>> of 'pessimization' (I can not find it in my English-Chinese dictionary,
>> in Thunderbird Spell Check, it is not a correct word either).
>
> Pessimization is the opposite of optimization. Both words are literally
> ridiculous as optimization means to make something more optimum, and
> pessimization means to make things more pessimal, and optimum and
> pessimal refer to the best and the worst possible situtations. But
> whoever let a silly dictionary meaning get in the way of a good use of a
> word.
>

OK, thank you for your details explanation.

>> But it seems it doesn't matter, what you want to say is only "it is not
>> an optimization", is it correct ?
>
> Correct.
>
>>>> 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.
>>>
>>
>> Hmm... it is useless for optimization.
>>
>> But in my opinion, at least, it can make code more clearer for C code
>> readers, although it may make performance a litter lower.
>>
>> Please see the 2 implementations below, I feel 2nd is clearer than 1st,
>> how about your feeling ?
>
> My feeling is if you don't care about sysctl(2) enough to figure out the
> history or compile test it, or even actually use it, it is highly
> unlikely you actually care about reading the code.
>

Hmm... e.g. for coredump analysers:

they don't care about each sub-systems,
they can not figure out most of their history,
and never compile test most of them,
or even never actually use most of them,
but at least, they have to care about reading the code which may related with various sub-systems
(also need analyze the related binary data).
(also need read some of the related dis-assembly code).
...
especially, some of root cause often exist in the 'almost waste' code.

> I care just enough about the code to keep people who would not even
> notice if it the code breaks from touching it.
>

Excuse me, my English is not quite well, I can not quite understand
what is your meaing. :-(

I assume it just the same meaning with your previous above paragraph,
if my assumption is incorrect, please reply.

>> Do you mean: you don't care about "checkpatch.pl", because some of
>> another reasons which in fact not related with "checkpatch.pl" though
>> ?
>
> checkpatch.pl is not the definition of good code, or good style but
> meerly a tool to help point out problems before a human has to be bother
> to look at it. If a style problem is real it can be explained without
> reference to an arbitrary tool.
>

Thank you for your details explanation, at least, I have the same
opinion with you.

Hmm... please see below, I still suggest to add additional white space
for it, since this file has already followed the related rules.

1183 if ((area > 63)||(node > 1023))

But it is really an individual trivial patch which need be sent to
trivial mail address (not to you).

>>> 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.
>>>
>>
>> Pardon?
>>
>> Excuse me, my English is not quite well, I don't quite understand your
>> meaning, could you please repeat again in details or say more clearly?
>
> You can enable or disable this code with "make menuconfig". Everyone
> should just turn binary sysctl support off and ignore this code. The
> someday when the code has bit-rotted because no one actually cares we
> can delete this code.
>

Thank you for your detail reply.

>>> 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.
>>>
>>
>> I guess no one ever invited you to maintain this file (for just as you
>> said, this file will be removed), so don't worry about it.
>
> We may get around to removing it. There are just enough silly people
> who think it is more important to keep open the possibility of keeping
> strange old binaries running than to avoid code bugs that the code has
> stayed this long. But no one has ever cared enough to in practice to
> maintain this code.
>

OK, thanks.

> I use people sending patches to sysctl_binary.c as an opportunity to
> educate people that their program is doing something silly and they
> should change their ways. sysctl_binary.c really is effectively a honey
> pot for developers and organizations who are not paying attention.
>

Hmm... how about for coredump analyzers, they are 'crazy' but not
'silly' programmers. ;-)

> Eric
>
>

Thanks.
--
Chen Gang

2013-08-07 18:39:14

[permalink] [raw]

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

Chen Gang <[email protected]> writes:

> Firstly, sorry for replying late, and also thank you for your detail
> patient reply.
>
> On 08/07/2013 03:45 PM, Eric W. Biederman wrote:
>> Chen Gang <[email protected]> writes:
>>
>>> On 08/07/2013 05:46 AM, Eric W. Biederman wrote:
>>>> Chen Gang <[email protected]> writes:
>>>>
>>>> Have you tested this code? Do you have anything that actually the
>>>> uses sysctl binary interface?
>>>>
>>>
>>> No, I only compile about it, not give a test. It is really better to
>>> give a test, but it seems not quite necessary to must give a test
>>> (since it is a simple change).
>>
>> Many programs have been broken by programmers not caring enough to test
>> their changes. In this case it is doubly important because if you don't
>> test this code it is likely no one will run this code for months.
>>
>
> It is only for code reconstruction, not for modifying real contents
> (especially not touch much code).
>
> So in my option, do 3 checking is enough:
>
> when edit the code, try to get selftest (only copy/past/delete, high light selections for checking, auto sentence checking ...).
> review the whole diff 2-3 times by oneself, review the whole source code 2-3 times by oneself.
> let it pass compiling without error/warnings.
>
> So I think, for an experienced programmer, it is better to give a test
> for our case, but not quite necessary to must do it (especially no
> related environments).

Sometimes it is not possible to test the code, when dealing with strange
architectures or hardware we don't have, and a best effort must be made.

However in the case of a single system call that is trivially callable
it is just good practice to test the code.

But seriously there is a human factor at play. Our internal algorthims
for doing things as human beings are not exact but best effor hueristics
and sometimes they go wrong. So practical double checks are important.

Being an experienced programmer actually means there is more reason to
stop and test your changes because it is terribly easy to get
overconfident.

And frankly I don't want to see patches from people who in general don't
care enough about what they are changing that they are not motivated to
test their 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.
>>>>
>>>
>>> Hmm... it is useless for optimization.
>>>
>>> But in my opinion, at least, it can make code more clearer for C code
>>> readers, although it may make performance a litter lower.
>>>
>>> Please see the 2 implementations below, I feel 2nd is clearer than 1st,
>>> how about your feeling ?
>>
>> My feeling is if you don't care about sysctl(2) enough to figure out the
>> history or compile test it, or even actually use it, it is highly
>> unlikely you actually care about reading the code.
>>
>
> Hmm... e.g. for coredump analysers:
>
> they don't care about each sub-systems,
> they can not figure out most of their history,
> and never compile test most of them,
> or even never actually use most of them,
> but at least, they have to care about reading the code which may related with various sub-systems
> (also need analyze the related binary data).
> (also need read some of the related dis-assembly code).
> ...
> especially, some of root cause often exist in the 'almost waste' code.

Then I would love to hear about which program was using the sysctl(2)
system call in a core dump because that program needs to be fixed.
Even if it was not the cause of the core dump.

But seriously if you are reading a lot of kernel code don't expect to be
able to convert it to your exact preferred style. Within limits of
Documentation/CodingStyle kernel code is kept in the authors preferred
style.

>> I use people sending patches to sysctl_binary.c as an opportunity to
>> educate people that their program is doing something silly and they
>> should change their ways. sysctl_binary.c really is effectively a honey
>> pot for developers and organizations who are not paying attention.
>>
>
> Hmm... how about for coredump analyzers, they are 'crazy' but not
> 'silly' programmers. ;-)

Well if your coredump lead you into sysctl_binary.c either you have a
broken trace, there is a binary using sysctl(2) that really should be
changed to use /proc/sys, or your following of the trace took you very
far off the mark.

So please find the owner of that binary using sysctl(2) and politely
inform them that it is a bad idea and regardless of whatever else is
going on the binary should be updated to do something better.

Eric

2013-08-08 03:20:38