[PATCH 0/4] S390: Fine-tuning for six function implementations

[PATCH 0/4] S390: Fine-tuning for six function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 7 May 2017 19:00:09 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Combine two function calls into one in show_cacheinfo()
  Use seq_putc() in show_cpu_summary()
  Replace six seq_printf() calls by seq_puts()
  Combine two function calls into one at four places

 arch/s390/kernel/cache.c     |  4 ++--
 arch/s390/kernel/processor.c |  2 +-
 arch/s390/kernel/sysinfo.c   | 25 +++++++++++--------------
 3 files changed, 14 insertions(+), 17 deletions(-)

-- 
2.12.2

[PATCH 1/4] s390/cache: Combine two function calls into one in show_cacheinfo()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 7 May 2017 17:53:31 +0200

A bit of data was put into a sequence by two separate function calls.
Print the same data by a single function call instead.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/cache.c b/arch/s390/kernel/cache.c
index c8a83276a4dc..9e9752ce3233 100644
--- a/arch/s390/kernel/cache.c
+++ b/arch/s390/kernel/cache.c
@@ -82,8 +82,8 @@ void show_cacheinfo(struct seq_file *m)
 			   cache->disable_sysfs ? "Shared" : "Private");
 		seq_printf(m, "size=%dK ", cache->size >> 10);
 		seq_printf(m, "line_size=%u ", cache->coherency_line_size);
-		seq_printf(m, "associativity=%d", cache->ways_of_associativity);
-		seq_puts(m, "\n");
+		seq_printf(m, "associativity=%d\n",
+			   cache->ways_of_associativity);
 	}
 }
 
-- 
2.12.2

[PATCH 2/4] s390/processor: Use seq_putc() in show_cpu_summary()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 7 May 2017 18:03:11 +0200

A single character (line break) 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/s390/kernel/processor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kernel/processor.c b/arch/s390/kernel/processor.c
index 778cd6536175..b9a40c81e3d8 100644
--- a/arch/s390/kernel/processor.c
+++ b/arch/s390/kernel/processor.c
@@ -128,7 +128,7 @@ static void show_cpu_summary(struct seq_file *m, void *v)
 	for (i = 0; i < ARRAY_SIZE(int_hwcap_str); i++)
 		if (int_hwcap_str[i] && (int_hwcap & (1UL << i)))
 			seq_printf(m, "%s ", int_hwcap_str[i]);
-	seq_puts(m, "\n");
+	seq_putc(m, '\n');
 	show_facilities(m);
 	show_cacheinfo(m);
 	for_each_online_cpu(cpu) {
-- 
2.12.2

[PATCH 3/4] s390/sysinfo: Replace six seq_printf() calls by seq_puts()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 7 May 2017 18:18:45 +0200

Six strings which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/sysinfo.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/sysinfo.c b/arch/s390/kernel/sysinfo.c
index eefcb54872a5..e7061fcd047a 100644
--- a/arch/s390/kernel/sysinfo.c
+++ b/arch/s390/kernel/sysinfo.c
@@ -139,13 +139,13 @@ static void stsi_15_1_x(struct seq_file *m, struct sysinfo_15_1_x *info)
 		return;
 	if (stsi(info, 15, 1, topology_max_mnest))
 		return;
-	seq_printf(m, "CPU Topology HW:     ");
+	seq_puts(m, "CPU Topology HW:     ");
 	for (i = 0; i < TOPOLOGY_NR_MAG; i++)
 		seq_printf(m, " %d", info->mag[i]);
 	seq_putc(m, '\n');
 #ifdef CONFIG_SCHED_TOPOLOGY
 	store_topology(info);
-	seq_printf(m, "CPU Topology SW:     ");
+	seq_puts(m, "CPU Topology SW:     ");
 	for (i = 0; i < TOPOLOGY_NR_MAG; i++)
 		seq_printf(m, " %d", info->mag[i]);
 	seq_putc(m, '\n');
@@ -202,13 +202,13 @@ static void stsi_2_2_2(struct seq_file *m, struct sysinfo_2_2_2 *info)
 	EBCASC(info->name, sizeof(info->name));
 	seq_putc(m, '\n');
 	seq_printf(m, "LPAR Number:          %d\n", info->lpar_number);
-	seq_printf(m, "LPAR Characteristics: ");
+	seq_puts(m, "LPAR Characteristics: ");
 	if (info->characteristics & LPAR_CHAR_DEDICATED)
-		seq_printf(m, "Dedicated ");
+		seq_puts(m, "Dedicated ");
 	if (info->characteristics & LPAR_CHAR_SHARED)
-		seq_printf(m, "Shared ");
+		seq_puts(m, "Shared ");
 	if (info->characteristics & LPAR_CHAR_LIMITED)
-		seq_printf(m, "Limited ");
+		seq_puts(m, "Limited ");
 	seq_putc(m, '\n');
 	seq_printf(m, "LPAR Name:            %-8.8s\n", info->name);
 	seq_printf(m, "LPAR Adjustment:      %d\n", info->caf);
-- 
2.12.2

[PATCH 4/4] s390/sysinfo: Combine two function calls into one

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 7 May 2017 18:48:39 +0200

A bit of data was put into a sequence by two separate function calls
at four places. Print the same data by single function calls instead.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/sysinfo.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/sysinfo.c b/arch/s390/kernel/sysinfo.c
index e7061fcd047a..7f81d40c8f90 100644
--- a/arch/s390/kernel/sysinfo.c
+++ b/arch/s390/kernel/sysinfo.c
@@ -98,8 +98,7 @@ static void stsi_1_1_1(struct seq_file *m, struct sysinfo_1_1_1 *info)
 	seq_printf(m, "Model:                %-16.16s", info->model_capacity);
 	if (info->model[0] != '\0')
 		seq_printf(m, " %-16.16s", info->model);
-	seq_putc(m, '\n');
-	seq_printf(m, "Sequence Code:        %-16.16s\n", info->sequence);
+	seq_printf(m, "\nSequence Code:        %-16.16s\n", info->sequence);
 	seq_printf(m, "Plant:                %-4.4s\n", info->plant);
 	seq_printf(m, "Model Capacity:       %-16.16s %08u\n",
 		   info->model_capacity, info->model_cap_rating);
@@ -200,8 +199,7 @@ static void stsi_2_2_2(struct seq_file *m, struct sysinfo_2_2_2 *info)
 	if (stsi(info, 2, 2, 2))
 		return;
 	EBCASC(info->name, sizeof(info->name));
-	seq_putc(m, '\n');
-	seq_printf(m, "LPAR Number:          %d\n", info->lpar_number);
+	seq_printf(m, "\nLPAR Number:          %d\n", info->lpar_number);
 	seq_puts(m, "LPAR Characteristics: ");
 	if (info->characteristics & LPAR_CHAR_DEDICATED)
 		seq_puts(m, "Dedicated ");
@@ -209,8 +207,7 @@ static void stsi_2_2_2(struct seq_file *m, struct sysinfo_2_2_2 *info)
 		seq_puts(m, "Shared ");
 	if (info->characteristics & LPAR_CHAR_LIMITED)
 		seq_puts(m, "Limited ");
-	seq_putc(m, '\n');
-	seq_printf(m, "LPAR Name:            %-8.8s\n", info->name);
+	seq_printf(m, "\nLPAR Name:            %-8.8s\n", info->name);
 	seq_printf(m, "LPAR Adjustment:      %d\n", info->caf);
 	seq_printf(m, "LPAR CPUs Total:      %d\n", info->cpus_total);
 	seq_printf(m, "LPAR CPUs Configured: %d\n", info->cpus_configured);
@@ -256,8 +253,8 @@ static void stsi_3_2_2(struct seq_file *m, struct sysinfo_3_2_2 *info)
 	for (i = 0; i < info->count; i++) {
 		EBCASC(info->vm[i].name, sizeof(info->vm[i].name));
 		EBCASC(info->vm[i].cpi, sizeof(info->vm[i].cpi));
-		seq_putc(m, '\n');
-		seq_printf(m, "VM%02d Name:            %-8.8s\n", i, info->vm[i].name);
+		seq_printf(m, "\nVM%02d Name:            %-8.8s\n",
+			   i, info->vm[i].name);
 		seq_printf(m, "VM%02d Control Program: %-16.16s\n", i, info->vm[i].cpi);
 		seq_printf(m, "VM%02d Adjustment:      %d\n", i, info->vm[i].caf);
 		seq_printf(m, "VM%02d CPUs Total:      %d\n", i, info->vm[i].cpus_total);
-- 
2.12.2

Re: [PATCH 0/4] S390: Fine-tuning for six function implementations

[Vegard Nossum]

On 05/07/17 19:12, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 7 May 2017 19:00:09 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
>   Combine two function calls into one in show_cacheinfo()
>   Use seq_putc() in show_cpu_summary()
>   Replace six seq_printf() calls by seq_puts()
>   Combine two function calls into one at four places
>
>  arch/s390/kernel/cache.c     |  4 ++--
>  arch/s390/kernel/processor.c |  2 +-
>  arch/s390/kernel/sysinfo.c   | 25 +++++++++++--------------
>  3 files changed, 14 insertions(+), 17 deletions(-)
>

I'm sorry, I wouldn't normally respond to this, but I was put on the Cc
after all so I'll give my feedback.

I think these patches are a waste of time and a resources.

It would be different if your patches fixed actual bugs. This is just
mindless code transformations that MAY in the best case save a few bytes
of code here and there (I don't know; you didn't say).

But the potential gains from these incredibly numerous and tiny patches
that don't fix anything are so small, it's a waste of time, bandwidth,
and mental capacity for you and for everybody involved.

I just searched my inbox for patches from you and you sent literally
_hundreds_ over the past few days, all doing this crazy printf/puts/putc
transformation.

Another bit of searching and I see that I'm not the first one giving you
this response:

https://lkml.org/lkml/2017/1/23/383 - Jens Axboe
https://lkml.org/lkml/2017/1/23/262 - Johannes Thumshirn
https://lkml.org/lkml/2017/1/12/513 - Cyrille Pitchen
https://lkml.org/lkml/2016/10/24/491 - Theodore Ts'o
https://lkml.org/lkml/2016/10/7/148 - Dan Carpenter
https://lkml.org/lkml/2016/9/14/58 - Christian Borntraeger

...and I'm sure there are many more.


Vegard

Re: S390: Fine-tuning for six function implementations

[SF Markus Elfring]

> It would be different if your patches fixed actual bugs.

I dare to point change possibilities out which correspond to a special error category.
There can be different opinions about their relevance for further software improvements.


> This is just mindless code transformations that MAY in the best case save a few bytes
> of code here and there (I don't know; you didn't say).

Do you know the run time characteristics for the discussed functions good enough?
http://elixir.free-electrons.com/linux/v4.11/source/fs/seq_file.c#L405


> But the potential gains from these incredibly numerous and tiny patches
> that don't fix anything are so small, it's a waste of time, bandwidth,
> and mental capacity for you and for everybody involved.

I suggest a bit of code reduction at various places once more.


> I just searched my inbox for patches from you and you sent literally
> _hundreds_ over the past few days,

I sent update suggestions in this scale since the year 2014.


> all doing this crazy printf/puts/putc transformation.

I agree that the corresponding number could be remarkable.
But there are also other source code search patterns involved besides information
around these logging functions.


> Another bit of searching and I see that I'm not the first one giving you
> this response:

You are right that some agreements and disagreements were expressed already
(depending on the software area).

Regards,
Markus

Re: [PATCH 0/4] S390: Fine-tuning for six function implementations

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@oracle.com> wrote:

> On 05/07/17 19:12, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 7 May 2017 19:00:09 +0200
> > 
> > A few update suggestions were taken into account
> > from static source code analysis.
> > 
> > Markus Elfring (4):
> >   Combine two function calls into one in show_cacheinfo()
> >   Use seq_putc() in show_cpu_summary()
> >   Replace six seq_printf() calls by seq_puts()
> >   Combine two function calls into one at four places
> > 
> >  arch/s390/kernel/cache.c     |  4 ++--
> >  arch/s390/kernel/processor.c |  2 +-
> >  arch/s390/kernel/sysinfo.c   | 25 +++++++++++--------------
> >  3 files changed, 14 insertions(+), 17 deletions(-)
> > 
> 
> I'm sorry, I wouldn't normally respond to this, but I was put on the Cc
> after all so I'll give my feedback.
> 
> I think these patches are a waste of time and a resources.

Agreed.

> It would be different if your patches fixed actual bugs. This is just mindless 
> code transformations that MAY in the best case save a few bytes of code here and 
> there (I don't know; you didn't say).

... they might also be acceptable if they came from a genuine newbie who does his 
first patch, or if these patches represented genuine interest in the subsystem in 
question, by being part of a larger work that adds new features or does some 
meaningful code transformations.

They don't: they are Cocceline generated trivial patches from all around the 
kernel, and there's probably thousands of such 'problems' in the kernel - do we 
really want the churn of thousands of patches?

The cost of individual patches might be small, but their cumulative effect is 
non-trivial if we add up all the extra noise and overhad this adds to the kernel 
development flow.

> But the potential gains from these incredibly numerous and tiny patches that 
> don't fix anything are so small, it's a waste of time, bandwidth, and mental 
> capacity for you and for everybody involved.
> 
> I just searched my inbox for patches from you and you sent literally _hundreds_ 
> over the past few days, all doing this crazy printf/puts/putc transformation.
> 
> Another bit of searching and I see that I'm not the first one giving you this 
> response:
> 
> https://lkml.org/lkml/2017/1/23/383 - Jens Axboe
> https://lkml.org/lkml/2017/1/23/262 - Johannes Thumshirn
> https://lkml.org/lkml/2017/1/12/513 - Cyrille Pitchen
> https://lkml.org/lkml/2016/10/24/491 - Theodore Ts'o
> https://lkml.org/lkml/2016/10/7/148 - Dan Carpenter
> https://lkml.org/lkml/2016/9/14/58 - Christian Borntraeger
> 
> ...and I'm sure there are many more.

I'm ignoring these minimal effort patches for subsystems I maintain and I suggest 
other maintainers do the same.

Thanks,

	Ingo

Re: S390: Fine-tuning for six function implementations

[SF Markus Elfring]

> ... they might also be acceptable if they came from a genuine newbie 
> who does his first patch,

How would you really react if such Linux development beginners would dare
to pick any of the shown software change opportunities up?


> or if these patches represented genuine interest in the subsystem in question,

I am interested in various improvements where their size is varying between small
and big as usual.


> by being part of a larger work that adds new features or does some meaningful
> code transformations.

These updates happened also several times.

Regards,
Markus