ppc: remove excessive logging

[PATCH] ppc: remove excessive logging

Posted by Joakim Tjernlund 4 years, 4 months ago

From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>

ppc logs every type of Invalid instruction. This generates a lot
of garbage on console when sshd/ssh_keygen executes as
they try various insn to optimize its performance.
The invalid operation log is still there so an unknown insn
will still be logged.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
---

	As far as I can see, ppc is the only one emiting thsi
	debug msg for Invalid insns.

target/ppc/excp_helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 50b004d00d..45c2fa3ff9 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             env->spr[SPR_BOOKE_ESR] = ESR_FP;
             break;
         case POWERPC_EXCP_INVAL:
-            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n", env->nip);
             msr |= 0x00080000;
             env->spr[SPR_BOOKE_ESR] = ESR_PIL;
             break;
-- 
2.24.1

Re: [PATCH] ppc: remove excessive logging

Posted by no-reply@patchew.org 4 years, 4 months ago

Patchew URL: https://patchew.org/QEMU/20191214121347.17071-1-joakim.tjernlund@infinera.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] ppc: remove excessive logging
Type: series
Message-id: 20191214121347.17071-1-joakim.tjernlund@infinera.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

fatal: failed to write ref-pack file
fatal: The remote end hung up unexpectedly
Traceback (most recent call last):
  File "patchew-tester2/src/patchew-cli", line 531, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester2/src/patchew-cli", line 62, in git_clone_repo
    subprocess.check_call(clone_cmd, stderr=logf, stdout=logf)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'clone', '-q', '/home/patchew2/.cache/patchew-git-cache/httpsgithubcompatchewprojectqemu-3c8cf5a9c21ff8782164d1def7f44bd888713384', '/var/tmp/patchew-tester-tmp-rp1xseix/src']' returned non-zero exit status 128.



The full log is available at
http://patchew.org/logs/20191214121347.17071-1-joakim.tjernlund@infinera.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] ppc: remove excessive logging

Posted by Philippe Mathieu-Daudé 4 years, 4 months ago

Hi Joakim,

I'm cc'ing the PPC maintainers for you, so they won't miss your patch 
(see 
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer 
and the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).

On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
> From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> 
> ppc logs every type of Invalid instruction. This generates a lot
> of garbage on console when sshd/ssh_keygen executes as
> they try various insn to optimize its performance.
> The invalid operation log is still there so an unknown insn
> will still be logged.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> ---
> 
> 	As far as I can see, ppc is the only one emiting thsi
> 	debug msg for Invalid insns.
> 
> target/ppc/excp_helper.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 50b004d00d..45c2fa3ff9 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>               env->spr[SPR_BOOKE_ESR] = ESR_FP;
>               break;
>           case POWERPC_EXCP_INVAL:
> -            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n", env->nip);

I don't think we want to remove a such important log. Since it make 
sense to not disturb the console, maybe we can replace some of the 
LOG_EXCP() calls by qemu_log_mask(LOG_GUEST_ERROR,...) instead?

>               msr |= 0x00080000;
>               env->spr[SPR_BOOKE_ESR] = ESR_PIL;
>               break;
>

Re: [PATCH] ppc: remove excessive logging

Posted by BALATON Zoltan 4 years, 4 months ago

On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:
> Hi Joakim,
>
> I'm cc'ing the PPC maintainers for you, so they won't miss your patch (see 
> https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer and 
> the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).
>
> On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
>> From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
>> 
>> ppc logs every type of Invalid instruction. This generates a lot
>> of garbage on console when sshd/ssh_keygen executes as
>> they try various insn to optimize its performance.
>> The invalid operation log is still there so an unknown insn
>> will still be logged.
>> 
>> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
>> ---
>>
>> 	As far as I can see, ppc is the only one emiting thsi
>> 	debug msg for Invalid insns.
>> 
>> target/ppc/excp_helper.c | 1 -
>>   1 file changed, 1 deletion(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 50b004d00d..45c2fa3ff9 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>> excp_model, int excp)
>>               env->spr[SPR_BOOKE_ESR] = ESR_FP;
>>               break;
>>           case POWERPC_EXCP_INVAL:
>> -            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n", 
>> env->nip);
>
> I don't think we want to remove a such important log. Since it make sense to 
> not disturb the console, maybe we can replace some of the LOG_EXCP() calls by 
> qemu_log_mask(LOG_GUEST_ERROR,...) instead?

I don't think that's a good idea. That would flood the -d guest_errors log 
with unwanted messages that are normal not really due to guest errors. The 
LOG_EXCP() is not enabled by default, you have to edit source to enable it 
so you can also then edit the unwanted messages as well (like commenting 
this one out if you don't like it). If this is removed, invalid opcodes 
are still logged from translate.c but the exception generated for them is 
not logged. I don't know if that's useful or not but these are debug logs 
so depends on what do you want to debug. I don't mind this being removed 
but would be also happy leaving it as it is as it's only shown for 
developers who enable it and might help debugging. Or maybe these could be 
converted to traces (although I generally find traces to be less 
convenient to work with than debug logs and not sure about potential 
performance impact). So my preferences would be in order: 1. leave it 
alone, 2. remove it, 3. convert to traces.

Regards,
BALATON Zoltan

>
>>               msr |= 0x00080000;
>>               env->spr[SPR_BOOKE_ESR] = ESR_PIL;
>>               break;

Re: [PATCH] ppc: remove excessive logging

Posted by Joakim Tjernlund 4 years, 4 months ago

On Sun, 2019-12-15 at 11:59 +0100, BALATON Zoltan wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:
> > Hi Joakim,
> > 
> > I'm cc'ing the PPC maintainers for you, so they won't miss your patch (see
> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.qemu.org%2FContribute%2FSubmitAPatch%23CC_the_relevant_maintainer&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C8fd615a611ec4bd9cce408d7814dda1a%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637120043688298205&amp;sdata=d433DXO8SEaFJqAu73VTQwkZptmvK2eMAxivELGMcMI%3D&amp;reserved=0 and
> > the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).
> > 
> > On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
> > > From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > > 
> > > ppc logs every type of Invalid instruction. This generates a lot
> > > of garbage on console when sshd/ssh_keygen executes as
> > > they try various insn to optimize its performance.
> > > The invalid operation log is still there so an unknown insn
> > > will still be logged.
> > > 
> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > > ---
> > > 
> > >      As far as I can see, ppc is the only one emiting thsi
> > >      debug msg for Invalid insns.
> > > 
> > > target/ppc/excp_helper.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > > index 50b004d00d..45c2fa3ff9 100644
> > > --- a/target/ppc/excp_helper.c
> > > +++ b/target/ppc/excp_helper.c
> > > @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> > > excp_model, int excp)
> > >               env->spr[SPR_BOOKE_ESR] = ESR_FP;
> > >               break;
> > >           case POWERPC_EXCP_INVAL:
> > > -            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n",
> > > env->nip);
> > 
> > I don't think we want to remove a such important log. Since it make sense to
> > not disturb the console, maybe we can replace some of the LOG_EXCP() calls by
> > qemu_log_mask(LOG_GUEST_ERROR,...) instead?

Why is that so important? As far as I can tell ppc is the only arch doing this?

> 
> I don't think that's a good idea. That would flood the -d guest_errors log
> with unwanted messages that are normal not really due to guest errors. The

It not OK to flood some log but OK to flood the console?

> LOG_EXCP() is not enabled by default, you have to edit source to enable it

LOG_EXCP is enabled on Gentoo, what about other distros?

> so you can also then edit the unwanted messages as well (like commenting
> this one out if you don't like it). If this is removed, invalid opcodes
> are still logged from translate.c but the exception generated for them is
> not logged. I don't know if that's useful or not but these are debug logs
> so depends on what do you want to debug. I don't mind this being removed
> but would be also happy leaving it as it is as it's only shown for
> developers who enable it and might help debugging. Or maybe these could be
> converted to traces (although I generally find traces to be less
> convenient to work with than debug logs and not sure about potential
> performance impact). So my preferences would be in order: 1. leave it
> alone, 2. remove it, 3. convert to traces.
> 
> Regards,
> BALATON Zoltan
> 
> > >               msr |= 0x00080000;
> > >               env->spr[SPR_BOOKE_ESR] = ESR_PIL;
> > >               break;

Re: [PATCH] ppc: remove excessive logging

Posted by Thomas Huth 4 years, 4 months ago

On 15/12/2019 22.15, Joakim Tjernlund wrote:
[...]
>> LOG_EXCP() is not enabled by default, you have to edit source to enable it
> 
> LOG_EXCP is enabled on Gentoo, what about other distros?

I don't think that this is enabled by any other distro. Why is this
enabled on Gentoo at all? It really should not be enabled in builds that
are supposed to be used by normal users. Have you tried to contact the
package maintainers of the QEMU Gentoo package and asked them to disable
it there again?

 Thomas

Re: [PATCH] ppc: remove excessive logging

Posted by david@gibson.dropbear.id.au 4 years, 4 months ago

On Mon, Dec 16, 2019 at 09:27:13AM +0100, Thomas Huth wrote:
> On 15/12/2019 22.15, Joakim Tjernlund wrote:
> [...]
> >> LOG_EXCP() is not enabled by default, you have to edit source to enable it
> > 
> > LOG_EXCP is enabled on Gentoo, what about other distros?
> 
> I don't think that this is enabled by any other distro. Why is this
> enabled on Gentoo at all? It really should not be enabled in builds that
> are supposed to be used by normal users. Have you tried to contact the
> package maintainers of the QEMU Gentoo package and asked them to disable
> it there again?

I concur.  LOG_EXCP is definitely there for qemu developer debugging,
it's not intended for use in "normal" builds.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] ppc: remove excessive logging

Posted by Joakim Tjernlund 4 years, 4 months ago

On Mon, 2019-12-16 at 09:27 +0100, Thomas Huth wrote:
> 
> On 15/12/2019 22.15, Joakim Tjernlund wrote:
> [...]
> > > LOG_EXCP() is not enabled by default, you have to edit source to enable it
> > 
> > LOG_EXCP is enabled on Gentoo, what about other distros?
> 
> I don't think that this is enabled by any other distro. Why is this
> enabled on Gentoo at all? It really should not be enabled in builds that
> are supposed to be used by normal users. Have you tried to contact the
> package maintainers of the QEMU Gentoo package and asked them to disable
> it there again?

hmm, I have been carrying that patch for a long time(years) and now when I look into the code/package
I don't see it enabled any more so I will delete this patch now from my tree and see what happens.

 Jocke

Re: [PATCH] ppc: remove excessive logging

Posted by David Gibson 4 years, 4 months ago

On Sun, Dec 15, 2019 at 11:59:22AM +0100, BALATON Zoltan wrote:
> On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:
> > Hi Joakim,
> > 
> > I'm cc'ing the PPC maintainers for you, so they won't miss your patch
> > (see
> > https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer
> > and the output of ./scripts/get_maintainer.pl -f
> > target/ppc/excp_helper.c).
> > 
> > On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
> > > From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > > 
> > > ppc logs every type of Invalid instruction. This generates a lot
> > > of garbage on console when sshd/ssh_keygen executes as
> > > they try various insn to optimize its performance.
> > > The invalid operation log is still there so an unknown insn
> > > will still be logged.
> > > 
> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > > ---
> > > 
> > > 	As far as I can see, ppc is the only one emiting thsi
> > > 	debug msg for Invalid insns.
> > > 
> > > target/ppc/excp_helper.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > > index 50b004d00d..45c2fa3ff9 100644
> > > --- a/target/ppc/excp_helper.c
> > > +++ b/target/ppc/excp_helper.c
> > > @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu,
> > > int excp_model, int excp)
> > >               env->spr[SPR_BOOKE_ESR] = ESR_FP;
> > >               break;
> > >           case POWERPC_EXCP_INVAL:
> > > -            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n",
> > > env->nip);
> > 
> > I don't think we want to remove a such important log. Since it make
> > sense to not disturb the console, maybe we can replace some of the
> > LOG_EXCP() calls by qemu_log_mask(LOG_GUEST_ERROR,...) instead?
> 
> I don't think that's a good idea. That would flood the -d guest_errors log
> with unwanted messages that are normal not really due to guest errors. The
> LOG_EXCP() is not enabled by default, you have to edit source to enable it
> so you can also then edit the unwanted messages as well (like commenting
> this one out if you don't like it). If this is removed, invalid opcodes are
> still logged from translate.c but the exception generated for them is not
> logged. I don't know if that's useful or not but these are debug logs so
> depends on what do you want to debug. I don't mind this being removed but
> would be also happy leaving it as it is as it's only shown for developers
> who enable it and might help debugging. Or maybe these could be converted to
> traces (although I generally find traces to be less convenient to work with
> than debug logs and not sure about potential performance impact). So my
> preferences would be in order: 1. leave it alone, 2. remove it, 3. convert
> to traces.

Yes, that's my preference as well.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson