* [PATCH] ppc: remove excessive logging @ 2019-12-14 12:13 Joakim Tjernlund 2019-12-14 22:51 ` no-reply 2019-12-15 5:08 ` Philippe Mathieu-Daudé 0 siblings, 2 replies; 9+ messages in thread From: Joakim Tjernlund @ 2019-12-14 12:13 UTC (permalink / raw) To: qemu-devel; +Cc: Joakim Tjernlund 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-14 12:13 [PATCH] ppc: remove excessive logging Joakim Tjernlund @ 2019-12-14 22:51 ` no-reply 2019-12-15 5:08 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 9+ messages in thread From: no-reply @ 2019-12-14 22:51 UTC (permalink / raw) To: joakim.tjernlund; +Cc: Joakim.Tjernlund, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-14 12:13 [PATCH] ppc: remove excessive logging Joakim Tjernlund 2019-12-14 22:51 ` no-reply @ 2019-12-15 5:08 ` Philippe Mathieu-Daudé 2019-12-15 10:59 ` BALATON Zoltan 1 sibling, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-15 5:08 UTC (permalink / raw) To: Joakim Tjernlund, qemu-devel, David Gibson, qemu-ppc 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; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-15 5:08 ` Philippe Mathieu-Daudé @ 2019-12-15 10:59 ` BALATON Zoltan 2019-12-15 21:15 ` Joakim Tjernlund 2019-12-17 0:31 ` David Gibson 0 siblings, 2 replies; 9+ messages in thread From: BALATON Zoltan @ 2019-12-15 10:59 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Joakim Tjernlund, qemu-ppc, qemu-devel, David Gibson [-- Attachment #1: Type: text/plain, Size: 2807 bytes --] 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; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-15 10:59 ` BALATON Zoltan @ 2019-12-15 21:15 ` Joakim Tjernlund 2019-12-16 8:27 ` Thomas Huth 2019-12-17 0:31 ` David Gibson 1 sibling, 1 reply; 9+ messages in thread From: Joakim Tjernlund @ 2019-12-15 21:15 UTC (permalink / raw) To: balaton, philmd; +Cc: qemu-ppc, qemu-devel, david 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&data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C8fd615a611ec4bd9cce408d7814dda1a%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637120043688298205&sdata=d433DXO8SEaFJqAu73VTQwkZptmvK2eMAxivELGMcMI%3D&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; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-15 21:15 ` Joakim Tjernlund @ 2019-12-16 8:27 ` Thomas Huth 2019-12-16 8:39 ` david 2019-12-16 8:52 ` Joakim Tjernlund 0 siblings, 2 replies; 9+ messages in thread From: Thomas Huth @ 2019-12-16 8:27 UTC (permalink / raw) To: Joakim Tjernlund, balaton, philmd; +Cc: qemu-ppc, qemu-devel, david 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-16 8:27 ` Thomas Huth @ 2019-12-16 8:39 ` david 2019-12-16 8:52 ` Joakim Tjernlund 1 sibling, 0 replies; 9+ messages in thread From: david @ 2019-12-16 8:39 UTC (permalink / raw) To: Thomas Huth; +Cc: qemu-ppc, Joakim Tjernlund, philmd, qemu-devel [-- Attachment #1: Type: text/plain, Size: 904 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-16 8:27 ` Thomas Huth 2019-12-16 8:39 ` david @ 2019-12-16 8:52 ` Joakim Tjernlund 1 sibling, 0 replies; 9+ messages in thread From: Joakim Tjernlund @ 2019-12-16 8:52 UTC (permalink / raw) To: balaton, philmd, thuth; +Cc: qemu-ppc, qemu-devel, david 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppc: remove excessive logging 2019-12-15 10:59 ` BALATON Zoltan 2019-12-15 21:15 ` Joakim Tjernlund @ 2019-12-17 0:31 ` David Gibson 1 sibling, 0 replies; 9+ messages in thread From: David Gibson @ 2019-12-17 0:31 UTC (permalink / raw) To: BALATON Zoltan Cc: qemu-ppc, Joakim Tjernlund, Philippe Mathieu-Daudé, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3092 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-17 0:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-14 12:13 [PATCH] ppc: remove excessive logging Joakim Tjernlund 2019-12-14 22:51 ` no-reply 2019-12-15 5:08 ` Philippe Mathieu-Daudé 2019-12-15 10:59 ` BALATON Zoltan 2019-12-15 21:15 ` Joakim Tjernlund 2019-12-16 8:27 ` Thomas Huth 2019-12-16 8:39 ` david 2019-12-16 8:52 ` Joakim Tjernlund 2019-12-17 0:31 ` David Gibson
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).