"Martin Schleier" <drahemm...@gmx.net> writes: > e.g: > - correct use of some blackfin hi/lo macros. > - if certain data structures are declared as const > (struct seq_operations/file_operations) > - correct use of NR_CPUS is usually wrong > - complains about in_atomic() outside core kernel code > - warns about LINUX_VERSION_CODE, #if 0, > volatile or deprecated functions. > - informs about needless kfree/usb_free_urb checks > - etc...
Did the patch in question contain such problems? -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> "Martin Schleier" <drahemm...@gmx.net> writes: >On Fri, 6 Nov 2009 18:22:18 Alan Cox wrote: > > > If it wasn't riddled with 19 errors (not bad for only 133 lines), > > > I would have bothered to remove these irrelevant lines.
> > Checkpatch is just formatting - its just an aide nothing more. > > It's not remotely useful to bother with them for stuff that is > > basically sanely formatted until such point as someone is actually > > sure the patch is worth going into the tree.
> > the utility is called checkpatch and not checkstyle or > > checkformatting. > > And there's a good reason behind this decision, because it does > > more than just checking style.
> > e.g: > > - correct use of some blackfin hi/lo macros. > > - if certain data structures are declared as const > > (struct seq_operations/file_operations) > > - correct use of NR_CPUS is usually wrong > > - complains about in_atomic() outside core kernel code > > - warns about LINUX_VERSION_CODE, #if 0, > > volatile or deprecated functions. > > - informs about needless kfree/usb_free_urb checks > > - etc...
> > and I'm sure that future modifications will add more > >useful functionality _checks_ to many more _common pitfalls_ > >areas.
> Did the patch in question contain such problems?
the last point: - etc... =>
"WARNING: externs should be avoided in .c files #56: FILE: arch/x86/kernel/nopl_emu.c:13: +void do_invalid_op(struct pt_regs *regs, long error_code);" ? (or do you think that this is a formatting issue?!)
a grep will give you a header file where it is defined: "arch/x86/include/asm/traps.h" dotraplinkage void do_invalid_op(struct pt_regs *, long);
anyway, in case we get more followers here. I put your question back in context of the original response. Because this discussion-branch was not about arguing about nopl emulation, since - apparently - nothing was/is wrong with the code itself.
Instead, we ended up here because of:
Fri, 6 Nov 2009 15:59:37 Alan Cox wrote: "Secondly Ingo knows how to operate checkpatch and trivial style bits like that are irrelevant to meaningful discussion about code."
And this is clearly not the case. It is the job of a Submitter (as described in Documentations/SubmittingPatches section 4) to check and test his patches with tools like checkpatch or sparse before posting them.
After all this patch is going into /arch/x86 and not /drivers/staging and this sort of "extern declaration" is prone to break one day when void do_invalid_op(struct pt_regs *, long); declaration is modified. -- GRATIS für alle GMX-Mitglieder: Die maxdome Movie-FLAT! Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Matteo Croce <technobo...@gmail.com> wrote: > On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: > > On 11/06/2009 06:59 AM, Matteo Croce wrote: > >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov.
cmov is not i686 either. Its an optional extension that *should* only be used after you test its available. So gcc "i686" isn't quite "686". There are of course good reasons for that choice.
> yes, I did some test like gzip, bzip2, lame etc and they give more or less > the same results of dhrystone
How does that compare to i486. Certainly the old Nat Semi geode seemed to prefer to be fed i486 code to i586 (and wouldn't of course hack i686). You might also want to play around with -mtune= as well as arch= before assuming why i686 is a win
(I still btw think the patch is a good idea, it simplifies life enormously for users with the CPU)
"Martin Schleier" <drahemm...@gmx.net> writes: >> Did the patch in question contain such problems? > the last point: > - etc... =>
Yeah.
> "WARNING: externs should be avoided in .c files
Ironically, it's the only "WARNING" while the rest are "ERRORS". OTOH I personally believe all output from checkpatch should be labeled "WARNING"; it's not for checkpatch to decide. It's only a tool.
> #56: FILE: arch/x86/kernel/nopl_emu.c:13: > +void do_invalid_op(struct pt_regs *regs, long error_code);" ? > (or do you think that this is a formatting issue?!)
Actually, I think it wasn't any issue at all at this point, when it wasn't yet established if the patch makes sense at all.
> It is the job of a Submitter > (as described in Documentations/SubmittingPatches section 4) > to check and test his patches with tools like checkpatch or sparse > before posting them.
You apparently forgot what SubmittingPatches file is all about:
"This text is a collection of suggestions which can greatly increase the chances of your change being accepted."
You know, we don't have laws for everything here. And we're not androids specialized in producing C code. We are supposed to use some common sense first.
> After all this patch is going into /arch/x86 and not /drivers/staging > and this sort of "extern declaration" is prone to break one day when > void do_invalid_op(struct pt_regs *, long); declaration is modified.
That's true, though it's the same for "staging". -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Sat, Nov 7, 2009 at 1:05 AM, Martin Schleier <drahemm...@gmx.net> wrote: > Sat, 07 Nov 2009 00:05:12 Krzystof Halasa >> "Martin Schleier" <drahemm...@gmx.net> writes: >>On Fri, 6 Nov 2009 18:22:18 Alan Cox wrote: >> > > If it wasn't riddled with 19 errors (not bad for only 133 lines), >> > > I would have bothered to remove these irrelevant lines.
>> > Checkpatch is just formatting - its just an aide nothing more. >> > It's not remotely useful to bother with them for stuff that is >> > basically sanely formatted until such point as someone is actually >> > sure the patch is worth going into the tree.
>> > the utility is called checkpatch and not checkstyle or >> > checkformatting. >> > And there's a good reason behind this decision, because it does >> > more than just checking style.
>> > e.g: >> > - correct use of some blackfin hi/lo macros. >> > - if certain data structures are declared as const >> > (struct seq_operations/file_operations) >> > - correct use of NR_CPUS is usually wrong >> > - complains about in_atomic() outside core kernel code >> > - warns about LINUX_VERSION_CODE, #if 0, >> > volatile or deprecated functions. >> > - informs about needless kfree/usb_free_urb checks >> > - etc...
>> > and I'm sure that future modifications will add more >> >useful functionality _checks_ to many more _common pitfalls_ >> >areas.
>> Did the patch in question contain such problems? > the last point: > - etc... =>
> "WARNING: externs should be avoided in .c files > #56: FILE: arch/x86/kernel/nopl_emu.c:13: > +void do_invalid_op(struct pt_regs *regs, long error_code);" ? > (or do you think that this is a formatting issue?!)
> a grep will give you a header file where it is defined: > "arch/x86/include/asm/traps.h" > dotraplinkage void do_invalid_op(struct pt_regs *, long);
> anyway, in case we get more followers here. I put your question back > in context of the original response. Because this discussion-branch was > not about arguing about nopl emulation, since - apparently - nothing > was/is wrong with the code itself.
> Instead, we ended up here because of:
> Fri, 6 Nov 2009 15:59:37 Alan Cox wrote: > "Secondly Ingo knows how to operate checkpatch and trivial style bits like > that are irrelevant to meaningful discussion about code."
> And this is clearly not the case. It is the job of a Submitter > (as described in Documentations/SubmittingPatches section 4) > to check and test his patches with tools like checkpatch or sparse > before posting them.
> After all this patch is going into /arch/x86 and not /drivers/staging > and this sort of "extern declaration" is prone to break one day when > void do_invalid_op(struct pt_regs *, long); declaration is modified. > -- > GRATIS für alle GMX-Mitglieder: Die maxdome Movie-FLAT! > Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01
This one is perfect according to checkpatch.pl
The AMD Geode LX has an x86 id of 5 (i586) tought it's technically an i686:
root@alix:~# egrep '^(cpu family|model name|flags)' /proc/cpuinfo cpu family : 5 model name : Geode(TM) Integrated Processor by AMD PCS flags : fpu de pse tsc msr cx8 sep pge cmov clflush mmx mmxext 3dnowext 3dnow
indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL). This patch adds a quirck to promote the Geode to an i686 and emulates the NOPL in the do_invalid_op trap, so the userspace never notices. Emulating the NOPL has minimum performance loss, emulating a NOPL takes 0.5 usecs and they are rarely used in x86
> "Martin Schleier" <drahemm...@gmx.net> writes: > >> Did the patch in question contain such problems? > > the last point: > > - etc... =>
> Yeah.
> > "WARNING: externs should be avoided in .c files
> Ironically, it's the only "WARNING" while the rest are "ERRORS". > OTOH I personally believe all output from checkpatch should be labeled > "WARNING"; it's not for checkpatch to decide. It's only a tool.
Ironically, I assumed that these matters are taken somewhat serious and everything would be fine after the respin...
But instead, all everyone (except the submitter) is barking whenever checkpatch.pl is irrelevant - because apparently it only catches formatting errors -
(BTW: I think this message should be an ERROR, because it can really break Randy Dulap's massive kernel compile tests)
> > #56: FILE: arch/x86/kernel/nopl_emu.c:13: > > +void do_invalid_op(struct pt_regs *regs, long error_code);" ? > > (or do you think that this is a formatting issue?!)
> Actually, I think it wasn't any issue at all at this point, when it > wasn't yet established if the patch makes sense at all.
here's the quote from on which the comment was based: | On Sat, Oct 3, 2009 at 8:21 AM, Ingo Molnar <mi...@elte.hu> wrote: | | Looks good, but your signoff line is missing. | | Ingo
now tell me: What is the word he was using to say that the idea needs _rethinking_ and that he's declined to merge the patch in the foreseeable future because of these shortcomings? I can't see them, but I would be delighted if you can point them out to me.
The discussion whenever this feature make sense has taken place _a bit_ earlier in the thread with a _positive_ result. (if you look at the date: the thread started over a month ago: http://lkml.org/lkml/2009/10/2/464 )
so I'm not sure if everyone was aware of this, since this might explain the _differences_.
> > It is the job of a Submitter > > (as described in Documentations/SubmittingPatches section 4) > > to check and test his patches with tools like checkpatch or sparse > > before posting them.
> You apparently forgot what SubmittingPatches file is all about:
> "This text is a collection of suggestions which can greatly increase the > chances of your change being accepted." > You know, we don't have laws for everything here. > And we're not androids specialized in producing C code. > We are supposed to use some common sense first.
Ahh common sense, so checking & testing your work before submitting it not _common sense_ anymore?
Surely it's hard for anyone new to know about this before hitting "send". But so far everyone has stumbled across this. :\
But back to the topic about laws. What about: "12) Sign your work" in the same SubmittingPatches file? Have you noticed that only SubmittingPatches talks about the signed-off-by? And we all know that every patch has to have one. So Clearly SubmittingPatches really contains LAWs for how to do things.
The only question is if "4) Style check your changes." is a guideline or a _more_... And there's a paragraph in Documentation/SubmitChecklist:"
5: Check your patch for general style as detailed in Documentation/CodingStyle. Check for trivial violations with the patch style checker prior to submission (scripts/checkpatch.pl). [BOLD] You should be able to justify all violations that remain in your patch. [BOLD]"
Andy Whitcroft <a...@shadowen.org> clearly wrote that down. (And there's no point in arguing about checkpatch.pl when you have to JUSTIFY ALL REMAINING VIOLATIONS, or more to the point: FIX THEM INSTEAD.)
And now my - head hurts - we need a lawyer to answer if this IS or IS NOT a law before we can bang on with this.
And yes Documentation/SubmitChecklist also has the same _header_: "Here are some basic things that developers should do if they want to see their kernel patch submissions accepted more quickly."
I know about that and I agree: time is always an issue. This cycle is already @ -rc6 (rc5) and given that the debating started over a month ago it's really time to get cracking... Thankfully v3 is already available, and even better: fixed :-).
> > After all this patch is going into /arch/x86 and not /drivers/staging > > and this sort of "extern declaration" is prone to break one day when > > void do_invalid_op(struct pt_regs *, long); declaration is modified.
> That's true, though it's the same for "staging".
AFAIK the only rule for staging is: it must compile (somehow). but I'm sure we can ask greg here if there are uncertainties.
"Martin Schleier" <drahemm...@gmx.net> writes: > And now my - head hurts - we need a lawyer to answer if this > IS or IS NOT a law before we can bang on with this.
Good luck. I have no more questions. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > +++ b/arch/x86/kernel/nopl_emu.c 2009-11-07 11:59:17.667748571 +0100 > @@ -0,0 +1,103 @@ > +/* > + * linux/arch/x86/kernel/nopl_emu.c > + * > + * Copyright (C) 2002 Willy Tarreau > + * Copyright (C) 2009 Matteo Croce > + */ > + > +#include <linux/linkage.h> > +#include <asm/math_emu.h> > +#include <asm/traps.h> > + > +/* This code can be used to allow the AMD Geode to hopefully correctly execute > + * some code which was originally compiled for an i686, by emulating NOPL, > + * the only missing i686 instruction in the CPU > + * > + * Willy Tarreau <wi...@meta-x.org> > + * Matteo Croce <technobo...@gmail.com> > + */ > +
If we're doing to introduce a missed-instruction interpreter (which is what this is) in the kernel, it needs to handle all the subtleties of x86 execution correctly; in particular I believe it needs to check the code segment limits, permissions, and mode. Things it doesn't understand it can SIGILL (or, if more appropriate, SIGSEGV) on, of course.
Personally I think the easiest is to verify that the code segment is flat 32 bits or even more specifically CS == USER_CS, and SIGILL otherwise.
-hpa
-- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
> if (c->x86_model == 10) { > - /* AMD Geode LX is model 10 */ > - /* placeholder for any needed mods */ > + /* Geode only lacks the NOPL instruction to be i686, > + but we can emulate it in the exception handler > + and promote it to a class 6 cpu */ > + boot_cpu_data.x86 = 6; > return; > }
If you're going to update this, you also need to make sure that you're not breaking things that check it. For example, arch/x86/include/asm/geode.h has an is_geode_lx check that expects boot_cpu_data.x86 to be 5. Please be sure to update all these places when creating a patch like this.
> The AMD Geode LX has an x86 id of 5 (i586) tought it's technically an i686:
It is not.
> root@alix:~# egrep '^(cpu family|model name|flags)' /proc/cpuinfo > cpu family : 5 > model name : Geode(TM) Integrated Processor by AMD PCS > flags : fpu de pse tsc msr cx8 sep pge cmov clflush mmx > mmxext 3dnowext 3dnow
> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL). > This patch adds a quirck to promote the Geode to an i686 and emulates > the NOPL in the do_invalid_op trap, so the userspace never notices. > Emulating the NOPL has minimum performance loss, emulating a NOPL > takes 0.5 usecs > and they are rarely used in x86
> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: > > On 11/06/2009 06:59 AM, Matteo Croce wrote: > >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. > > I'm somewhat wondering about the general value of this patch; is i686 > > code really that much faster on Geode that it's worth it?
> > ? ? ? ?-hpa
> > -- > > H. Peter Anvin, Intel Open Source Technology Center > > I work for Intel. ?I don't speak on their behalf.
> yes, I did some test like gzip, bzip2, lame etc and they give more or less > the same results of dhrystone
> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c > Microseconds for one run through Dhrystone: 1.4 > Dhrystones per Second: 740741 .. > root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c > Trying 5000000 runs through Dhrystone: > Microseconds for one run through Dhrystone: 1.2 > Dhrystones per Second: 841751
Teach gcc that geodelx exists? No need to break kernel for that... and you probably can gain even bigger gains. Pavel
On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
>> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. >> > I'm somewhat wondering about the general value of this patch; is i686 >> > code really that much faster on Geode that it's worth it?
>> > ? ? ? ?-hpa
>> > -- >> > H. Peter Anvin, Intel Open Source Technology Center >> > I work for Intel. ?I don't speak on their behalf.
>> yes, I did some test like gzip, bzip2, lame etc and they give more or less >> the same results of dhrystone
>> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c >> Microseconds for one run through Dhrystone: 1.4 >> Dhrystones per Second: 740741 > ... >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c >> Trying 5000000 runs through Dhrystone: >> Microseconds for one run through Dhrystone: 1.2 >> Dhrystones per Second: 841751
> Teach gcc that geodelx exists? No need to break kernel for that... and > you probably can gain even bigger gains. > Pavel
Gcc 4.4 already knows about it, just sucks at optimizing:
Dhrystone Benchmark, Version C, Version 2.2 Program compiled without 'register' attribute Using times(), HZ=100
Trying 5000000 runs through Dhrystone: Microseconds for one run through Dhrystone: 1.4 Dhrystones per Second: 719424 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon <dilin...@collabora.co.uk> wrote: > See comment below. BTW, how does this affect performance on LXs? > Do you have any hard numbers for common tasks?
> On Sat, 7 Nov 2009 12:11:55 +0100 > Matteo Croce <technobo...@gmail.com> wrote: > [...]
>> if (c->x86_model == 10) { >> - /* AMD Geode LX is model 10 */ >> - /* placeholder for any needed mods */ >> + /* Geode only lacks the NOPL instruction to be i686, >> + but we can emulate it in the exception handler >> + and promote it to a class 6 cpu */ >> + boot_cpu_data.x86 = 6; >> return; >> }
> If you're going to update this, you also need to make sure that you're > not breaking things that check it. For example, > arch/x86/include/asm/geode.h has an is_geode_lx check that expects > boot_cpu_data.x86 to be 5. Please be sure to update all these places > when creating a patch like this.
True, but also remove the duplicate function is_geode in the NAND driver and use the identical one defined in geode.h:
-static int is_geode(void) -{ - /* These are the CPUs which will have a CS553[56] companion chip */ - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && - boot_cpu_data.x86 == 5 && - boot_cpu_data.x86_model == 10) - return 1; /* Geode LX */ - - if ((boot_cpu_data.x86_vendor == X86_VENDOR_NSC || - boot_cpu_data.x86_vendor == X86_VENDOR_CYRIX) && - boot_cpu_data.x86 == 5 && - boot_cpu_data.x86_model == 5) - return 1; /* Geode GX (née GX2) */ - - return 0; -} -
#ifdef CONFIG_MTD_PARTITIONS static const char *part_probes[] = { "cmdlinepart", NULL }; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: > > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: > >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: > >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: > >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. > >> > I'm somewhat wondering about the general value of this patch; is i686 > >> > code really that much faster on Geode that it's worth it?
> >> > ? ? ? ?-hpa
> >> > -- > >> > H. Peter Anvin, Intel Open Source Technology Center > >> > I work for Intel. ?I don't speak on their behalf.
> >> yes, I did some test like gzip, bzip2, lame etc and they give more or less > >> the same results of dhrystone
> >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c > >> Microseconds for one run through Dhrystone: 1.4 > >> Dhrystones per Second: 740741 > > ... > >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c > >> Trying 5000000 runs through Dhrystone: > >> Microseconds for one run through Dhrystone: 1.2 > >> Dhrystones per Second: 841751
> > Teach gcc that geodelx exists? No need to break kernel for that... and > > you probably can gain even bigger gains.
> Gcc 4.4 already knows about it, just sucks at optimizing:
Good. So there's really no point in breaking kernel.
On Sun, Nov 8, 2009 at 7:10 PM, Pavel Machek <pa...@ucw.cz> wrote: > On Sun 2009-11-08 18:40:06, Matteo Croce wrote: >> On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: >> > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: >> >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: >> >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: >> >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
>> >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. >> >> > I'm somewhat wondering about the general value of this patch; is i686 >> >> > code really that much faster on Geode that it's worth it?
>> >> > ? ? ? ?-hpa
>> >> > -- >> >> > H. Peter Anvin, Intel Open Source Technology Center >> >> > I work for Intel. ?I don't speak on their behalf.
>> >> yes, I did some test like gzip, bzip2, lame etc and they give more or less >> >> the same results of dhrystone
>> >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c >> >> Microseconds for one run through Dhrystone: 1.4 >> >> Dhrystones per Second: 740741 >> > ... >> >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c >> >> Trying 5000000 runs through Dhrystone: >> >> Microseconds for one run through Dhrystone: 1.2 >> >> Dhrystones per Second: 841751
>> > Teach gcc that geodelx exists? No need to break kernel for that... and >> > you probably can gain even bigger gains.
>> Gcc 4.4 already knows about it, just sucks at optimizing:
> Good. So there's really no point in breaking kernel.
This is on my TODO list, but better to discuss it in the GCC mailing list, the kernel should use -march=geode and GCC should generate the best code for the AMD Geode, actually the i686 one -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon <dilin...@collabora.co.uk> wrote: > See comment below. BTW, how does this affect performance on LXs? > Do you have any hard numbers for common tasks?
> On Sat, 7 Nov 2009 12:11:55 +0100 > Matteo Croce <technobo...@gmail.com> wrote: > [...]
>> if (c->x86_model == 10) { >> - /* AMD Geode LX is model 10 */ >> - /* placeholder for any needed mods */ >> + /* Geode only lacks the NOPL instruction to be i686, >> + but we can emulate it in the exception handler >> + and promote it to a class 6 cpu */ >> + boot_cpu_data.x86 = 6; >> return; >> }
> If you're going to update this, you also need to make sure that you're > not breaking things that check it. For example, > arch/x86/include/asm/geode.h has an is_geode_lx check that expects > boot_cpu_data.x86 to be 5. Please be sure to update all these places > when creating a patch like this.
Right, but what if is_geode_lx() is called befor the x86.id change takes effect? Maybe something like this?
--- a/arch/x86/include/asm/geode.h 2009-11-08 19:13:43.531117343 +0100 +++ b/arch/x86/include/asm/geode.h 2009-11-08 19:19:42.130618023 +0100 @@ -177,7 +177,7 @@ static inline int is_geode_lx(void) { return ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && - (boot_cpu_data.x86 == 5) && + (boot_cpu_data.x86 == 5 || boot_cpu_data.x86 == 6) && (boot_cpu_data.x86_model == 10)); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote:
> On 11/06/2009 06:59 AM, Matteo Croce wrote: >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. > I'm somewhat wondering about the general value of this patch; is i686 > code really that much faster on Geode that it's worth it?
> -hpa
> -- > H. Peter Anvin, Intel Open Source Technology Center > I work for Intel. I don't speak on their behalf.
There is a small advantage, but considering that GCC isn't much geode aware yet there is stil room for improvement IMHO:
root@alix:/usr/src/dist# ll totale 257M -rwxr-xr-x 1 1000 src 93K 8 nov 2009 bzip2-i586 -rwxr-xr-x 1 1000 src 93K 8 nov 2009 bzip2-i686 -rwxr-xr-x 1 1000 src 60K 8 nov 2009 gzip-i586 -rwxr-xr-x 1 1000 src 60K 8 nov 2009 gzip-i686 -rw-r--r-- 1 1000 src 256M 8 nov 2009 linux-2.6.31.5.tar -rwxr-xr-x 1 1000 src 90K 8 nov 2009 lzma-i586 -rwxr-xr-x 1 1000 src 94K 8 nov 2009 lzma-i686 root@alix:/usr/src/dist# time cat linux-2.6.31.5.tar >/dev/null
real 0m10.168s user 0m0.030s sys 0m1.390s root@alix:/usr/src/dist# time ./gzip-i586 -9 < linux-2.6.31.5.tar >/dev/null
real 5m22.331s user 5m10.820s sys 0m11.170s root@alix:/usr/src/dist# time ./gzip-i686 -9 < linux-2.6.31.5.tar >/dev/null
real 5m3.737s user 4m51.880s sys 0m11.510s root@alix:/usr/src/dist# time ./bzip2-i586 -9 < linux-2.6.31.5.tar >/dev/null
real 9m16.539s user 9m4.410s sys 0m11.760s root@alix:/usr/src/dist# time ./bzip2-i686 -9 < linux-2.6.31.5.tar >/dev/null
real 8m48.682s user 8m34.950s sys 0m13.260s -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
I think the nand driver needs a bit more love than this. The cs553x is available for non-geode platforms, so a cs553x driver should not be checking for the existence of a specific CPU. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Matteo Croce <technobo...@gmail.com> wrote: > On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon > <dilin...@collabora.co.uk> wrote: > > See comment below. BTW, how does this affect performance on LXs? > > Do you have any hard numbers for common tasks?
> > On Sat, 7 Nov 2009 12:11:55 +0100 > > Matteo Croce <technobo...@gmail.com> wrote: > > [...]
> >> if (c->x86_model == 10) { > >> - /* AMD Geode LX is model 10 */ > >> - /* placeholder for any needed mods */ > >> + /* Geode only lacks the NOPL instruction to be i686, > >> + but we can emulate it in the exception handler > >> + and promote it to a class 6 cpu */ > >> + boot_cpu_data.x86 = 6; > >> return; > >> }
> > If you're going to update this, you also need to make sure that > > you're not breaking things that check it. For example, > > arch/x86/include/asm/geode.h has an is_geode_lx check that expects > > boot_cpu_data.x86 to be 5. Please be sure to update all these > > places when creating a patch like this.
> Right, but what if is_geode_lx() is called befor the x86.id change > takes effect? Maybe something like this?
> > On Sun 2009-11-08 18:40:06, Matteo Croce wrote: > > > On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: > > > > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: > > > >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: > > > >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: > > > >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> > > >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. > > > >> > I'm somewhat wondering about the general value of this patch; is i686 > > > >> > code really that much faster on Geode that it's worth it?
> > > >> > ? ? ? ?-hpa
> > > >> > -- > > > >> > H. Peter Anvin, Intel Open Source Technology Center > > > >> > I work for Intel. ?I don't speak on their behalf.
> > > >> yes, I did some test like gzip, bzip2, lame etc and they give more or less > > > >> the same results of dhrystone
> > > >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c > > > >> Microseconds for one run through Dhrystone: 1.4 > > > >> Dhrystones per Second: 740741 > > > > ... > > > >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c > > > >> Trying 5000000 runs through Dhrystone: > > > >> Microseconds for one run through Dhrystone: 1.2 > > > >> Dhrystones per Second: 841751
> > > > Teach gcc that geodelx exists? No need to break kernel for that... and > > > > you probably can gain even bigger gains.
> But no standard distribution will be made available in a geode special > version - not enough machines in the marekt. So I think it is better to > be able to use the i686 specific things they already support, like > libc6-686 from debian for example.
So hack your distribution to use libc6-686 if you know that it is safe... (that is no NOPL usage there). Still no need to break /proc/cpuinfo. Pavel
On Sun, 8 Nov 2009, Pavel Machek wrote: > On Sun 2009-11-08 18:40:06, Matteo Croce wrote: > > On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: > > > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: > > >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: > > >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: > > >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
> > >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. > > >> > I'm somewhat wondering about the general value of this patch; is i686 > > >> > code really that much faster on Geode that it's worth it?
> > >> > ? ? ? ?-hpa
> > >> > -- > > >> > H. Peter Anvin, Intel Open Source Technology Center > > >> > I work for Intel. ?I don't speak on their behalf.
> > >> yes, I did some test like gzip, bzip2, lame etc and they give more or less > > >> the same results of dhrystone
> > >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c > > >> Microseconds for one run through Dhrystone: 1.4 > > >> Dhrystones per Second: 740741 > > > ... > > >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c > > >> Trying 5000000 runs through Dhrystone: > > >> Microseconds for one run through Dhrystone: 1.2 > > >> Dhrystones per Second: 841751
> > > Teach gcc that geodelx exists? No need to break kernel for that... and > > > you probably can gain even bigger gains.
But no standard distribution will be made available in a geode special version - not enough machines in the marekt. So I think it is better to be able to use the i686 specific things they already support, like libc6-686 from debian for example.
On Sun, Nov 8, 2009 at 8:29 PM, Sven-Haegar Koch <hae...@sdinet.de> wrote: > On Sun, 8 Nov 2009, Pavel Machek wrote:
>> On Sun 2009-11-08 18:40:06, Matteo Croce wrote: >> > On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: >> > > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: >> > >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: >> > >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: >> > >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
>> > >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. >> > >> > I'm somewhat wondering about the general value of this patch; is i686 >> > >> > code really that much faster on Geode that it's worth it?
>> > >> > ? ? ? ?-hpa
>> > >> > -- >> > >> > H. Peter Anvin, Intel Open Source Technology Center >> > >> > I work for Intel. ?I don't speak on their behalf.
>> > >> yes, I did some test like gzip, bzip2, lame etc and they give more or less >> > >> the same results of dhrystone
>> > >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c >> > >> Microseconds for one run through Dhrystone: 1.4 >> > >> Dhrystones per Second: 740741 >> > > ... >> > >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c >> > >> Trying 5000000 runs through Dhrystone: >> > >> Microseconds for one run through Dhrystone: 1.2 >> > >> Dhrystones per Second: 841751
>> > > Teach gcc that geodelx exists? No need to break kernel for that... and >> > > you probably can gain even bigger gains.
> But no standard distribution will be made available in a geode special > version - not enough machines in the marekt. So I think it is better to > be able to use the i686 specific things they already support, like > libc6-686 from debian for example.
On Sun, Nov 8, 2009 at 8:36 PM, Pavel Machek <pa...@ucw.cz> wrote: > On Sun 2009-11-08 20:29:55, Sven-Haegar Koch wrote: >> On Sun, 8 Nov 2009, Pavel Machek wrote:
>> > On Sun 2009-11-08 18:40:06, Matteo Croce wrote: >> > > On Sun, Nov 8, 2009 at 6:37 PM, Pavel Machek <pa...@ucw.cz> wrote: >> > > > On Fri 2009-11-06 23:18:06, Matteo Croce wrote: >> > > >> On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <h...@zytor.com> wrote: >> > > >> > On 11/06/2009 06:59 AM, Matteo Croce wrote: >> > > >> >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL).
>> > > >> > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. >> > > >> > I'm somewhat wondering about the general value of this patch; is i686 >> > > >> > code really that much faster on Geode that it's worth it?
>> > > >> > ? ? ? ?-hpa
>> > > >> > -- >> > > >> > H. Peter Anvin, Intel Open Source Technology Center >> > > >> > I work for Intel. ?I don't speak on their behalf.
>> > > >> yes, I did some test like gzip, bzip2, lame etc and they give more or less >> > > >> the same results of dhrystone
>> > > >> root@alix:/usr/src# CFLAGS='-march=i586' ./dry.c >> > > >> Microseconds for one run through Dhrystone: 1.4 >> > > >> Dhrystones per Second: 740741 >> > > > ... >> > > >> root@alix:/usr/src# CFLAGS='-march=i686' ./dry.c >> > > >> Trying 5000000 runs through Dhrystone: >> > > >> Microseconds for one run through Dhrystone: 1.2 >> > > >> Dhrystones per Second: 841751
>> > > > Teach gcc that geodelx exists? No need to break kernel for that... and >> > > > you probably can gain even bigger gains.
>> But no standard distribution will be made available in a geode special >> version - not enough machines in the marekt. So I think it is better to >> be able to use the i686 specific things they already support, like >> libc6-686 from debian for example.
> So hack your distribution to use libc6-686 if you know that it is > safe... (that is no NOPL usage there). Still no need to break > /proc/cpuinfo. > Pavel
Better to be sure that a NOPL whouldn't SIGILL your program, isn't it? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> >> > > Teach gcc that geodelx exists? No need to break kernel for that... and > >> > > you probably can gain even bigger gains.
> > But no standard distribution will be made available in a geode special > > version - not enough machines in the marekt. So I think it is better to > > be able to use the i686 specific things they already support, like > > libc6-686 from debian for example.
> That's exactly the patch point
Not quite.
Your cpu is not 686; stop trying to pretend it is.
Now, maybe it is good idea to run libc6-686 on it. If you can guarantee it contains no NOPL, fix whatever code is responsible for selecting libc to use libc6-686 to use that. (Bonus points for renaming libc6-686 to something more suitable. libc6-cmov?)
> >> But no standard distribution will be made available in a geode special > >> version - not enough machines in the marekt. So I think it is better to > >> be able to use the i686 specific things they already support, like > >> libc6-686 from debian for example.
> > So hack your distribution to use libc6-686 if you know that it is > > safe... (that is no NOPL usage there). Still no need to break > > /proc/cpuinfo.
> Better to be sure that a NOPL whouldn't SIGILL your program, isn't > it?
SIGILL is easier to debug than NOPL mysteriously taking 100x time it should, sorry. Pavel
|
