[PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[SF Markus Elfring]

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

Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[Julia Lawall]

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
>

Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[SF Markus Elfring]

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

Re: [PATCH] Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[Theodore Ts'o]

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

Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[SF Markus Elfring]

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

Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

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

Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[SF Markus Elfring]

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
> 

Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[SF Markus Elfring]

> 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

Re: Hexagon-setup: Combine four seq_printf() calls into one call in show_cpuinfo()

[Richard Kuo]

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