Significant performance regression in qemu-system-mips.

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>


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

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
>

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