linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
@ 2016-10-21  6:33 SF Markus Elfring
  2016-10-21  7:20 ` Julia Lawall
  0 siblings, 1 reply; 9+ messages in thread
From: SF Markus Elfring @ 2016-10-21  6:33 UTC (permalink / raw)
  To: linux-hexagon, Richard Kuo; +Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 21 Oct 2016 08:18:38 +0200

Some data were printed into a sequence by four 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/hexagon/kernel/setup.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/hexagon/kernel/setup.c b/arch/hexagon/kernel/setup.c
index 6981949..ff37044 100644
--- a/arch/hexagon/kernel/setup.c
+++ b/arch/hexagon/kernel/setup.c
@@ -132,13 +132,14 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 	if (!cpu_online(cpu))
 		return 0;
 #endif
-
-	seq_printf(m, "processor\t: %d\n", cpu);
-	seq_printf(m, "model name\t: Hexagon Virtual Machine\n");
-	seq_printf(m, "BogoMips\t: %lu.%02lu\n",
-		(loops_per_jiffy * HZ) / 500000,
-		((loops_per_jiffy * HZ) / 5000) % 100);
-	seq_printf(m, "\n");
+	seq_printf(m,
+		   "processor\t: %d\n"
+		   "model name\t: Hexagon Virtual Machine\n"
+		   "BogoMips\t: %lu.%02lu\n"
+		   "\n",
+		   cpu,
+		   (loops_per_jiffy * HZ) / 500000,
+		   ((loops_per_jiffy * HZ) / 5000) % 100);
 	return 0;
 }
 
-- 
2.10.1

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

* Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21  6:33 [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo() SF Markus Elfring
@ 2016-10-21  7:20 ` Julia Lawall
  2016-10-21  9:00   ` SF Markus Elfring
  0 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2016-10-21  7:20 UTC (permalink / raw)
  To: SF Markus Elfring; +Cc: linux-hexagon, Richard Kuo, LKML, kernel-janitors



On Fri, 21 Oct 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 21 Oct 2016 08:18:38 +0200
>
> Some data were printed into a sequence by four 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/hexagon/kernel/setup.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/hexagon/kernel/setup.c b/arch/hexagon/kernel/setup.c
> index 6981949..ff37044 100644
> --- a/arch/hexagon/kernel/setup.c
> +++ b/arch/hexagon/kernel/setup.c
> @@ -132,13 +132,14 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  	if (!cpu_online(cpu))
>  		return 0;
>  #endif
> -
> -	seq_printf(m, "processor\t: %d\n", cpu);
> -	seq_printf(m, "model name\t: Hexagon Virtual Machine\n");
> -	seq_printf(m, "BogoMips\t: %lu.%02lu\n",
> -		(loops_per_jiffy * HZ) / 500000,
> -		((loops_per_jiffy * HZ) / 5000) % 100);
> -	seq_printf(m, "\n");
> +	seq_printf(m,
> +		   "processor\t: %d\n"
> +		   "model name\t: Hexagon Virtual Machine\n"
> +		   "BogoMips\t: %lu.%02lu\n"
> +		   "\n",
> +		   cpu,
> +		   (loops_per_jiffy * HZ) / 500000,
> +		   ((loops_per_jiffy * HZ) / 5000) % 100);

This looks completely pointless. It is harder to see how the arguments fit
into the strings and it is harder to see where the strings end and the
arguments begin.

julia


>  	return 0;
>  }
>
> --
> 2.10.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

* Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21  7:20 ` Julia Lawall
@ 2016-10-21  9:00   ` SF Markus Elfring
  2016-10-21 15:46     ` Theodore Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: SF Markus Elfring @ 2016-10-21  9:00 UTC (permalink / raw)
  To: Julia Lawall; +Cc: linux-hexagon, Richard Kuo, LKML, kernel-janitors

>> +++ b/arch/hexagon/kernel/setup.c
>> @@ -132,13 +132,14 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>>  	if (!cpu_online(cpu))
>>  		return 0;
>>  #endif
>> -
>> -	seq_printf(m, "processor\t: %d\n", cpu);
>> -	seq_printf(m, "model name\t: Hexagon Virtual Machine\n");
>> -	seq_printf(m, "BogoMips\t: %lu.%02lu\n",
>> -		(loops_per_jiffy * HZ) / 500000,
>> -		((loops_per_jiffy * HZ) / 5000) % 100);
>> -	seq_printf(m, "\n");
>> +	seq_printf(m,
>> +		   "processor\t: %d\n"
>> +		   "model name\t: Hexagon Virtual Machine\n"
>> +		   "BogoMips\t: %lu.%02lu\n"
>> +		   "\n",
>> +		   cpu,
>> +		   (loops_per_jiffy * HZ) / 500000,
>> +		   ((loops_per_jiffy * HZ) / 5000) % 100);
> 
> This looks completely pointless.

Thanks for your software development opinion in this use case.


> It is harder to see how the arguments fit into the strings
> and it is harder to see where the strings end and the arguments begin.

Is it really so difficult to interpret the suggested construction
of a single (and relatively small) format string?

Regards,
Markus

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

* Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21  9:00   ` SF Markus Elfring
@ 2016-10-21 15:46     ` Theodore Ts'o
  2016-10-21 17:33       ` SF Markus Elfring
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Ts'o @ 2016-10-21 15:46 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: Julia Lawall, linux-hexagon, Richard Kuo, LKML, kernel-janitors

On Fri, Oct 21, 2016 at 11:00:22AM +0200, SF Markus Elfring wrote:
> 
> > It is harder to see how the arguments fit into the strings
> > and it is harder to see where the strings end and the arguments begin.
> 
> Is it really so difficult to interpret the suggested construction
> of a single (and relatively small) format string?

It's not so difficult, so much as it makes things worse.  It's easier
the way it originally was.  It might be interesting to see if the
compiler could be taught to collapse the function calls, but (a) this
isn't performance critical, and (b) the number of bytes saved is
really tiny.  But at least if the compiler was doing the work, it
would at least deal with it everywhere.

					- Ted

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

* Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21 15:46     ` Theodore Ts'o
@ 2016-10-21 17:33       ` SF Markus Elfring
  2016-10-21 17:53         ` Theodore Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: SF Markus Elfring @ 2016-10-21 17:33 UTC (permalink / raw)
  To: Theodore Ts'o, linux-hexagon
  Cc: Julia Lawall, Richard Kuo, LKML, kernel-janitors

>> Is it really so difficult to interpret the suggested construction
>> of a single (and relatively small) format string?
> 
> It's not so difficult, so much as it makes things worse.  It's easier
> the way it originally was.

Thanks for your view on this refactoring approach.


> It might be interesting to see if the compiler could be taught
> to collapse the function calls,

How does this wish fit to your previous rejection?


> but (a) this isn't performance critical,

This can be.


> and (b) the number of bytes saved is really tiny.

I imagine that the corresponding code and data size reduction could
be occasionally useful, couldn't it?


> But at least if the compiler was doing the work, it would at least deal with
> it everywhere.

I would find such an optimisation possibility also nice.

Can it become acceptable to achieve a similar effect by the application
of scripts for the semantic patch language in the meantime?

Regards,
Markus

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

* Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21 17:33       ` SF Markus Elfring
@ 2016-10-21 17:53         ` Theodore Ts'o
  2016-10-21 18:28           ` SF Markus Elfring
  2016-10-21 18:50           ` SF Markus Elfring
  0 siblings, 2 replies; 9+ messages in thread
From: Theodore Ts'o @ 2016-10-21 17:53 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: linux-hexagon, Julia Lawall, Richard Kuo, LKML, kernel-janitors

On Fri, Oct 21, 2016 at 07:33:30PM +0200, SF Markus Elfring wrote:
> 
> > but (a) this isn't performance critical,
> 
> This can be.

In this case, no, it really can't possibly be performance critical.
If you can't see why, you have no business trying to send patches.

> > and (b) the number of bytes saved is really tiny.
> 
> I imagine that the corresponding code and data size reduction could
> be occasionally useful, couldn't it?

Note that in some cases, attempts to shirnk the code by tiny amounts
can actually, paradoxically, cause the code to actually *expand*.  In
any case, shrinking the kernel by 0.00015% really won't matter, since
for no other reason, we round memory used to 4k pages.

So keeping the code easily readible is aslo a consideration that needs
to be taken into acconut.

> > But at least if the compiler was doing the work, it would at least deal with
> > it everywhere.
> 
> I would find such an optimisation possibility also nice.
> 
> Can it become acceptable to achieve a similar effect by the application
> of scripts for the semantic patch language in the meantime?

The problem with scripts like this is that they very clearly don't
have any human judgement.  And when the person using the scripts also
doesn't seem to have good judgement, it's a real problem.

When the author of the semantic patch language is telling you to stand
down, and you still want to try to argue for blind application of
patches, we have a really big problem.  Especially when some of your
patches have actually introduced bugs.

					- Ted

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

* Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21 17:53         ` Theodore Ts'o
@ 2016-10-21 18:28           ` SF Markus Elfring
  2016-10-21 18:50           ` SF Markus Elfring
  1 sibling, 0 replies; 9+ messages in thread
From: SF Markus Elfring @ 2016-10-21 18:28 UTC (permalink / raw)
  To: Theodore Ts'o, Julia Lawall
  Cc: linux-hexagon, Richard Kuo, LKML, kernel-janitors

Am 21.10.2016 um 19:53 schrieb Theodore Ts'o:
> On Fri, Oct 21, 2016 at 07:33:30PM +0200, SF Markus Elfring wrote:
>>
>>> but (a) this isn't performance critical,
>>
>> This can be.
> 
> In this case, no, it really can't possibly be performance critical.
> If you can't see why, you have no business trying to send patches.
> 
>>> and (b) the number of bytes saved is really tiny.
>>
>> I imagine that the corresponding code and data size reduction could
>> be occasionally useful, couldn't it?
> 
> Note that in some cases, attempts to shirnk the code by tiny amounts
> can actually, paradoxically, cause the code to actually *expand*.  In
> any case, shrinking the kernel by 0.00015% really won't matter, since
> for no other reason, we round memory used to 4k pages.
> 
> So keeping the code easily readible is aslo a consideration that needs
> to be taken into acconut.
> 
>>> But at least if the compiler was doing the work, it would at least deal with
>>> it everywhere.
>>
>> I would find such an optimisation possibility also nice.
>>
>> Can it become acceptable to achieve a similar effect by the application
>> of scripts for the semantic patch language in the meantime?
> 
> The problem with scripts like this is that they very clearly don't
> have any human judgement.  And when the person using the scripts also
> doesn't seem to have good judgement, it's a real problem.
> 
> When the author of the semantic patch language is telling you to stand down,

A collaboration evolved between Julia and me during the years
where different software development opinions can be usual.
There are eventually further opinions from Linux contributors like you.


> and you still want to try to argue for blind application of patches,


> we have a really big problem. 
> Especially when some of your patches have actually introduced bugs.
> 
> 					- Ted
> 

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

* Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21 17:53         ` Theodore Ts'o
  2016-10-21 18:28           ` SF Markus Elfring
@ 2016-10-21 18:50           ` SF Markus Elfring
  2016-10-25 21:37             ` Richard Kuo
  1 sibling, 1 reply; 9+ messages in thread
From: SF Markus Elfring @ 2016-10-21 18:50 UTC (permalink / raw)
  To: Theodore Ts'o, Julia Lawall
  Cc: linux-hexagon, Richard Kuo, LKML, kernel-janitors

> When the author of the semantic patch language is telling you to stand down,

The collaboration evolved between Julia and me during the years somehow.
Different software development opinions occur then as usual.
Further opinions from contributors like you can eventually show variations
between disagreement and acceptance.


> and you still want to try to argue for blind application of patches,

I guess that we have got different views about "blind" tries.


> we have a really big problem.

I hope that potential communication difficulties can still be resolved.


> Especially when some of your patches have actually introduced bugs.

I assume that these incidents could be clarified further, couldn't they?

Regards,
Markus

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

* Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()
  2016-10-21 18:50           ` SF Markus Elfring
@ 2016-10-25 21:37             ` Richard Kuo
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Kuo @ 2016-10-25 21:37 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: Theodore Ts'o, Julia Lawall, linux-hexagon, LKML, kernel-janitors

I wrote it the original way precisely for readability; it's easier, at least
to me, to read and modify the old way.

However, in my development version I happen to be printing a lot more
stuff.

To test, I collapsed 18 of my seq_printf's into one call.  That reduced the
function size by a couple hundred bytes.  (Didn't do anything for the final
kernel size though.)

If it makes things better, even if only slightly, doesn't introduce bugs, 
and doesn't otherwise violate any other rules (correct me if I'm wrong), I
would personally accept the minor readability tradeoff in this case.


Acked-by: Richard Kuo <rkuo@codeaurora.org>



On Fri, Oct 21, 2016 at 08:50:11PM +0200, SF Markus Elfring wrote:
> > When the author of the semantic patch language is telling you to stand down,
> 
> The collaboration evolved between Julia and me during the years somehow.
> Different software development opinions occur then as usual.
> Further opinions from contributors like you can eventually show variations
> between disagreement and acceptance.
> 
> 
> > and you still want to try to argue for blind application of patches,
> 
> I guess that we have got different views about "blind" tries.
> 
> 
> > we have a really big problem.
> 
> I hope that potential communication difficulties can still be resolved.
> 
> 
> > Especially when some of your patches have actually introduced bugs.
> 
> I assume that these incidents could be clarified further, couldn't they?
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hexagon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-21  6:33 [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo() SF Markus Elfring
2016-10-21  7:20 ` Julia Lawall
2016-10-21  9:00   ` SF Markus Elfring
2016-10-21 15:46     ` Theodore Ts'o
2016-10-21 17:33       ` SF Markus Elfring
2016-10-21 17:53         ` Theodore Ts'o
2016-10-21 18:28           ` SF Markus Elfring
2016-10-21 18:50           ` SF Markus Elfring
2016-10-25 21:37             ` Richard Kuo

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