[Qemu-devel] Significant performance regression in qemu-system-mips.

[Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

I have a native build under qemu that gets killed if it doesn't produce a line 
of output for 60 seconds (hang detection enforced by the host monitoring 
qemu's stdout with --nographic, not from within qemu).

In the most recent release version, it never came close to triggering on mips 
with a 30 second timeout.  In the current -git version (well, as of Thursday 
anyway), it triggers frequently (about 90% of the time) even with a 60 second 
timeout.

I bisected it to this:

commit 1828be316f6637d43dd4c4f5f32925b17fb8107f
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Mar 10 11:38:41 2010 +0100

    more alarm timer cleanup
    
    The timer_alarm_pending variable is related to the alarm timer but not
    placed in the struct.  Also, in qemu_mod_timer the wrong flag was being
    tested: the timer is rearmed in the alarm timer "bottom half", so the
    right flag to test there is the "pending" flag.
    
    Finally, I hoisted the NULL checks from alarm_has_dynticks to
    host_alarm_handler.
    
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Reverting that patch fixed it (git show HEAD | patch -R -p1), by which I mean 
three consecutive runs with 30 second timeout didn't trigger the hang 
detection.

Unfortunately, I can't revert that patch in current origin/master because most 
of the hunks fail...

Help?

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Artyom Tarasenko]

2010/3/24 Rob Landley <rob@landley.net>:
> I have a native build under qemu that gets killed if it doesn't produce a line
> of output for 60 seconds (hang detection enforced by the host monitoring
> qemu's stdout with --nographic, not from within qemu).
>
> In the most recent release version, it never came close to triggering on mips
> with a 30 second timeout.  In the current -git version (well, as of Thursday
> anyway), it triggers frequently (about 90% of the time) even with a 60 second
> timeout.

Are other platforms affected as well? Do your automated tests run
against qemu-sparc meanwhile?

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Thursday 25 March 2010 04:20:26 Artyom Tarasenko wrote:
> 2010/3/24 Rob Landley <rob@landley.net>:
> > I have a native build under qemu that gets killed if it doesn't produce a
> > line of output for 60 seconds (hang detection enforced by the host
> > monitoring qemu's stdout with --nographic, not from within qemu).
> >
> > In the most recent release version, it never came close to triggering on
> > mips with a 30 second timeout.  In the current -git version (well, as of
> > Thursday anyway), it triggers frequently (about 90% of the time) even
> > with a 60 second timeout.
>
> Are other platforms affected as well? Do your automated tests run
> against qemu-sparc meanwhile?

That was the only platform I hit this particular regression on.  It affects 
mips, mipsel, and mips64.

The arm, x86, and x86-64 targets built to the end just fine.

Sparc works fine from a performance perspective (the timeout doesn't trigger), 
it just dies building strace with:

  In file included from file.c:88:^M
  /usr/bin/../include/asm/stat.h:56: error: expected specifier-qualifier-list
    before 'uid16_t'^

Which is either an strace bug or something wrong with the kernel headers, 
either way I need too track that down and fix it.

Powerpc got broken by the 2.6.32->2.6.33 kernel upgrade (the hard drives don't 
work because something broke in DMA interrupt handling, I'm bisecting it), so 
I can't comment on its performance at the moment.  I'll get back to you on 
that one.

As far as I can tell the sh4 linux-kernel maintainer officially doesn't care 
about anybody who isn't employed by his company, so I'm not sure I still care 
about supporting that platform.  It's not real hardware, it's a one-company 
toy:

  http://permalink.gmane.org/gmane.linux.ports.sh.devel/7233
  http://permalink.gmane.org/gmane.linux.ports.sh.devel/7237

There aren't any system emulations for m68k and alpha upstream in qemu yet, 
although I live in hope...

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Thu, Mar 25, 2010 at 12:33:33PM -0500, Rob Landley wrote:
> On Thursday 25 March 2010 04:20:26 Artyom Tarasenko wrote:
> > 2010/3/24 Rob Landley <rob@landley.net>:
> > > I have a native build under qemu that gets killed if it doesn't produce a
> > > line of output for 60 seconds (hang detection enforced by the host
> > > monitoring qemu's stdout with --nographic, not from within qemu).
> > >
> > > In the most recent release version, it never came close to triggering on
> > > mips with a 30 second timeout.  In the current -git version (well, as of
> > > Thursday anyway), it triggers frequently (about 90% of the time) even
> > > with a 60 second timeout.
> >
> > Are other platforms affected as well? Do your automated tests run
> > against qemu-sparc meanwhile?
> 
> That was the only platform I hit this particular regression on.  It affects 
> mips, mipsel, and mips64.
> 
> The arm, x86, and x86-64 targets built to the end just fine.
> 
> Sparc works fine from a performance perspective (the timeout doesn't trigger), 
> it just dies building strace with:
> 
>   In file included from file.c:88:^M
>   /usr/bin/../include/asm/stat.h:56: error: expected specifier-qualifier-list
>     before 'uid16_t'^
> 
> Which is either an strace bug or something wrong with the kernel headers, 
> either way I need too track that down and fix it.
> 
> Powerpc got broken by the 2.6.32->2.6.33 kernel upgrade (the hard drives don't 
> work because something broke in DMA interrupt handling, I'm bisecting it), so 
> I can't comment on its performance at the moment.  I'll get back to you on 
> that one.

This has been broken in r680 of openbios. I haven't found time to find
the real problem though.

> As far as I can tell the sh4 linux-kernel maintainer officially doesn't care 
> about anybody who isn't employed by his company, so I'm not sure I still care 
> about supporting that platform.  It's not real hardware, it's a one-company 
> toy:
> 
>   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7233
>   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7237
> 

If you continue to attack people like that, not sure I'll continue to
care about your emails. Your emails on this mailing list are always
complaining, and it really starts to be annoying.

Aurelien

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Alexander Graf]

On 26.03.2010, at 00:25, Aurelien Jarno wrote:

> On Thu, Mar 25, 2010 at 12:33:33PM -0500, Rob Landley wrote:
>> On Thursday 25 March 2010 04:20:26 Artyom Tarasenko wrote:
>>> 2010/3/24 Rob Landley <rob@landley.net>:
>>>> I have a native build under qemu that gets killed if it doesn't produce a
>>>> line of output for 60 seconds (hang detection enforced by the host
>>>> monitoring qemu's stdout with --nographic, not from within qemu).
>>>> 
>>>> In the most recent release version, it never came close to triggering on
>>>> mips with a 30 second timeout.  In the current -git version (well, as of
>>>> Thursday anyway), it triggers frequently (about 90% of the time) even
>>>> with a 60 second timeout.
>>> 
>>> Are other platforms affected as well? Do your automated tests run
>>> against qemu-sparc meanwhile?
>> 
>> That was the only platform I hit this particular regression on.  It affects 
>> mips, mipsel, and mips64.
>> 
>> The arm, x86, and x86-64 targets built to the end just fine.
>> 
>> Sparc works fine from a performance perspective (the timeout doesn't trigger), 
>> it just dies building strace with:
>> 
>>  In file included from file.c:88:^M
>>  /usr/bin/../include/asm/stat.h:56: error: expected specifier-qualifier-list
>>    before 'uid16_t'^
>> 
>> Which is either an strace bug or something wrong with the kernel headers, 
>> either way I need too track that down and fix it.
>> 
>> Powerpc got broken by the 2.6.32->2.6.33 kernel upgrade (the hard drives don't 
>> work because something broke in DMA interrupt handling, I'm bisecting it), so 
>> I can't comment on its performance at the moment.  I'll get back to you on 
>> that one.
> 
> This has been broken in r680 of openbios. I haven't found time to find
> the real problem though.

Ugh.

Yeah, I really need to look into it again. It's probably something really simple. 99% of the code was if(newworld) conditionized.


Alex

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Thursday 25 March 2010 18:57:00 Alexander Graf wrote:
> > This has been broken in r680 of openbios. I haven't found time to find
> > the real problem though.
>
> Ugh.
>
> Yeah, I really need to look into it again. It's probably something really
> simple. 99% of the code was if(newworld) conditionized.

According to "qemu-system-ppc -M ?", g3beige is still the default.  The mac99 
machine is the default for ppc64.  (Are there newworlds that run a 32 bit ppc 
instead of a 64 bit ppc?)

I intend to get a mac99 kernel configured for ppc64, but haven't gotten around 
to adding that target yet.  I'm currently working on adding mips64.

> Alex

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Andreas Färber]

Am 26.03.2010 um 03:44 schrieb Rob Landley:

> According to "qemu-system-ppc -M ?", g3beige is still the default.   
> The mac99
> machine is the default for ppc64.  (Are there newworlds that run a  
> 32 bit ppc
> instead of a 64 bit ppc?)

There are. All G3 and G4 PowerMacs, iMacs, iBooks were ppc32 AFAIK.
I changed the ppc64 default to mac99 to get a working default  
configuration there.

Andreas

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Alexander Graf]

On 01.04.2010, at 15:49, Andreas Färber wrote:

> 
> Am 26.03.2010 um 03:44 schrieb Rob Landley:
> 
>> According to "qemu-system-ppc -M ?", g3beige is still the default.  The mac99
>> machine is the default for ppc64.  (Are there newworlds that run a 32 bit ppc
>> instead of a 64 bit ppc?)
> 
> There are. All G3 and G4 PowerMacs, iMacs, iBooks were ppc32 AFAIK.
> I changed the ppc64 default to mac99 to get a working default configuration there.

I just pushed a fix to OpenBIOS that should make oldworld macs boot again. I tested and verified that it makes -M g3beige work and doesn't break PPC64, both using KVM.

Alex

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Fri, Apr 02, 2010 at 01:42:27AM +0200, Alexander Graf wrote:
> 
> On 01.04.2010, at 15:49, Andreas Färber wrote:
> 
> > 
> > Am 26.03.2010 um 03:44 schrieb Rob Landley:
> > 
> >> According to "qemu-system-ppc -M ?", g3beige is still the default.  The mac99
> >> machine is the default for ppc64.  (Are there newworlds that run a 32 bit ppc
> >> instead of a 64 bit ppc?)
> > 
> > There are. All G3 and G4 PowerMacs, iMacs, iBooks were ppc32 AFAIK.
> > I changed the ppc64 default to mac99 to get a working default configuration there.
> 
> I just pushed a fix to OpenBIOS that should make oldworld macs boot again. I tested and verified that it makes -M g3beige work and doesn't break PPC64, both using KVM.
> 

I confirm it fixes the problem, thanks a lot. I have pushed the new
version of OpenBIOS to git HEAD.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Thursday 01 April 2010 18:42:27 Alexander Graf wrote:
> On 01.04.2010, at 15:49, Andreas Färber wrote:
> > Am 26.03.2010 um 03:44 schrieb Rob Landley:
> >> According to "qemu-system-ppc -M ?", g3beige is still the default.  The
> >> mac99 machine is the default for ppc64.  (Are there newworlds that run a
> >> 32 bit ppc instead of a 64 bit ppc?)
> >
> > There are. All G3 and G4 PowerMacs, iMacs, iBooks were ppc32 AFAIK.
> > I changed the ppc64 default to mac99 to get a working default
> > configuration there.
>
> I just pushed a fix to OpenBIOS that should make oldworld macs boot again.
> I tested and verified that it makes -M g3beige work and doesn't break
> PPC64, both using KVM.
>
> Alex

Confirmed, ppc32 g3beige working for me.

Thanks,

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Thursday 25 March 2010 18:25:41 Aurelien Jarno wrote:
> On Thu, Mar 25, 2010 at 12:33:33PM -0500, Rob Landley wrote:
> > On Thursday 25 March 2010 04:20:26 Artyom Tarasenko wrote:
> > > 2010/3/24 Rob Landley <rob@landley.net>:
> > > > I have a native build under qemu that gets killed if it doesn't
> > > > produce a line of output for 60 seconds (hang detection enforced by
> > > > the host monitoring qemu's stdout with --nographic, not from within
> > > > qemu).
> > > >
> > > > In the most recent release version, it never came close to triggering
> > > > on mips with a 30 second timeout.  In the current -git version (well,
> > > > as of Thursday anyway), it triggers frequently (about 90% of the
> > > > time) even with a 60 second timeout.
> > >
> > > Are other platforms affected as well? Do your automated tests run
> > > against qemu-sparc meanwhile?
> >
> > That was the only platform I hit this particular regression on.  It
> > affects mips, mipsel, and mips64.
> >
> > The arm, x86, and x86-64 targets built to the end just fine.
> >
> > Sparc works fine from a performance perspective (the timeout doesn't
> > trigger), it just dies building strace with:
> >
> >   In file included from file.c:88:^M
> >   /usr/bin/../include/asm/stat.h:56: error: expected
> > specifier-qualifier-list before 'uid16_t'^
> >
> > Which is either an strace bug or something wrong with the kernel headers,
> > either way I need too track that down and fix it.
> >
> > Powerpc got broken by the 2.6.32->2.6.33 kernel upgrade (the hard drives
> > don't work because something broke in DMA interrupt handling, I'm
> > bisecting it), so I can't comment on its performance at the moment.  I'll
> > get back to you on that one.
>
> This has been broken in r680 of openbios. I haven't found time to find
> the real problem though.

According to the boot messages, it looks like it moved the IRQ of the IDE 
controller from 19 to 16, thus making it shared with the serial controller.

This boots:

  ttyS0 at MMIO 0x80893020 (irq = 16) is a Z85c30 ESCC - Serial port
  ttyS1 at MMIO 0x80893000 (irq = 17) is a Z85c30 ESCC - Serial port
  ide-pmac: Found Apple Heathrow ATA controller (macio), bus ID 0, irq 19
  ide0 at 0xd1010000-0xd1010070,0xd1010160 on irq 19

This does not:

  ttyS0 at MMIO 0x80893020 (irq = 16) is a Z85c30 ESCC - Serial port
  ttyS1 at MMIO 0x80893000 (irq = 17) is a Z85c30 ESCC - Serial port
  ide-pmac: Found Apple Heathrow ATA controller (macio), bus ID 0, irq 16
  ide0 at 0xd1010000-0xd1010070,0xd1010160 on irq 16

> > As far as I can tell the sh4 linux-kernel maintainer officially doesn't
> > care about anybody who isn't employed by his company, so I'm not sure I
> > still care about supporting that platform.  It's not real hardware, it's
> > a one-company toy:
> >
> >   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7233
> >   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7237
>
> If you continue to attack people like that, not sure I'll continue to
> care about your emails.

I'm not asking anyone to care about me personally, I'm asking them to care 
about specific technical issues.  If those issues don't interest you, they 
don't interest you.

Speaking of ppc, last month I sent this patch:

  http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html

And I was under the impression people agreed with it:

  http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
  http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html

But today's -git is still having that same issue, and the same patch still 
applies cleanly and fixes it for me.

> Your emails on this mailing list are always
> complaining, and it really starts to be annoying.

I tend to email when something isn't working for me, and not email when things 
are working for me, yes.

In this case, I was listing the platforms I have existing .configs to build 
system images for, and explaining why I've currently lost interest in one of 
them (sh4), despite still being interested in even older things like the alpha 
and m68k (neither of which qemu has in-tree system emulations for yet).

I was unaware of attacking anyone personally.  I've never met the sh4 
maintainer, that I'm aware of.  I disagree with his judgement call in this 
instance.  Of course I respect his right to take any position he likes, since 
he _is_ maintainer, but his position removes any motivation to put more of my 
own time into his platform just now.  (I feel similarly disinterested in 
itanium and pa-risc.)

> Aurelien

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Fri, Mar 26, 2010 at 04:53:02AM -0500, Rob Landley wrote:
> > > As far as I can tell the sh4 linux-kernel maintainer officially doesn't
> > > care about anybody who isn't employed by his company, so I'm not sure I
> > > still care about supporting that platform.  It's not real hardware, it's
> > > a one-company toy:
> > >
> > >   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7233
> > >   http://permalink.gmane.org/gmane.linux.ports.sh.devel/7237
> >
> > If you continue to attack people like that, not sure I'll continue to
> > care about your emails.
> 
> I'm not asking anyone to care about me personally, I'm asking them to care 
> about specific technical issues.  If those issues don't interest you, they 
> don't interest you.
> 
> Speaking of ppc, last month I sent this patch:
> 
>   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html
> 
> And I was under the impression people agreed with it:
> 
>   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
>   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html
> 
> But today's -git is still having that same issue, and the same patch still 
> applies cleanly and fixes it for me.

Re-read the last link you quoted, and especially this part:

| The
| same way using CONFIG_BSD in linux-user/elfload.c doesn't make sense,
| as this code will never been compiled.

While your patch goes in the good direction, it doesn't mean it is
correct. Conditionally compiling code on CONFIG_BSD in a Linux specific
file doesn't make sense.

I am pretty fine applying a correct patch if you send a new one.


> > Your emails on this mailing list are always
> > complaining, and it really starts to be annoying.
> 
> I tend to email when something isn't working for me, and not email when things 
> are working for me, yes.
> 
> In this case, I was listing the platforms I have existing .configs to build 
> system images for, and explaining why I've currently lost interest in one of 
> them (sh4), despite still being interested in even older things like the alpha 
> and m68k (neither of which qemu has in-tree system emulations for yet).
> 
> I was unaware of attacking anyone personally.  I've never met the sh4 
> maintainer, that I'm aware of.  I disagree with his judgement call in this 
> instance.  Of course I respect his right to take any position he likes, since 
> he _is_ maintainer, but his position removes any motivation to put more of my 
> own time into his platform just now.  (I feel similarly disinterested in 
> itanium and pa-risc.)
> 

I have no problem with you having no interest in sh4, a lot of people
are in you case. I don't think it gives you the right to describe the
sh4 kernel maintainer as "sh4 linux-kernel maintainer officially doesn't
care about anybody who isn't employed by his company", or later "It's 
not real hardware, it's a one-company toy". This is not something
reflected in link you quoted. Paul Mundt has been nicely answering your
question on this thread.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > I'm not asking anyone to care about me personally, I'm asking them to
> > care about specific technical issues.  If those issues don't interest
> > you, they don't interest you.
> >
> > Speaking of ppc, last month I sent this patch:
> >
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html
> >
> > And I was under the impression people agreed with it:
> >
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
> >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html
> >
> > But today's -git is still having that same issue, and the same patch
> > still applies cleanly and fixes it for me.
>
> Re-read the last link you quoted, and especially this part:
> | The
> | same way using CONFIG_BSD in linux-user/elfload.c doesn't make sense,
> | as this code will never been compiled.

I didn't know the BSD comments werer addressed at me.  (I haven't got a BSD
test system.)

> While your patch goes in the good direction, it doesn't mean it is
> correct. Conditionally compiling code on CONFIG_BSD in a Linux specific
> file doesn't make sense.

Ok.

> I am pretty fine applying a correct patch if you send a new one.

By which you mean rip out the whole #ifdef block?

Here you go:

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 682a813..44405dd 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -518,12 +518,11 @@ do {                                                                    \
 static inline void init_thread(struct target_pt_regs *_regs, struct image_info *infop)
 {
     abi_ulong pos = infop->start_stack;
-    abi_ulong tmp;
 #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
     abi_ulong entry, toc;
 #endif
 
-    _regs->gpr[1] = infop->start_stack;
+    _regs->gpr[1] = pos;
 #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
     entry = ldq_raw(infop->entry) + infop->load_addr;
     toc = ldq_raw(infop->entry + 8) + infop->load_addr;
@@ -531,17 +530,6 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
     infop->entry = entry;
 #endif
     _regs->nip = infop->entry;
-    /* Note that isn't exactly what regular kernel does
-     * but this is what the ABI wants and is needed to allow
-     * execution of PPC BSD programs.
-     */
-    /* FIXME - what to for failure of get_user()? */
-    get_user_ual(_regs->gpr[3], pos);
-    pos += sizeof(abi_ulong);
-    _regs->gpr[4] = pos;
-    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
-        tmp = ldl(pos);
-    _regs->gpr[5] = pos;
 }
 
 #define ELF_EXEC_PAGESIZE	4096

Re-tested and it works fine for me.

> I have no problem with you having no interest in sh4, a lot of people
> are in you case. I don't think it gives you the right to describe the
> sh4 kernel maintainer as "sh4 linux-kernel maintainer officially doesn't
> care about anybody who isn't employed by his company", or later "It's
> not real hardware, it's a one-company toy". This is not something
> reflected in link you quoted. Paul Mundt has been nicely answering your
> question on this thread.

Ok, I agree I was a bit harsh.  (He's the one who introduced his employer into
the discussion, but I suspect I read more into that than he meant by it.)

But this doesn't revive my interest in the platform.  He did confirm that he
doesn't care if users of even six-month-old tools could still build the thing,
and that he was willing to break toolchains that had built existing usable
kernels for years in the name of cleanup patches.  Add in the fact that
I've never encountered anyone actually using this hardware since the
Dreamcast, and that the first, third, and fourth hits googling for "sh4" are
the video games Silent Hunter 4 and Silent Hill 4.  (The first four hits for
"arm" are the processor despite everybody having two of that word attached to
their shoulders...)

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Fri, Mar 26, 2010 at 04:47:51PM -0500, Rob Landley wrote:
> On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > > I'm not asking anyone to care about me personally, I'm asking them to
> > > care about specific technical issues.  If those issues don't interest
> > > you, they don't interest you.
> > >
> > > Speaking of ppc, last month I sent this patch:
> > >
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00917.html
> > >
> > > And I was under the impression people agreed with it:
> > >
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01044.html
> > >   http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg01714.html
> > >
> > > But today's -git is still having that same issue, and the same patch
> > > still applies cleanly and fixes it for me.
> >
> > Re-read the last link you quoted, and especially this part:
> > | The
> > | same way using CONFIG_BSD in linux-user/elfload.c doesn't make sense,
> > | as this code will never been compiled.
> 
> I didn't know the BSD comments werer addressed at me.  (I haven't got a BSD
> test system.)
> 
> > While your patch goes in the good direction, it doesn't mean it is
> > correct. Conditionally compiling code on CONFIG_BSD in a Linux specific
> > file doesn't make sense.
> 
> Ok.
> 
> > I am pretty fine applying a correct patch if you send a new one.
> 
> By which you mean rip out the whole #ifdef block?

Yes.

> Here you go:

This looks much better. Can you please resend it with the changes below
and a Signed-off-by: ?

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 682a813..44405dd 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -518,12 +518,11 @@ do {                                                                    \
>  static inline void init_thread(struct target_pt_regs *_regs, struct image_info *infop)
>  {
>      abi_ulong pos = infop->start_stack;

This line should probably be removed...

> -    abi_ulong tmp;
>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
>      abi_ulong entry, toc;
>  #endif
>  
> -    _regs->gpr[1] = infop->start_stack;
> +    _regs->gpr[1] = pos;

...instead of doing the change here.

>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
>      entry = ldq_raw(infop->entry) + infop->load_addr;
>      toc = ldq_raw(infop->entry + 8) + infop->load_addr;
> @@ -531,17 +530,6 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
>      infop->entry = entry;
>  #endif
>      _regs->nip = infop->entry;
> -    /* Note that isn't exactly what regular kernel does
> -     * but this is what the ABI wants and is needed to allow
> -     * execution of PPC BSD programs.
> -     */
> -    /* FIXME - what to for failure of get_user()? */
> -    get_user_ual(_regs->gpr[3], pos);
> -    pos += sizeof(abi_ulong);
> -    _regs->gpr[4] = pos;
> -    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
> -        tmp = ldl(pos);
> -    _regs->gpr[5] = pos;
>  }
>  
>  #define ELF_EXEC_PAGESIZE	4096
> 
> Re-tested and it works fine for me.
> 
> > I have no problem with you having no interest in sh4, a lot of people
> > are in you case. I don't think it gives you the right to describe the
> > sh4 kernel maintainer as "sh4 linux-kernel maintainer officially doesn't
> > care about anybody who isn't employed by his company", or later "It's
> > not real hardware, it's a one-company toy". This is not something
> > reflected in link you quoted. Paul Mundt has been nicely answering your
> > question on this thread.
> 
> Ok, I agree I was a bit harsh.  (He's the one who introduced his employer into
> the discussion, but I suspect I read more into that than he meant by it.)
> 

I think you misunderstood him. You were talking about Super-Hitachi
which is a train [1] from Hitachi (not his employer), while he was talking
about Super-H which is a CPU [2] from Renesas (his employer).

He has the right to not care about trains ;-)

[1] http://en.wikipedia.org/wiki/Hitachi_(Japanese_train)
[2] http://en.wikipedia.org/wiki/SuperH

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Saturday 27 March 2010 07:32:41 Aurelien Jarno wrote:
> On Fri, Mar 26, 2010 at 04:47:51PM -0500, Rob Landley wrote:
> > On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > > I am pretty fine applying a correct patch if you send a new one.
> >
> > By which you mean rip out the whole #ifdef block?
>
> Yes.
>
> > Here you go:
>
> This looks much better. Can you please resend it with the changes below
> and a Signed-off-by: ?

If you want the code actually cleaned up instead of minimally changed,
here's a stab at that.  (Unfortunately I haven't got a ppc64 setup to test it
with yet, but ppc32 still works.)

Signed-off-by: Rob Landley <rob@landley.net>

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 682a813..3c3ef21 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -517,31 +517,12 @@ do {                                                                    \
 
 static inline void init_thread(struct target_pt_regs *_regs, struct image_info *infop)
 {
-    abi_ulong pos = infop->start_stack;
-    abi_ulong tmp;
-#if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
-    abi_ulong entry, toc;
-#endif
-
     _regs->gpr[1] = infop->start_stack;
 #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
-    entry = ldq_raw(infop->entry) + infop->load_addr;
-    toc = ldq_raw(infop->entry + 8) + infop->load_addr;
-    _regs->gpr[2] = toc;
-    infop->entry = entry;
+    _regs->gpr[2] = ldq_raw(infop->entry + 8) + infop->load_addr;
+    infop->entry = ldq_raw(infop->entry) + infop->load_addr;
 #endif
     _regs->nip = infop->entry;
-    /* Note that isn't exactly what regular kernel does
-     * but this is what the ABI wants and is needed to allow
-     * execution of PPC BSD programs.
-     */
-    /* FIXME - what to for failure of get_user()? */
-    get_user_ual(_regs->gpr[3], pos);
-    pos += sizeof(abi_ulong);
-    _regs->gpr[4] = pos;
-    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
-        tmp = ldl(pos);
-    _regs->gpr[5] = pos;
 }
 
 #define ELF_EXEC_PAGESIZE	4096

> > Ok, I agree I was a bit harsh.  (He's the one who introduced his employer
> > into the discussion, but I suspect I read more into that than he meant by
> > it.)
>
> I think you misunderstood him. You were talking about Super-Hitachi
> which is a train [1] from Hitachi (not his employer), while he was talking
> about Super-H which is a CPU [2] from Renesas (his employer).

So essentially he's insisting he works for Freescale, not Motorola, because
Motorola stopped being interested in the m68k and divested itself of its
processor manufacturing operations.  And I'm confusing his product with
something _else_ Motorola used to do.

Only transliterated to Japan.

*shrug*  The "SuperH" chipset was developed by Hitachi.  I thought the H stood
for "Hitachi".  I hadn't actually noticed that Hitachi had divested itself of
its chip design operations, and was trying to avoid referring to it as "sh4"
because that's an architecture generation, not a chip family.  (There used to
be sh3 and similar, and I thought there might be an sh5 someday but now that
I've looked into it I can understand why they don't seem too worried about
that happening.)

My project is trying to get all the architectures Linux and QEMU support to
behave the same way.  Thus I'm no more an sh4 expert than I am a ppc expert, I
just poke at it and look stuff up when it doesn't work (which is frequently).

Speaking of which, qemu-system-ppc in 0.12.3 segfaults accessing /dev/hdc, and
the one in current -git has the missed IRQ issue when accessing /dev/hda.  Is
there any chance of 0.12.4 in the near future?  (I hate to point people
interested in PPC at a random non-current git snapshot.)

> He has the right to not care about trains ;-)

It was more the "I can build it, I don't care if you still can" issue, when
the commit in question was a primarily cosmetic change to code that was only
theoretically broken.  (Not only did it work for me, but it was so broken
nobody actually noticed the issue in question for years.)

I got the impression that the reason he didn't care about my use case was
because I was not a customer of his company.  That he was acting on behalf
of his employer, not in an impartial purely technical capacity.  I have
no commercial interest in sh4, and never did, so I stopped bothering him.

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Sat, Mar 27, 2010 at 06:01:39PM -0500, Rob Landley wrote:
> On Saturday 27 March 2010 07:32:41 Aurelien Jarno wrote:
> > On Fri, Mar 26, 2010 at 04:47:51PM -0500, Rob Landley wrote:
> > > On Friday 26 March 2010 14:00:00 Aurelien Jarno wrote:
> > > > I am pretty fine applying a correct patch if you send a new one.
> > >
> > > By which you mean rip out the whole #ifdef block?
> >
> > Yes.
> >
> > > Here you go:
> >
> > This looks much better. Can you please resend it with the changes below
> > and a Signed-off-by: ?
> 
> If you want the code actually cleaned up instead of minimally changed,
> here's a stab at that.  (Unfortunately I haven't got a ppc64 setup to test it
> with yet, but ppc32 still works.)

Not necessarily a code cleanup, but at least a patch which doesn't
introduce useless changes. Anyway applied.

> Signed-off-by: Rob Landley <rob@landley.net>
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 682a813..3c3ef21 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -517,31 +517,12 @@ do {                                                                    \
>  
>  static inline void init_thread(struct target_pt_regs *_regs, struct image_info *infop)
>  {
> -    abi_ulong pos = infop->start_stack;
> -    abi_ulong tmp;
> -#if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
> -    abi_ulong entry, toc;
> -#endif
> -
>      _regs->gpr[1] = infop->start_stack;
>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
> -    entry = ldq_raw(infop->entry) + infop->load_addr;
> -    toc = ldq_raw(infop->entry + 8) + infop->load_addr;
> -    _regs->gpr[2] = toc;
> -    infop->entry = entry;
> +    _regs->gpr[2] = ldq_raw(infop->entry + 8) + infop->load_addr;
> +    infop->entry = ldq_raw(infop->entry) + infop->load_addr;
>  #endif
>      _regs->nip = infop->entry;
> -    /* Note that isn't exactly what regular kernel does
> -     * but this is what the ABI wants and is needed to allow
> -     * execution of PPC BSD programs.
> -     */
> -    /* FIXME - what to for failure of get_user()? */
> -    get_user_ual(_regs->gpr[3], pos);
> -    pos += sizeof(abi_ulong);
> -    _regs->gpr[4] = pos;
> -    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
> -        tmp = ldl(pos);
> -    _regs->gpr[5] = pos;
>  }
>  
>  #define ELF_EXEC_PAGESIZE	4096
> 
> > > Ok, I agree I was a bit harsh.  (He's the one who introduced his employer
> > > into the discussion, but I suspect I read more into that than he meant by
> > > it.)
> >
> > I think you misunderstood him. You were talking about Super-Hitachi
> > which is a train [1] from Hitachi (not his employer), while he was talking
> > about Super-H which is a CPU [2] from Renesas (his employer).
> 
> So essentially he's insisting he works for Freescale, not Motorola, because
> Motorola stopped being interested in the m68k and divested itself of its
> processor manufacturing operations.  And I'm confusing his product with
> something _else_ Motorola used to do.

He is insisted on the fact the name of the CPU is Super-H and not
Super-Hitachi.

> Only transliterated to Japan.
> 
> *shrug*  The "SuperH" chipset was developed by Hitachi.  I thought the H stood
> for "Hitachi".  I hadn't actually noticed that Hitachi had divested itself of
> its chip design operations, and was trying to avoid referring to it as "sh4"
> because that's an architecture generation, not a chip family.  (There used to
> be sh3 and similar, and I thought there might be an sh5 someday but now that
> I've looked into it I can understand why they don't seem too worried about
> that happening.)
> 
> My project is trying to get all the architectures Linux and QEMU support to
> behave the same way.  Thus I'm no more an sh4 expert than I am a ppc expert, I
> just poke at it and look stuff up when it doesn't work (which is frequently).
> 
> Speaking of which, qemu-system-ppc in 0.12.3 segfaults accessing /dev/hdc, and
> the one in current -git has the missed IRQ issue when accessing /dev/hda.  Is
> there any chance of 0.12.4 in the near future?  (I hate to point people
> interested in PPC at a random non-current git snapshot.)

It is something fixed in the stable-0.12. Someone has to roll out 0.12.4.

> > He has the right to not care about trains ;-)
> 
> It was more the "I can build it, I don't care if you still can" issue, when
> the commit in question was a primarily cosmetic change to code that was only
> theoretically broken.  (Not only did it work for me, but it was so broken
> nobody actually noticed the issue in question for years.)
> 
> I got the impression that the reason he didn't care about my use case was
> because I was not a customer of his company.  That he was acting on behalf
> of his employer, not in an impartial purely technical capacity.  I have
> no commercial interest in sh4, and never did, so I stopped bothering him.
> 
> Rob
> -- 
> Latency is more important than throughput. It's that simple. - Linus Torvalds
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Sunday 28 March 2010 09:57:09 Aurelien Jarno wrote:
> > If you want the code actually cleaned up instead of minimally changed,
> > here's a stab at that.  (Unfortunately I haven't got a ppc64 setup to
> > test it with yet, but ppc32 still works.)
>
> Not necessarily a code cleanup, but at least a patch which doesn't
> introduce useless changes. Anyway applied.

The original change was to eliminate an "unused variable pos" warning when the 
BSD code was #ifdefed out.

Yay applied.  Thanks,

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

[Qemu-devel] Re: Significant performance regression in qemu-system-mips.

[Paolo Bonzini]

On 03/24/2010 09:34 PM, Rob Landley wrote:
> Unfortunately, I can't revert that patch in current origin/master because most
> of the hunks fail...

Try this, or try disabling dynticks as a stopgap measure:

http://permalink.gmane.org/gmane.comp.emulators.qemu/65821

Paolo

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Aurelien Jarno]

On Wed, Mar 24, 2010 at 03:34:00PM -0500, Rob Landley wrote:
> I have a native build under qemu that gets killed if it doesn't produce a line 
> of output for 60 seconds (hang detection enforced by the host monitoring 
> qemu's stdout with --nographic, not from within qemu).
> 
> In the most recent release version, it never came close to triggering on mips 
> with a 30 second timeout.  In the current -git version (well, as of Thursday 
> anyway), it triggers frequently (about 90% of the time) even with a 60 second 
> timeout.
> 
> I bisected it to this:
> 
> commit 1828be316f6637d43dd4c4f5f32925b17fb8107f
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Mar 10 11:38:41 2010 +0100
> 
>     more alarm timer cleanup
>     
>     The timer_alarm_pending variable is related to the alarm timer but not
>     placed in the struct.  Also, in qemu_mod_timer the wrong flag was being
>     tested: the timer is rearmed in the alarm timer "bottom half", so the
>     right flag to test there is the "pending" flag.
>     
>     Finally, I hoisted the NULL checks from alarm_has_dynticks to
>     host_alarm_handler.
>     
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Reverting that patch fixed it (git show HEAD | patch -R -p1), by which I mean 
> three consecutive runs with 30 second timeout didn't trigger the hang 
> detection.
> 

I have tried to reproduce the issue by measuring the boot time of a mips
system, but it stay unchanged between before and after the patch. Do you
have some more details about how to reproduce the issue ? Have you tried
to pay with the -clock option?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Significant performance regression in qemu-system-mips.

[Rob Landley]

On Thursday 01 April 2010 16:33:09 Aurelien Jarno wrote:
> On Wed, Mar 24, 2010 at 03:34:00PM -0500, Rob Landley wrote:
> > Reverting that patch fixed it (git show HEAD | patch -R -p1), by which I
> > mean three consecutive runs with 30 second timeout didn't trigger the
> > hang detection.
>
> I have tried to reproduce the issue by measuring the boot time of a mips
> system, but it stay unchanged between before and after the patch.  Do you
> have some more details about how to reproduce the issue ? Have you tried
> to pay with the -clock option?

It was fixed by commit ca5a2a4b12bd447 from Paolo Bonzini, which is in current 
-git.  (I just confirmed that current -git builds fine.)

If you did want to reproduce the issue get and extract

  http://impactlinux.com/fwl/downloads/firmware-0.9.11.tar.bz2

Create a mips system image:

  ./build.sh mips

And when the noise dies down run the native build with:

  sources/native-builds/static-tools.sh temp.hdc
  sources/more/native-build.sh mips temp.hdc output 30

That creates a build control image (temp.hdc, with the source tarballs for 
dropbear and strace and a control script to build static binaries of both), 
and then runs a native build using the mips system image under qemu (it'll 
grab qemu-system-mips from your $PATH), uploading the results into the 
"output" directory on the host, with a 30 second inactivity timeout.

When the bug was manifesting, it usually wouldn't even complete with a 60 
second timeout, let alone 30.  (Paolo said disabling dynticks would help, but 
his patch seems to have fixed it properly.)

Thanks,

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds