linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations
@ 2016-10-24 12:26 SF Markus Elfring
  2016-10-24 12:27 ` [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show() SF Markus Elfring
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 12:26 UTC (permalink / raw)
  To: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel
  Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 12:26 [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations SF Markus Elfring
@ 2016-10-24 12:27 ` SF Markus Elfring
  2016-10-24 13:13   ` Theodore Ts'o
  2016-10-24 12:28 ` [PATCH 2/4] MIPS/kernel/proc: Use seq_putc() in show_cpuinfo() SF Markus Elfring
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 12:27 UTC (permalink / raw)
  To: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel
  Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] MIPS/kernel/proc: Use seq_putc() in show_cpuinfo()
  2016-10-24 12:26 [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations SF Markus Elfring
  2016-10-24 12:27 ` [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show() SF Markus Elfring
@ 2016-10-24 12:28 ` SF Markus Elfring
  2016-10-24 12:29 ` [PATCH 3/4] MIPS/kernel/proc: Replace 28 seq_printf() calls by seq_puts() " SF Markus Elfring
  2016-10-24 12:30 ` [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call " SF Markus Elfring
  3 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 12:28 UTC (permalink / raw)
  To: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel
  Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] MIPS/kernel/proc: Replace 28 seq_printf() calls by seq_puts() in show_cpuinfo()
  2016-10-24 12:26 [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations SF Markus Elfring
  2016-10-24 12:27 ` [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show() SF Markus Elfring
  2016-10-24 12:28 ` [PATCH 2/4] MIPS/kernel/proc: Use seq_putc() in show_cpuinfo() SF Markus Elfring
@ 2016-10-24 12:29 ` SF Markus Elfring
  2016-10-24 12:30 ` [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call " SF Markus Elfring
  3 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 12:29 UTC (permalink / raw)
  To: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel
  Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-24 12:26 [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations SF Markus Elfring
                   ` (2 preceding siblings ...)
  2016-10-24 12:29 ` [PATCH 3/4] MIPS/kernel/proc: Replace 28 seq_printf() calls by seq_puts() " SF Markus Elfring
@ 2016-10-24 12:30 ` SF Markus Elfring
  2016-10-25  8:55   ` Geert Uytterhoeven
  3 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 12:30 UTC (permalink / raw)
  To: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel
  Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 12:27 ` [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show() SF Markus Elfring
@ 2016-10-24 13:13   ` Theodore Ts'o
  2016-10-24 14:02     ` SF Markus Elfring
  0 siblings, 1 reply; 13+ messages in thread
From: Theodore Ts'o @ 2016-10-24 13:13 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 13:13   ` Theodore Ts'o
@ 2016-10-24 14:02     ` SF Markus Elfring
  2016-10-24 14:20       ` Theodore Ts'o
  0 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 14:02 UTC (permalink / raw)
  To: Theodore Ts'o, linux-mips
  Cc: Andrea Gelmini, Andrew Morton, Leonid Yegoshin, Masahiro Yamada,
	Matt Redfearn, Paul Burton, Paul Gortmaker, Ralf Bächle,
	Zubair Lutfullah Kakakhel, LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 14:02     ` SF Markus Elfring
@ 2016-10-24 14:20       ` Theodore Ts'o
  2016-10-24 14:53         ` SF Markus Elfring
  0 siblings, 1 reply; 13+ messages in thread
From: Theodore Ts'o @ 2016-10-24 14:20 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 14:20       ` Theodore Ts'o
@ 2016-10-24 14:53         ` SF Markus Elfring
  2016-10-24 15:51           ` Theodore Ts'o
  0 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 14:53 UTC (permalink / raw)
  To: Theodore Ts'o, linux-mips
  Cc: Andrea Gelmini, Andrew Morton, Leonid Yegoshin, Masahiro Yamada,
	Matt Redfearn, Paul Burton, Paul Gortmaker, Ralf Bächle,
	Zubair Lutfullah Kakakhel, LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show()
  2016-10-24 14:53         ` SF Markus Elfring
@ 2016-10-24 15:51           ` Theodore Ts'o
  2016-10-24 18:10             ` Further software improvements around Linux sequence API? SF Markus Elfring
  0 siblings, 1 reply; 13+ messages in thread
From: Theodore Ts'o @ 2016-10-24 15:51 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: linux-mips, Andrea Gelmini, Andrew Morton, Leonid Yegoshin,
	Masahiro Yamada, Matt Redfearn, Paul Burton, Paul Gortmaker,
	Ralf Bächle, Zubair Lutfullah Kakakhel, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Further software improvements around Linux sequence API?
  2016-10-24 15:51           ` Theodore Ts'o
@ 2016-10-24 18:10             ` SF Markus Elfring
  0 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-10-24 18:10 UTC (permalink / raw)
  To: Theodore Ts'o, linux-mips
  Cc: Andrea Gelmini, Andrew Morton, Leonid Yegoshin, Masahiro Yamada,
	Matt Redfearn, Paul Burton, Paul Gortmaker, Ralf Bächle,
	Zubair Lutfullah Kakakhel, LKML, kernel-janitors

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-24 12:30 ` [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call " SF Markus Elfring
@ 2016-10-25  8:55   ` Geert Uytterhoeven
  2016-10-25  9:08     ` Ralf Baechle
  0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2016-10-25  8:55 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: Linux MIPS Mailing List, Andrea Gelmini, Andrew Morton,
	Leonid Yegoshin, Masahiro Yamada, Matt Redfearn, Paul Burton,
	Paul Gortmaker, Ralf Bächle, Zubair Lutfullah Kakakhel,
	LKML, kernel-janitors

On Mon, Oct 24, 2016 at 2:30 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  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 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-25  8:55   ` Geert Uytterhoeven
@ 2016-10-25  9:08     ` Ralf Baechle
  0 siblings, 0 replies; 13+ messages in thread
From: Ralf Baechle @ 2016-10-25  9:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: SF Markus Elfring, Linux MIPS Mailing List, Andrea Gelmini,
	Andrew Morton, Leonid Yegoshin, Masahiro Yamada, Matt Redfearn,
	Paul Burton, Paul Gortmaker, Zubair Lutfullah Kakakhel, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-10-25  9:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-24 12:26 [PATCH 0/4] MIPS-kernel: Fine-tuning for two function implementations SF Markus Elfring
2016-10-24 12:27 ` [PATCH 1/4] MIPS/kernel/r2-to-r6-emul: Use seq_puts() in mipsr2_stats_show() SF Markus Elfring
2016-10-24 13:13   ` Theodore Ts'o
2016-10-24 14:02     ` SF Markus Elfring
2016-10-24 14:20       ` Theodore Ts'o
2016-10-24 14:53         ` SF Markus Elfring
2016-10-24 15:51           ` Theodore Ts'o
2016-10-24 18:10             ` Further software improvements around Linux sequence API? SF Markus Elfring
2016-10-24 12:28 ` [PATCH 2/4] MIPS/kernel/proc: Use seq_putc() in show_cpuinfo() SF Markus Elfring
2016-10-24 12:29 ` [PATCH 3/4] MIPS/kernel/proc: Replace 28 seq_printf() calls by seq_puts() " SF Markus Elfring
2016-10-24 12:30 ` [PATCH 4/4] MIPS/kernel/proc: Combine four seq_printf() calls into one call " SF Markus Elfring
2016-10-25  8:55   ` Geert Uytterhoeven
2016-10-25  9:08     ` Ralf Baechle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).