From: Markus Elfring <[email protected]>
Date: Mon, 24 Oct 2016 14:05:14 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
proc: Use seq_putc() in show_cpuinfo()
proc: Replace 28 seq_printf() calls by seq_puts() in show_cpuinfo()
proc: Combine four seq_printf() calls into one call in show_cpuinfo()
arch/mips/kernel/mips-r2-to-r6-emul.c | 7 +--
arch/mips/kernel/proc.c | 95 ++++++++++++++++++++---------------
2 files changed, 59 insertions(+), 43 deletions(-)
--
2.10.1
From: Markus Elfring <[email protected]>
Date: Mon, 24 Oct 2016 09:34:51 +0200
A string which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function "seq_puts"
so that the data output will be a bit more efficient for the headline.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/mips/kernel/mips-r2-to-r6-emul.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/mips/kernel/mips-r2-to-r6-emul.c b/arch/mips/kernel/mips-r2-to-r6-emul.c
index 22dedd6..1bdcb65 100644
--- a/arch/mips/kernel/mips-r2-to-r6-emul.c
+++ b/arch/mips/kernel/mips-r2-to-r6-emul.c
@@ -2232,9 +2232,10 @@ int mipsr2_decoder(struct pt_regs *regs, u32 inst, unsigned long *fcr31)
static int mipsr2_stats_show(struct seq_file *s, void *unused)
{
-
- seq_printf(s, "Instruction\tTotal\tBDslot\n------------------------------\n");
- seq_printf(s, "movs\t\t%ld\t%ld\n",
+ seq_puts(s,
+ "Instruction\tTotal\tBDslot\n------------------------------\n"
+ "movs\t\t");
+ seq_printf(s, "%ld\t%ld\n",
(unsigned long)__this_cpu_read(mipsr2emustats.movs),
(unsigned long)__this_cpu_read(mipsr2bdemustats.movs));
seq_printf(s, "hilo\t\t%ld\t%ld\n",
--
2.10.1
From: Markus Elfring <[email protected]>
Date: Mon, 24 Oct 2016 12:54:15 +0200
A few single characters (line breaks) should be put into a sequence.
Thus use the corresponding function "seq_putc".
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/mips/kernel/proc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index 4eff2ae..765489b 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c
@@ -122,7 +122,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
if (cpu_has_eva) seq_printf(m, "%s", " eva");
if (cpu_has_htw) seq_printf(m, "%s", " htw");
if (cpu_has_xpa) seq_printf(m, "%s", " xpa");
- seq_printf(m, "\n");
+ seq_putc(m, '\n');
if (cpu_has_mmips) {
seq_printf(m, "micromips kernel\t: %s\n",
@@ -152,9 +152,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
raw_notifier_call_chain(&proc_cpuinfo_chain, 0,
&proc_cpuinfo_notifier_args);
-
- seq_printf(m, "\n");
-
+ seq_putc(m, '\n');
return 0;
}
--
2.10.1
From: Markus Elfring <[email protected]>
Date: Mon, 24 Oct 2016 13:45:04 +0200
Strings which did not contain data format specifications should be put
into a sequence.
* Thus use the corresponding function "seq_puts" so that the data output
will be a bit more efficient.
* Omit the format string "%s" which became unnecessary with this refactoring.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/mips/kernel/proc.c | 74 +++++++++++++++++++++++++++++--------------------
1 file changed, 44 insertions(+), 30 deletions(-)
diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index 765489b..07480a9 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c
@@ -79,49 +79,63 @@ static int show_cpuinfo(struct seq_file *m, void *v)
for (i = 0; i < cpu_data[n].watch_reg_count; i++)
seq_printf(m, "%s0x%04x", i ? ", " : "" ,
cpu_data[n].watch_reg_masks[i]);
- seq_printf(m, "]\n");
+ seq_puts(m, "]\n");
}
- seq_printf(m, "isa\t\t\t:");
+ seq_puts(m, "isa\t\t\t:");
if (cpu_has_mips_r1)
- seq_printf(m, " mips1");
+ seq_puts(m, " mips1");
if (cpu_has_mips_2)
- seq_printf(m, "%s", " mips2");
+ seq_puts(m, " mips2");
if (cpu_has_mips_3)
- seq_printf(m, "%s", " mips3");
+ seq_puts(m, " mips3");
if (cpu_has_mips_4)
- seq_printf(m, "%s", " mips4");
+ seq_puts(m, " mips4");
if (cpu_has_mips_5)
- seq_printf(m, "%s", " mips5");
+ seq_puts(m, " mips5");
if (cpu_has_mips32r1)
- seq_printf(m, "%s", " mips32r1");
+ seq_puts(m, " mips32r1");
if (cpu_has_mips32r2)
- seq_printf(m, "%s", " mips32r2");
+ seq_puts(m, " mips32r2");
if (cpu_has_mips32r6)
- seq_printf(m, "%s", " mips32r6");
+ seq_puts(m, " mips32r6");
if (cpu_has_mips64r1)
- seq_printf(m, "%s", " mips64r1");
+ seq_puts(m, " mips64r1");
if (cpu_has_mips64r2)
- seq_printf(m, "%s", " mips64r2");
+ seq_puts(m, " mips64r2");
if (cpu_has_mips64r6)
- seq_printf(m, "%s", " mips64r6");
- seq_printf(m, "\n");
-
- seq_printf(m, "ASEs implemented\t:");
- if (cpu_has_mips16) seq_printf(m, "%s", " mips16");
- if (cpu_has_mdmx) seq_printf(m, "%s", " mdmx");
- if (cpu_has_mips3d) seq_printf(m, "%s", " mips3d");
- if (cpu_has_smartmips) seq_printf(m, "%s", " smartmips");
- if (cpu_has_dsp) seq_printf(m, "%s", " dsp");
- if (cpu_has_dsp2) seq_printf(m, "%s", " dsp2");
- if (cpu_has_dsp3) seq_printf(m, "%s", " dsp3");
- if (cpu_has_mipsmt) seq_printf(m, "%s", " mt");
- if (cpu_has_mmips) seq_printf(m, "%s", " micromips");
- if (cpu_has_vz) seq_printf(m, "%s", " vz");
- if (cpu_has_msa) seq_printf(m, "%s", " msa");
- if (cpu_has_eva) seq_printf(m, "%s", " eva");
- if (cpu_has_htw) seq_printf(m, "%s", " htw");
- if (cpu_has_xpa) seq_printf(m, "%s", " xpa");
+ seq_puts(m, " mips64r6");
+ seq_puts(m,
+ "\n"
+ "ASEs implemented\t:");
+ if (cpu_has_mips16)
+ seq_puts(m, " mips16");
+ if (cpu_has_mdmx)
+ seq_puts(m, " mdmx");
+ if (cpu_has_mips3d)
+ seq_puts(m, " mips3d");
+ if (cpu_has_smartmips)
+ seq_puts(m, " smartmips");
+ if (cpu_has_dsp)
+ seq_puts(m, " dsp");
+ if (cpu_has_dsp2)
+ seq_puts(m, " dsp2");
+ if (cpu_has_dsp3)
+ seq_puts(m, " dsp3");
+ if (cpu_has_mipsmt)
+ seq_puts(m, " mt");
+ if (cpu_has_mmips)
+ seq_puts(m, " micromips");
+ if (cpu_has_vz)
+ seq_puts(m, " vz");
+ if (cpu_has_msa)
+ seq_puts(m, " msa");
+ if (cpu_has_eva)
+ seq_puts(m, " eva");
+ if (cpu_has_htw)
+ seq_puts(m, " htw");
+ if (cpu_has_xpa)
+ seq_puts(m, " xpa");
seq_putc(m, '\n');
if (cpu_has_mmips) {
--
2.10.1
From: Markus Elfring <[email protected]>
Date: Mon, 24 Oct 2016 13:54:58 +0200
Some data were printed into a sequence by four separate function calls.
Print the same data by one function call instead.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/mips/kernel/proc.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index 07480a9..1047a03 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c
@@ -142,12 +142,15 @@ static int show_cpuinfo(struct seq_file *m, void *v)
seq_printf(m, "micromips kernel\t: %s\n",
(read_c0_config3() & MIPS_CONF3_ISA_OE) ? "yes" : "no");
}
- seq_printf(m, "shadow register sets\t: %d\n",
- cpu_data[n].srsets);
- seq_printf(m, "kscratch registers\t: %d\n",
- hweight8(cpu_data[n].kscratch_mask));
- seq_printf(m, "package\t\t\t: %d\n", cpu_data[n].package);
- seq_printf(m, "core\t\t\t: %d\n", cpu_data[n].core);
+ seq_printf(m,
+ "shadow register sets\t: %d\n"
+ "kscratch registers\t: %d\n"
+ "package\t\t\t: %d\n"
+ "core\t\t\t: %d\n",
+ cpu_data[n].srsets,
+ hweight8(cpu_data[n].kscratch_mask),
+ cpu_data[n].package,
+ cpu_data[n].core);
#if defined(CONFIG_MIPS_MT_SMP) || defined(CONFIG_CPU_MIPSR6)
if (cpu_has_mipsmt)
--
2.10.1
On Mon, Oct 24, 2016 at 02:27:55PM +0200, SF Markus Elfring wrote:
>
> A string which did not contain a data format specification should be put
> into a sequence.
This is not a correct description of what you are doing. A better
description would be to say:
"Use seq_put[sc]() instead of seq_printf() since the string does not
contain a data format specifier". You should fix this in all the
patches. Please also note this is really pointless patch, since
reading from /proc isn't done in a tight loop, and even if it were,
the use of vsprintf is the tiniest part of the overhead. It otherwise
reduces the text space or the number of lines of code....
- Ted
>> A string which did not contain a data format specification should be put
>> into a sequence.
>
> This is not a correct description of what you are doing. A better
> description would be to say:
>
> "Use seq_put[sc]() instead of seq_printf() since the string does not
> contain a data format specifier".
Thanks for your suggestion about an other wording.
> You should fix this in all the patches.
I am curious if a second approach will become acceptable in the near future.
> Please also note this is really pointless patch,
If you do not like the proposed changes for some subsystems so far,
I would appreciate another clarification:
* Could you tolerate them for any other software components?
* May I continue to inform involved developers about similar change possibilities?
> since reading from /proc isn't done in a tight loop, and even if it were,
> the use of vsprintf is the tiniest part of the overhead.
Thanks for your software development opinion.
> It otherwise reduces the text space or the number of lines of code....
Do other system testers and Linux users care a bit more for corresponding
chances in improved software efficiency?
Regards,
Markus
On Mon, Oct 24, 2016 at 04:02:49PM +0200, SF Markus Elfring wrote:
> > You should fix this in all the patches.
> I am curious if a second approach will become acceptable in the near
> future.
I don't know what you were asking. I was merely point out that the
> wording was factually incorrect in all of the patches, and I didn't
> feel like replying five times to point out the same mistake.
> > since reading from /proc isn't done in a tight loop, and even if it were,
> > the use of vsprintf is the tiniest part of the overhead.
>
> Thanks for your software development opinion.
It's a lot more than just an opinion. I challenge you to demonstrate
how much savings it would take. Try learning how to use another tool
--- say, perf. Measure how many clock cycles it takes to read from a
proc file that uses seq_printf(). Then measure how many clock cycles
it takes to read from a proc file that uses seq_puts(). Try doing the
experiment 3-5 times each way, to see if the difference is within
measurement error, and then figure out what percentage of the total
CPU time you have saved.
If this sort of thing appeals to you, you might want to consider a
more productive line of work. For example, you could do scalability
measurements. Run various benchmarks with lockdep enabled, and
measure the average and max hold time on various locks. Now see if
you can reduce the max hold time on those locks. You may find that
you can improve performance for real work loads by orders of magnitude
more than you can by sending the sorts of patches you've sent up until
now.
You'd also development more marketable kernel skills, if that has been
your goal by spamming the list with hundreds and thousands of mostly
pointless patches. Note that if a hiring manager were to talk to
developers and get their opinion of the sorts of patches you have been
sending, trust me, it would _not_ be positive. So trying to send more
useful patches might be more helpful if your goal is to try to get
gainful employment.
Cheers,
- Ted
>> I am curious if a second approach will become acceptable in the near future.
>
> I don't know what you were asking.
I am trying to clarify the suggested software evolution again.
> I was merely point out that the wording was factually incorrect
> in all of the patches,
Thanks for this information.
> and I didn't feel like replying five times to point out the same mistake.
This is fine.
>>> since reading from /proc isn't done in a tight loop, and even if it were,
>>> the use of vsprintf is the tiniest part of the overhead.
>>
>> Thanks for your software development opinion.
>
> It's a lot more than just an opinion. I challenge you to demonstrate
> how much savings it would take. Try learning how to use another tool
> --- say, perf. Measure how many clock cycles it takes to read from a
> proc file that uses seq_printf(). Then measure how many clock cycles
> it takes to read from a proc file that uses seq_puts(). Try doing the
> experiment 3-5 times each way, to see if the difference is within
> measurement error, and then figure out what percentage of the total
> CPU time you have saved.
Are there any more software developers interested in such system
performance analyses?
> If this sort of thing appeals to you, you might want to consider a
> more productive line of work. For example, you could do scalability
> measurements. Run various benchmarks with lockdep enabled, and
> measure the average and max hold time on various locks. Now see if
> you can reduce the max hold time on those locks. You may find that
> you can improve performance for real work loads by orders of magnitude
> more than you can by sending the sorts of patches you've sent up until now.
Thanks for your hints around other software development areas.
> You'd also development more marketable kernel skills, if that has been
> your goal by spamming the list with hundreds and thousands of mostly
> pointless patches.
You might categorise my update suggestions with a low value so far.
> Note that if a hiring manager were to talk to developers and get
> their opinion of the sorts of patches you have been sending, trust me,
> it would _not_ be positive.
There are also some constraints around change resistance involved,
aren't there?
* Do my suggestions show small improvements for Linux source files?
* If you find some of them so awful, why should I attempt to improve
any commit messages in another patch series then?
> So trying to send more useful patches might be more helpful
> if your goal is to try to get gainful employment.
Financial incentives would be also nice as you seem to indicate here.
Regards,
Markus
On Mon, Oct 24, 2016 at 04:53:32PM +0200, SF Markus Elfring wrote:
> >>> since reading from /proc isn't done in a tight loop, and even if it were,
> >>> the use of vsprintf is the tiniest part of the overhead.
> >>
> >> Thanks for your software development opinion.
> >
> > It's a lot more than just an opinion. I challenge you to demonstrate
> > how much savings it would take. Try learning how to use another tool
> > --- say, perf. Measure how many clock cycles it takes to read from a
> > proc file that uses seq_printf(). Then measure how many clock cycles
> > it takes to read from a proc file that uses seq_puts(). Try doing the
> > experiment 3-5 times each way, to see if the difference is within
> > measurement error, and then figure out what percentage of the total
> > CPU time you have saved.
>
> Are there any more software developers interested in such system
> performance analyses?
Sure, of course. This is why serious professionals take a huge amount
of time doing benchmarking, and making sure that the benchmarks are
reproducible, and accurately measure something that reflects real
world usage. This can often take as much time or more time than the
actual optimization in some cases.
> There are also some constraints around change resistance involved,
> aren't there?
To quote from the great Don Knuth:
Programmers waste enormous amounts of time thinking about, or
worrying about, the speed of noncritical parts of their programs,
and these attempts at efficiency actually have a strong negative
impact when debugging and maintenance are considered. We should
forget about small efficiencies, say about 97% of the time:
premature optimization is the root of all evil. Yet we should not
pass up our opportunities in that critical 3%.
An experienced developer would be able to very easily spot that trying
to optimize seq_printf() versus seq_puts() is barely going to be
measurable. It's the sort of thing that a developer might fix while
making other, more useful changes to a source file. But there are
costs associated with processing a patch, in terms of developer time
and attention, and those are externalities. It's for the same reason
that whitespace only or comment-only patches are controversial. There
is always the suspicion that there are people doing this because they
hope to win some patch counting game, and that they don't care about
the costs they might be introducing to the rest of the system. And
when the patches cause bugs, then it goes from outright marginal value
to negative value.
> > So trying to send more useful patches might be more helpful
> > if your goal is to try to get gainful employment.
>
> Financial incentives would be also nice as you seem to indicate here.
Well, please note that having a reputation of someone who insists on
sending mostly junk patches (and like junk food, they may have some
nutritive value; but that doesn't change the effect that the net
benefit to person consuming them is marginal or negative), tends to
give you a bad reputation, and may in fact be a hinderance towards
your being able to attain "financial incentives".
If that is in fact your goal, I would gently suggest that you spend
more time improving your skills, and learning more about higher-value
ways you could contribute to the kernel, instead of spamming the
kernel list with lots of low value patches. In the future if you are
adding higher value improvements, and you want to do various cleanups,
such as fixing up seq_printf -> seq_puts changes, sure. But to the
extent that dirties the history so it's harder to find who introduced
a problem using tools such as "git blame", low-value cleanup patches
do have some costs, so it's not enough to say, "but it improves the
CPU time used by 0.000001%!"
Cheers,
- Ted
> An experienced developer would be able to very easily spot that trying
> to optimize seq_printf() versus seq_puts() is barely going to be measurable.
Would you like to offer any incentives to use a more appropriate function
from this Linux programming interface for sequences?
> It's the sort of thing that a developer might fix while
> making other, more useful changes to a source file.
I get doubts when you expect that change possibilities with a higher
priority should and will almost always picked up before update candidates
with a lower impact.
> Well, please note that having a reputation of someone who insists on
> sending mostly junk patches (and like junk food, they may have some
> nutritive value; but that doesn't change the effect that the net
> benefit to person consuming them is marginal or negative), tends to
> give you a bad reputation, and may in fact be a hinderance towards
> your being able to attain "financial incentives".
I can not offer the ?shiny gold nugget? or ?pure diamond? so far directly
which is often preferred.
> If that is in fact your goal, I would gently suggest that you spend
> more time improving your skills, and learning more about higher-value
> ways you could contribute to the kernel, instead of spamming the
> kernel list with lots of low value patches.
* I could extend my source code search patterns in principle.
How many developers and software reviewers struggle with results
from existing code analysis tools?
* Will your interest occasionally grow for collateral software evolution?
> In the future if you are adding higher value improvements, and you want
> to do various cleanups, such as fixing up seq_printf -> seq_puts changes, sure.
Is this kind of feedback a contradiction at the moment when you seem to give
the impression that my software development reputation is so damaged in the
?junk food? sense that I could hardly achieve the software change mixture
which you would prefer?
Regards,
Markus
On Mon, Oct 24, 2016 at 2:30 PM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 24 Oct 2016 13:54:58 +0200
>
> Some data were printed into a sequence by four separate function calls.
> Print the same data by one function call instead.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> arch/mips/kernel/proc.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
> index 07480a9..1047a03 100644
> --- a/arch/mips/kernel/proc.c
> +++ b/arch/mips/kernel/proc.c
> @@ -142,12 +142,15 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> seq_printf(m, "micromips kernel\t: %s\n",
> (read_c0_config3() & MIPS_CONF3_ISA_OE) ? "yes" : "no");
> }
> - seq_printf(m, "shadow register sets\t: %d\n",
> - cpu_data[n].srsets);
> - seq_printf(m, "kscratch registers\t: %d\n",
> - hweight8(cpu_data[n].kscratch_mask));
> - seq_printf(m, "package\t\t\t: %d\n", cpu_data[n].package);
> - seq_printf(m, "core\t\t\t: %d\n", cpu_data[n].core);
> + seq_printf(m,
> + "shadow register sets\t: %d\n"
> + "kscratch registers\t: %d\n"
> + "package\t\t\t: %d\n"
> + "core\t\t\t: %d\n",
> + cpu_data[n].srsets,
> + hweight8(cpu_data[n].kscratch_mask),
> + cpu_data[n].package,
> + cpu_data[n].core);
I think the code is much easier to read with separate seq_printf()s for
each line printed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Oct 25, 2016 at 10:55:42AM +0200, Geert Uytterhoeven wrote:
> > - seq_printf(m, "shadow register sets\t: %d\n",
> > - cpu_data[n].srsets);
> > - seq_printf(m, "kscratch registers\t: %d\n",
> > - hweight8(cpu_data[n].kscratch_mask));
> > - seq_printf(m, "package\t\t\t: %d\n", cpu_data[n].package);
> > - seq_printf(m, "core\t\t\t: %d\n", cpu_data[n].core);
> > + seq_printf(m,
> > + "shadow register sets\t: %d\n"
> > + "kscratch registers\t: %d\n"
> > + "package\t\t\t: %d\n"
> > + "core\t\t\t: %d\n",
> > + cpu_data[n].srsets,
> > + hweight8(cpu_data[n].kscratch_mask),
> > + cpu_data[n].package,
> > + cpu_data[n].core);
>
> I think the code is much easier to read with separate seq_printf()s for
> each line printed.
Which is why I originally implemented this as separate function calls.
Code size and performance are hardly an argument for /proc/cpuinfo.
Ralf