Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> >>>That's a non-issue.  300 bytes matters a lot on some systems.  The
> >>>fact that there are drivers that are bloated is nothing to do with
> >>>it.
> >>>
> >>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
> >>alignment to all the cache friendly structure padding.
> >>
> >
> >My intention is that we won't have done 128 byte alignments just by
> >'supporting' Athlons, only if we want to run fast on Athlons.  A
> >distribution kernel that is intended to boot on all CPUs needs
> >workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
> >
> >Obviously using such a kernel for anything other than getting a system
> >up and running to compile a better kernel is a Bad Thing, but the
> >distributions could supply separate Athlon, PIV, and 386 _optimised_
> >kernels.
> >
>
> Why bother with that complexity? Just use 128 byte lines. This allows
> a decent generic kernel. The people who have space requirements would
> only compile what they need anyway.

So, basically, if you compile a kernel for a 386, but think that maybe
one day you might need to run it on an Athlon for debugging purposes,
you use 128 byte padding, because it's not too bad on the 386?  Seems
pretty wasteful to me when the obvious, simple, elegant solution is to
allow independent selection of workaround inclusion and optimisation.
Especially since half of the work has already been done.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

John Bradford wrote:

>>>>>That's a non-issue.  300 bytes matters a lot on some systems.  The
>>>>>fact that there are drivers that are bloated is nothing to do with
>>>>>it.
>>>>>
>>>>>
>>>>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
>>>>alignment to all the cache friendly structure padding.
>>>>
>>>>
>>>My intention is that we won't have done 128 byte alignments just by
>>>'supporting' Athlons, only if we want to run fast on Athlons.  A
>>>distribution kernel that is intended to boot on all CPUs needs
>>>workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
>>>
>>>Obviously using such a kernel for anything other than getting a system
>>>up and running to compile a better kernel is a Bad Thing, but the
>>>distributions could supply separate Athlon, PIV, and 386 _optimised_
>>>kernels.
>>>
>>>
>>Why bother with that complexity? Just use 128 byte lines. This allows
>>a decent generic kernel. The people who have space requirements would
>>only compile what they need anyway.
>>
>
>So, basically, if you compile a kernel for a 386, but think that maybe
>one day you might need to run it on an Athlon for debugging purposes,
>you use 128 byte padding, because it's not too bad on the 386?  Seems
>pretty wasteful to me when the obvious, simple, elegant solution is to
>allow independent selection of workaround inclusion and optimisation.
>Especially since half of the work has already been done.
>

I missed the "simple, elegant" part. Conceptually elegant maybe.

If you mean to use the optimise option only to set cache line size, then
that might be a bit saner.

As far as the case study goes though: if you were worried about being
wasteful, why wouldn't you compile just for the 386 and debug from that?

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Thu, 18 Sep 2003, Pavel Machek wrote:

> > I think Alan brought up a very good point. Even if you
> > use a generic kernel that avoids prefetch use on Athlon
> > (which I am opposed to), it doesn't solve the problem
> > of user space programs detecting that the ISA supports
> > prefetch and using prefetch instructions and hitting the
> > errata on Athlon.
> > 
> > The user space problem worries me more, because the expectation
> > is that if CPUID says the program can use perfetch, it could
> > and should regardless of what the kernel decided to do here.
> 
> User programs should not rely on cpuid; they should read /proc/cpuinfo
> exactly because this kind of errata.

That's fine, but impacts portability. I don't think there is a right
way, since the o/s may not know about some features and the CPU may be
optimistic. Unless they are "recent Linux only" programs they may well
check the CPU itself, there are reasons for it in portable code.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Dave Jones wrote:
>  > they should read /proc/cpuinfo exactly because this kind of errata.
> 
> We don't do any change to /proc/cpuinfo in this case. As I mentioned
> in an earlier mail, the only way you could disable prefetch in this way
> would be by reporting the entire sse/3dnow instruction sets as unavailable
> which is overkill.

If you're running a *specifically non-AMD* kernel on an Athlon (that's
what this strand of the thread is about), is it a big deal to report
sse/3dnow as unavailable?  Presumably if you want sse/3dnow to be
reported on AMDs, you can just run a generic or AMD-enabled kernel.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Dave Jones]

On Thu, Sep 18, 2003 at 09:43:31AM +0200, Pavel Machek wrote:

 > > The user space problem worries me more, because the expectation
 > > is that if CPUID says the program can use perfetch, it could
 > > and should regardless of what the kernel decided to do here.
 > 
 > User programs should not rely on cpuid;

Depends. /dev/cpu/x/cpuid is preferred over cpuinfo, as it remains
constant across kernel versions.

 > they should read /proc/cpuinfo exactly because this kind of errata.

We don't do any change to /proc/cpuinfo in this case. As I mentioned
in an earlier mail, the only way you could disable prefetch in this way
would be by reporting the entire sse/3dnow instruction sets as unavailable
which is overkill.

With Andi's fix, userspace programs doing prefetch get fixed up
transparently, making this a non-issue.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Pavel Machek]

Hi!

> I think Alan brought up a very good point. Even if you
> use a generic kernel that avoids prefetch use on Athlon
> (which I am opposed to), it doesn't solve the problem
> of user space programs detecting that the ISA supports
> prefetch and using prefetch instructions and hitting the
> errata on Athlon.
> 
> The user space problem worries me more, because the expectation
> is that if CPUID says the program can use perfetch, it could
> and should regardless of what the kernel decided to do here.

User programs should not rely on cpuid; they should read /proc/cpuinfo
exactly because this kind of errata.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Bill Davidsen wrote:
> > prefetch isn't a cpuid feature flag. The only way you could do
> > what you suggest is by removing '3dnow' or 'sse', which cripples
> > things more than necessary.
> 
> Good point, and even if it were a separate feature, any code which was
> clever enough to use prefetch is likely to check the CPU bits rather
> than the /proc anyway. That's a guess, I suspect most programs do
> whatever gcc/glibc choose.

As I pointed out, other programs _especially_ glibc will need to check
for the AMD errata anyway, because of older kernels.

> If the fixup were not in place, would it be useful to emit a warning
> like "you have booted a non-Athlon kernel on an Athlon process, user
> programs may get unexpected page faults."

I agree, that's a good idea.  It will fit nicely alongside the
messages for broken WP bit etc.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Timothy Miller wrote:
> How many bytes would that code require?

Once the kernel has booted, zero.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Bill Davidsen wrote:
> I'm not sure which way you're going here, those have their own config
> entries, does that mean that you are advocating just making the Athlon
> fixup a config option the user must set, or that all of these should be
> on by default and appear in the delete-features menu, and/or be
> controlled by my proposed option to build only for the target CPU. Or
> was this just an informational comment?
> 
> I'm not looking to disagree with any of those suggestions, if you were
> making one.

I'm suggesting the prefetch workaround be on by default, and also the
option would only by visible if someone selects the new "compile only
for cpu XXX" feature.

There's no danger in turning the option off, as long as the init code
always detects AMD prefetch, and turns off in-kernel prefetching if
the workaround isn't configured in.  That's just a performance hit.

We already have a mechanism for removing prefetch instructions when
SSE isn't present, and should be used to handle this too.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Tue, 16 Sep 2003, Timothy Miller wrote:

> 
> 
> Bill Davidsen wrote:
> 
> > 
> > If the fixup were not in place, would it be useful to emit a warning
> > like "you have booted a non-Athlon kernel on an Athlon process, user
> > programs may get unexpected page faults." That's in init code, hopefully
> > there is no critical size issue there, I assume, other than how large a
> > kernel can be booted by the boot loader.
> > 
> 
> How many bytes would that code require?

No resident bytes, as noted it's in init and will be released at the end
of boot.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Timothy Miller]

Bill Davidsen wrote:

> 
> If the fixup were not in place, would it be useful to emit a warning
> like "you have booted a non-Athlon kernel on an Athlon process, user
> programs may get unexpected page faults." That's in init code, hopefully
> there is no critical size issue there, I assume, other than how large a
> kernel can be booted by the boot loader.
> 

How many bytes would that code require?

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

Bill Davidsen wrote:

>On Mon, 15 Sep 2003, Nick Piggin wrote:
>
>
>>
>>John Bradford wrote:
>>
>
>>I guess more specialised users would be able to edit the cache line
>>size themselves. I wouldn't be adverse to a kconfig setting under the
>>embedded menu though.
>>
>
>Clearly something which might be useful for some embedded CPUs.
>
>
>>I can see an argument for cache line size but thats about it. I can't
>>think of my optimisations that should be done on one architecture but
>>not another.
>>
>
>Well, if you meant "any optimizations" there are lots, and they already
>have config entries. Ex: F.P. emulation, RZ1000 fixups, etc.
>
>
>>No I definitely agree. Except sometimes you'll have to check at runtime:
>>a kernel compiled for all cpus for example needs Andi's Athlon prefetch
>>scheme. You'd really want some good helpers to either do runtime check
>>or always, or never. Something like this optimises down OK if cpu and
>>archmask are constant.
>>
>>static inline void ifcpu(const int cpumask, void (*func)(void))
>>{
>>        if ((cpumask&archmask) && ((~cpumask)&archmask)) {
>>                if (cpumask&current_cpu)
>>                        func();
>>        } else if (cpumask&archmask) {
>>                func();
>>        }
>>}
>>
>>ifcpu(K7||K8, &prefetch_workaround);
>>
>>You then need to get kconfig to generate cpu and archmask nicely.
>>You obviously still need to ifdef your prefetch_workaround to get the
>>space saving.
>>
>
>True, there's no way to get a minimal kernel without at least some
>ifdef'iness, although if you defined the code as an inline function and
>used your code above... a matter of style, a few preprocessor lines in
>that much code aren't going to be confusing.
>

I don't think ifdefing around functions is that bad for this sort of
workaround code. You have to take the address of the function with my
above code, so you can't use an inline one. I don't think the
preprocessor can solve the problem either because it seems you'd need
conditional compilation directives within a macro. I'm not a C expert
though so someone might have a way to do it.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Nick Piggin wrote:

> 
> 
> John Bradford wrote:

> I guess more specialised users would be able to edit the cache line
> size themselves. I wouldn't be adverse to a kconfig setting under the
> embedded menu though.

Clearly something which might be useful for some embedded CPUs.

> I can see an argument for cache line size but thats about it. I can't
> think of my optimisations that should be done on one architecture but
> not another.

Well, if you meant "any optimizations" there are lots, and they already
have config entries. Ex: F.P. emulation, RZ1000 fixups, etc.

> No I definitely agree. Except sometimes you'll have to check at runtime:
> a kernel compiled for all cpus for example needs Andi's Athlon prefetch
> scheme. You'd really want some good helpers to either do runtime check
> or always, or never. Something like this optimises down OK if cpu and
> archmask are constant.
> 
> static inline void ifcpu(const int cpumask, void (*func)(void))
> {
>         if ((cpumask&archmask) && ((~cpumask)&archmask)) {
>                 if (cpumask&current_cpu)
>                         func();
>         } else if (cpumask&archmask) {
>                 func();
>         }
> }
>
> ifcpu(K7||K8, &prefetch_workaround);
> 
> You then need to get kconfig to generate cpu and archmask nicely.
> You obviously still need to ifdef your prefetch_workaround to get the
> space saving.

True, there's no way to get a minimal kernel without at least some
ifdef'iness, although if you defined the code as an inline function and
used your code above... a matter of style, a few preprocessor lines in
that much code aren't going to be confusing.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Tue, 16 Sep 2003, Dave Jones wrote:

> On Tue, Sep 16, 2003 at 12:46:36PM +0100, Jamie Lokier wrote:
> 
>  > > The user space problem worries me more, because the expectation
>  > > is that if CPUID says the program can use perfetch, it could
>  > > and should regardless of what the kernel decided to do here.
>  > 
>  > If the workaround isn't compiled in, "prefetch" should be removed from
>  > /proc/cpuinfo on the buggy chips.
> 
> prefetch isn't a cpuid feature flag. The only way you could do
> what you suggest is by removing '3dnow' or 'sse', which cripples
> things more than necessary.

Good point, and even if it were a separate feature, any code which was
clever enough to use prefetch is likely to check the CPU bits rather
than the /proc anyway. That's a guess, I suspect most programs do
whatever gcc/glibc choose.

If the fixup were not in place, would it be useful to emit a warning
like "you have booted a non-Athlon kernel on an Athlon process, user
programs may get unexpected page faults." That's in init code, hopefully
there is no critical size issue there, I assume, other than how large a
kernel can be booted by the boot loader.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Tue, 16 Sep 2003, Jamie Lokier wrote:

> richard.brunner@amd.com wrote:
> > My concern is trying to prevent 
> > the flood of emails where someone thinks they built 
> > a "standard" kernel only to  discover that they forgot 
> > to select the various suboptions
> > and it doesn't work on their processor. I'd like
> > to simplfy what the majority of folks need to do
> > to get a broadly working kernel.
> 
> There's a similar situation with workarounds for buggy IDE chipsets,
> CMD640 and RZ1000

I'm not sure which way you're going here, those have their own config
entries, does that mean that you are advocating just making the Athlon
fixup a config option the user must set, or that all of these should be
on by default and appear in the delete-features menu, and/or be
controlled by my proposed option to build only for the target CPU. Or
was this just an informational comment?

I'm not looking to disagree with any of those suggestions, if you were
making one.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Dave Jones]

On Tue, Sep 16, 2003 at 12:46:36PM +0100, Jamie Lokier wrote:

 > > The user space problem worries me more, because the expectation
 > > is that if CPUID says the program can use perfetch, it could
 > > and should regardless of what the kernel decided to do here.
 > 
 > If the workaround isn't compiled in, "prefetch" should be removed from
 > /proc/cpuinfo on the buggy chips.

prefetch isn't a cpuid feature flag. The only way you could do
what you suggest is by removing '3dnow' or 'sse', which cripples
things more than necessary.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Mikael Pettersson wrote:
> extern inline void prefetch(const void *x)
> {
>         if (cpu_data[0].x86_vendor == X86_VENDOR_AMD)
>                 return;         /* Some athlons fault if the address is bad */
>         alternative_input(ASM_NOP4,
>                           "prefetchnta (%1)",
>                           X86_FEATURE_XMM,
>                           "r" (x));
> }
> 
> A dynamic test at each occurrence. That's truly horrible.

Is there any reason why alternative_input() cannot be used by itself?
Another synthetic feature bit would sort this out, making a kernel
that always works on AMD, and is optimised for AMD if the fixup is
configured in: X86_FEATURE_XMM_PREFETCH.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

richard.brunner@amd.com wrote:
> My concern is trying to prevent 
> the flood of emails where someone thinks they built 
> a "standard" kernel only to  discover that they forgot 
> to select the various suboptions
> and it doesn't work on their processor. I'd like
> to simplfy what the majority of folks need to do
> to get a broadly working kernel.

There's a similar situation with workarounds for buggy IDE chipsets,
CMD640 and RZ1000

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

richard.brunner@amd.com wrote:
> Andi's patch solves both the kernel space and the user space
> issues in a pretty small footprint.

Not really.  Userspace still has a problem when run on older kernels,
so it will have to check for AMD and kernel version anyway before
deciding to use the prefetch instruction.  That, or install SIGSEGV
and SIGBUS handlers to do the fixup in userspace.

> The user space problem worries me more, because the expectation
> is that if CPUID says the program can use perfetch, it could
> and should regardless of what the kernel decided to do here.

If the workaround isn't compiled in, "prefetch" should be removed from
/proc/cpuinfo on the buggy chips.

-- Jamie

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

Bill,

FWIW, I think you are on the right track!

I may haggle over really "*slow* it in some way",
but I think that can be worked out. It just a nit
in the grand scheme.

Thanks!

] -Rich ...
] AMD Fellow
] richard.brunner at amd com 

> -----Original Message-----
> From: Bill Davidsen [mailto:davidsen@tmr.com] 
> Sent: Monday, September 15, 2003 5:27 PM
> To: Adrian Bunk
> Cc: John Bradford; zwane@linuxpower.ca; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> 
> On Mon, 15 Sep 2003, Adrian Bunk wrote:
> 
> > On Mon, Sep 15, 2003 at 07:32:49AM +0100, John Bradford wrote:
> > >...
> > > It should be possible, and straightforward, to compile a 
> kernel which:
> > > 
> > > 1. Supports, (I.E. has workarounds for), any combination of CPUs.
> > >    E.G. a kernel which supports 386s, and Athlons _only_ would not
> > >    need the F00F bug workaround.  Currently '386' kernels 
> include it,
> > >    because '386' means 'support 386 and above processors'.
> > > 
> > > 2. Has compiler optimisations for one particular CPU.
> > >    E.G. the 386 and Athlon supporting kernel above could have
> > >    alignment optimised for either 386 or Athlon.
> > >...
> > 
> > That's the point where even I consider such a system to be 
> too complex.
> 
> How does it strike you to have these:
>  - compile support for any CPU which doesn't break the target
>    (including slow it in some serious way)
>  - drop support for any CPU except the target
> 
> It seems to me that this is what the vendors want (as general 
> as possible)
> and the size limited users want (small is beautiful).
> 
> Fitting the code to this model could be done gradually and 
> hopefully with
> some macros to prevent too much ugly ifdef code.
> 
> -- 
> bill davidsen <davidsen@tmr.com>
>   CTO, TMR Associates, Inc
> Doing interesting things with little computers since 1979.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Adrian Bunk wrote:

> On Mon, Sep 15, 2003 at 07:32:49AM +0100, John Bradford wrote:
> >...
> > It should be possible, and straightforward, to compile a kernel which:
> > 
> > 1. Supports, (I.E. has workarounds for), any combination of CPUs.
> >    E.G. a kernel which supports 386s, and Athlons _only_ would not
> >    need the F00F bug workaround.  Currently '386' kernels include it,
> >    because '386' means 'support 386 and above processors'.
> > 
> > 2. Has compiler optimisations for one particular CPU.
> >    E.G. the 386 and Athlon supporting kernel above could have
> >    alignment optimised for either 386 or Athlon.
> >...
> 
> That's the point where even I consider such a system to be too complex.

How does it strike you to have these:
 - compile support for any CPU which doesn't break the target
   (including slow it in some serious way)
 - drop support for any CPU except the target

It seems to me that this is what the vendors want (as general as possible)
and the size limited users want (small is beautiful).

Fitting the code to this model could be done gradually and hopefully with
some macros to prevent too much ugly ifdef code.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:

> My concern is trying to prevent 
> the flood of emails where someone thinks they built 
> a "standard" kernel only to  discover that they forgot 
> to select the various suboptions
> and it doesn't work on their processor. I'd like
> to simplfy what the majority of folks need to do
> to get a broadly working kernel.
> 
> I'd prefer that some set of options "take away"
> rather than "add" features/work-arounds. In the case below,
> I'd prefer a "don't support Athlon Prefetch Errata".

If that's what it takes I could be content to add it to the "delete
features" menu, or whatever it's called. It certainly could be on by
default if it were up in the features with F.P. emulation. As long as
non-Athlon users can make it go away with config, it's a useful solution.

But I really like having a single option which builds ONLY support for the
target CPU, with no other support which can be removed. And I'm sure the
embedded folks would help identify sections to be covered by that
definition.

> 
> I think you can argue that some features are going to 
> be common enough for the majority of users that they
> should be in the "take-away" category.
> 
> Others are uncommon enough that they fall into
> the "add" category.

I doubt anyone would disagree with that, until you start deciding which
falls where. If F.P. emulation and SMP can go in the features menu I would
think "use prefetch if available" could as well, but if it would bring
concensus to this discussion I would support putting prefetch fixup on the
takeaway menu. It solves what I see as a size problem for the future, and
continues to address the and that's valuable. Now if a few other people can agree...
> 
> If you stick with consistent nomenclature 
> (e.g. add_feature_TBD & sub_default_feature_TBD) 
> and put these options in one common config file, 
> I think the embedded folks and the 
> "optimize every last bit" folks get 
> what they want.

I think in the long run there are too many tiny features to justify a
config option for each. I would hope people would like the idea of having
a single flag (not to replace individual options) to drop support for
every architecture but the stated target. It need not do much initially,
but would be a good hook so when someone identifies code like that there's
a simple flag to use.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[David Lang]

On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:

> My concern is trying to prevent
> the flood of emails where someone thinks they built
> a "standard" kernel only to  discover that they forgot
> to select the various suboptions
> and it doesn't work on their processor. I'd like
> to simplfy what the majority of folks need to do
> to get a broadly working kernel.

I agree, and if the kernel reports a CPU/build mismatch as early in the
boot process as possible this would seem to solve this problem.

we already get a steady stream of such messages and so being able to add
this check would cut down on the support load now.

the problem seems to be that currently we don't get the ability to display
anything until fairly late in the boot process, so reporting this cleanly
may require adding another chunk of code (thankfully code that can be
thrown away as soon as it's used) to do this checking.

or another option would be to make the 'configured CPU' mask something
that bootloaders can check before they load the kernel (but that would
mean that the bootloader would have to detect the CPU type)

David Lang

> I'd prefer that some set of options "take away"
> rather than "add" features/work-arounds. In the case below,
> I'd prefer a "don't support Athlon Prefetch Errata".
>
> I think you can argue that some features are going to
> be common enough for the majority of users that they
> should be in the "take-away" category.
>
> Others are uncommon enough that they fall into
> the "add" category.
>
> If you stick with consistent nomenclature
> (e.g. add_feature_TBD & sub_default_feature_TBD)
> and put these options in one common config file,
> I think the embedded folks and the
> "optimize every last bit" folks get
> what they want.
>
> ] -Rich ...
> ] AMD Fellow
> ] richard.brunner at amd com
>
> > -----Original Message-----
> > From: Bill Davidsen [mailto:davidsen@tmr.com]
> > Sent: Monday, September 15, 2003 12:16 PM
> > To: Brunner, Richard
> > Cc: alan@lxorguk.ukuu.org.uk; zwane@linuxpower.ca;
> > linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> >
> >
> > On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:
> >
> > > I think Alan brought up a very good point. Even if you
> > > use a generic kernel that avoids prefetch use on Athlon
> > > (which I am opposed to), it doesn't solve the problem
> > > of user space programs detecting that the ISA supports
> > > prefetch and using prefetch instructions and hitting the
> > > errata on Athlon.
> > >
> > > The user space problem worries me more, because the expectation
> > > is that if CPUID says the program can use perfetch, it could
> > > and should regardless of what the kernel decided to do here.
> > >
> > > Andi's patch solves both the kernel space and the user space
> > > issues in a pretty small footprint.
> >
> > Clearly AMD would like to avoid having PIV and Athlon
> > optimized kernels,
> > and to default to adding unnecessary size to the PIV kernel to support
> > errata in the Athlon. But fighting against having a config
> > which produces
> > a smaller and faster kernel for all non-Athlon users and all
> > embedded or
> > otherwise size limited users seems to be just a marketing
> > thing so P4 code
> > will seem to work correctly on Athlon.
> >
> > Vendors will build a kernel which runs as well as possible on
> > as many CPUs
> > as possible, but users who build their own kernel want to
> > build a kernel
> > for a particular config in most cases and should have the
> > option. There
> > should be a "support Athlon prefetch" option as well, which turns on
> > the fix only when it's needed, just as there is for P4
> > thermal throttling,
> > F.P. emulation, etc. Why shouldn't this be treated the same
> > way as other
> > features already in the config menu?
> >
> > --
> > bill davidsen <davidsen@tmr.com>
> >   CTO, TMR Associates, Inc
> > Doing interesting things with little computers since 1979.
> >
> >
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Llu, 2003-09-15 at 20:34, John Bradford wrote:
> Yes, you're right, from a stability point of view I was being a bit
> impractical.  Any idea how many developers are actually regularly
> testing code on 386s these days, by the way?

I test 486 still but not 386. Even finding embedded 386 stuff that isnt
obsolete is hard nowdays (6117 etc). 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Adrian Bunk]

On Mon, Sep 15, 2003 at 07:32:49AM +0100, John Bradford wrote:
>...
> It should be possible, and straightforward, to compile a kernel which:
> 
> 1. Supports, (I.E. has workarounds for), any combination of CPUs.
>    E.G. a kernel which supports 386s, and Athlons _only_ would not
>    need the F00F bug workaround.  Currently '386' kernels include it,
>    because '386' means 'support 386 and above processors'.
> 
> 2. Has compiler optimisations for one particular CPU.
>    E.G. the 386 and Athlon supporting kernel above could have
>    alignment optimised for either 386 or Athlon.
>...

That's the point where even I consider such a system to be too complex.

If you want maximum performance compile a kernel specific for your 
system.

Compiling a kernel that supports more than one CPU is for people that 
can even live with let's say a 10% performance penalty.

If you want to run a kernel on both a 386 and an Athlon it's most likely 
non-optimal for both of them.

> John.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Adrian Bunk]

On Mon, Sep 15, 2003 at 06:32:49PM +1000, Nick Piggin wrote:
>...
> While I like Adrian's patch a lot from a functionality and user
> simplicity point of view, the key to getting it merged is not to 
> increase the complexity of the implementation. The only objections to
> the concept came from people who didn't understand it AFAIK.
>...

My impresion is that much problem comes from the fact that I didn't 
split the patch the first time I sent it.

Most of the oppositon came against the arch/i386/kernel/cpu/{,mtrr/} 
optimizations that are more an eample of how to achive further space 
savings in this scheme but not a required part of this patch.

The main part wasn't non-controversal but it didn't have such a big 
number of opponents.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nakajima, Jun]

> I'd prefer that some set of options "take away"
> rather than "add" features/work-arounds. In the case below,
> I'd prefer a "don't support Athlon Prefetch Errata".

So the best way would be "make it configurable, and set it on by
default."? I don't think forcing everyone to have workarounds for
particular errata is desirable, but I agree that preventing mistakes is
a good thing. 

Thanks,
Jun

> -----Original Message-----
> From: richard.brunner@amd.com [mailto:richard.brunner@amd.com]
> Sent: Monday, September 15, 2003 12:51 PM
> To: davidsen@tmr.com
> Cc: alan@lxorguk.ukuu.org.uk; zwane@linuxpower.ca; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> My concern is trying to prevent
> the flood of emails where someone thinks they built
> a "standard" kernel only to  discover that they forgot
> to select the various suboptions
> and it doesn't work on their processor. I'd like
> to simplfy what the majority of folks need to do
> to get a broadly working kernel.
> 
> I'd prefer that some set of options "take away"
> rather than "add" features/work-arounds. In the case below,
> I'd prefer a "don't support Athlon Prefetch Errata".
> 
> I think you can argue that some features are going to
> be common enough for the majority of users that they
> should be in the "take-away" category.
> 
> Others are uncommon enough that they fall into
> the "add" category.
> 
> If you stick with consistent nomenclature
> (e.g. add_feature_TBD & sub_default_feature_TBD)
> and put these options in one common config file,
> I think the embedded folks and the
> "optimize every last bit" folks get
> what they want.
> 
> ] -Rich ...
> ] AMD Fellow
> ] richard.brunner at amd com
> 
> > -----Original Message-----
> > From: Bill Davidsen [mailto:davidsen@tmr.com]
> > Sent: Monday, September 15, 2003 12:16 PM
> > To: Brunner, Richard
> > Cc: alan@lxorguk.ukuu.org.uk; zwane@linuxpower.ca;
> > linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch
errata
> >
> >
> > On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:
> >
> > > I think Alan brought up a very good point. Even if you
> > > use a generic kernel that avoids prefetch use on Athlon
> > > (which I am opposed to), it doesn't solve the problem
> > > of user space programs detecting that the ISA supports
> > > prefetch and using prefetch instructions and hitting the
> > > errata on Athlon.
> > >
> > > The user space problem worries me more, because the expectation
> > > is that if CPUID says the program can use perfetch, it could
> > > and should regardless of what the kernel decided to do here.
> > >
> > > Andi's patch solves both the kernel space and the user space
> > > issues in a pretty small footprint.
> >
> > Clearly AMD would like to avoid having PIV and Athlon
> > optimized kernels,
> > and to default to adding unnecessary size to the PIV kernel to
support
> > errata in the Athlon. But fighting against having a config
> > which produces
> > a smaller and faster kernel for all non-Athlon users and all
> > embedded or
> > otherwise size limited users seems to be just a marketing
> > thing so P4 code
> > will seem to work correctly on Athlon.
> >
> > Vendors will build a kernel which runs as well as possible on
> > as many CPUs
> > as possible, but users who build their own kernel want to
> > build a kernel
> > for a particular config in most cases and should have the
> > option. There
> > should be a "support Athlon prefetch" option as well, which turns on
> > the fix only when it's needed, just as there is for P4
> > thermal throttling,
> > F.P. emulation, etc. Why shouldn't this be treated the same
> > way as other
> > features already in the config menu?
> >
> > --
> > bill davidsen <davidsen@tmr.com>
> >   CTO, TMR Associates, Inc
> > Doing interesting things with little computers since 1979.
> >
> >
> 
> -
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nakajima, Jun]

> Of course if you have PIV kernel you don't need any of the Athlon code
at
> all, so the size and performance penalty is zero. And if you build for
a
> CPU which doesn't have prefetch you don't need any of that code at
all.

That's true. But I think sharing the same source/binary in some
reasonable fashion is much more beneficial to Linux and the users,
especially because we can improve the quality and performance more
efficiently. 

I think the way Andi implemented (see below, for example) is one of the
best solutions as far as I think of. 

extern inline void prefetchw(const void *x)
{
	alternative_input(ASM_NOP4,
			  "prefetchw (%1)",
			  X86_FEATURE_3DNOW,
			  "r" (x));
}

Thanks,
Jun

> -----Original Message-----
> From: Bill Davidsen [mailto:davidsen@tmr.com]
> Sent: Monday, September 15, 2003 11:51 AM
> To: Alan Cox
> Cc: Zwane Mwaikambo; Linux Kernel Mailing List
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> On Mon, 15 Sep 2003, Alan Cox wrote:
> 
> > On Llu, 2003-09-15 at 13:11, Bill Davidsen wrote:
> > > The code to disable prefetch on Athlon is 300 bytes and hurts your
> PIV?
> > > Really? I'll dig back through the code, but I recall it as adding
or
> > > deleting an entry in a table to enable prefetch. If it's affecting
PIV
> the
> > > code to use prefetch is seriously broken.
> >
> > Big time. Its over 300 bytes long because its embedded in each
inline
> > prefetch and it causes a branch misprediction almost every time.
Amazing
> > what you find when you actually measure stuff 8)
> >
> > So right now, its faster on PIV to delete the prefetch than use the
> > current hack, and adding the Athlon fix makes Athlon and PIV faster
and
> > total memory size lower.
> 
> Of course if you have PIV kernel you don't need any of the Athlon code
at
> all, so the size and performance penalty is zero. And if you build for
a
> CPU which doesn't have prefetch you don't need any of that code at
all.
> 
> --
> bill davidsen <davidsen@tmr.com>
>   CTO, TMR Associates, Inc
> Doing interesting things with little computers since 1979.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

My concern is trying to prevent 
the flood of emails where someone thinks they built 
a "standard" kernel only to  discover that they forgot 
to select the various suboptions
and it doesn't work on their processor. I'd like
to simplfy what the majority of folks need to do
to get a broadly working kernel.

I'd prefer that some set of options "take away"
rather than "add" features/work-arounds. In the case below,
I'd prefer a "don't support Athlon Prefetch Errata".

I think you can argue that some features are going to 
be common enough for the majority of users that they
should be in the "take-away" category.

Others are uncommon enough that they fall into
the "add" category.

If you stick with consistent nomenclature 
(e.g. add_feature_TBD & sub_default_feature_TBD) 
and put these options in one common config file, 
I think the embedded folks and the 
"optimize every last bit" folks get 
what they want.

] -Rich ...
] AMD Fellow
] richard.brunner at amd com 

> -----Original Message-----
> From: Bill Davidsen [mailto:davidsen@tmr.com] 
> Sent: Monday, September 15, 2003 12:16 PM
> To: Brunner, Richard
> Cc: alan@lxorguk.ukuu.org.uk; zwane@linuxpower.ca; 
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> 
> On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:
> 
> > I think Alan brought up a very good point. Even if you
> > use a generic kernel that avoids prefetch use on Athlon
> > (which I am opposed to), it doesn't solve the problem
> > of user space programs detecting that the ISA supports
> > prefetch and using prefetch instructions and hitting the
> > errata on Athlon.
> > 
> > The user space problem worries me more, because the expectation
> > is that if CPUID says the program can use perfetch, it could
> > and should regardless of what the kernel decided to do here.
> > 
> > Andi's patch solves both the kernel space and the user space
> > issues in a pretty small footprint.
> 
> Clearly AMD would like to avoid having PIV and Athlon 
> optimized kernels,
> and to default to adding unnecessary size to the PIV kernel to support
> errata in the Athlon. But fighting against having a config 
> which produces
> a smaller and faster kernel for all non-Athlon users and all 
> embedded or
> otherwise size limited users seems to be just a marketing 
> thing so P4 code
> will seem to work correctly on Athlon.
> 
> Vendors will build a kernel which runs as well as possible on 
> as many CPUs
> as possible, but users who build their own kernel want to 
> build a kernel
> for a particular config in most cases and should have the 
> option. There
> should be a "support Athlon prefetch" option as well, which turns on
> the fix only when it's needed, just as there is for P4 
> thermal throttling,
> F.P. emulation, etc. Why shouldn't this be treated the same 
> way as other
> features already in the config menu?
> 
> -- 
> bill davidsen <davidsen@tmr.com>
>   CTO, TMR Associates, Inc
> Doing interesting things with little computers since 1979.
> 
> 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> > > I still like the idea of a single config variable to remove all special
> > > case code for non-configured CPUs, call it NO_BLOAT or MINIMALIST_KERNEL
> > > or EMBEDDED_HELPER as you will. The embedded folks would then have a good
> > > handle to do the work and identify sections to be so identified.
> > 
> > Removing the code for non-configured CPUs should be the default.  It's
> > common sense - if you configure a kernel to support Athlons only, why
> > have PIV workarounds in there, unless you're actually debugging a
> > kernel problem?
>
> If we adopt a bit-per-CPUtype or similar approach maybe. But then you have
> to go back and test each code section to see if it applies to multiple
> types. I'm happy to have existing code stay, as long as there's a way to
> clean it up (or attempt to do so). I don't think making super clean is
> compatible with stability, and I'd rather see sections marked as
> architecture specific as the performance and embedded folks look for
> places to clean up the kernel.

Yes, you're right, from a stability point of view I was being a bit
impractical.  Any idea how many developers are actually regularly
testing code on 386s these days, by the way?

> I'm not on a crusade to get the tiny kernel, just to (a) provide a path
> for future work started by the config "feature removal" menu, and (b)
> avoid inserting a chunk of very specific code without ifdefs, now that
> developers have started thinking about putting the kernel on a diet. I
> don't suggest we do anything which will break existing code, just not
> introduce new bloat right now.

Yeah, breaking existing code can wait until 2.7 :-).  Putting the
hooks in for future code removal is all we need to do - until somebody
actually feels the need to trim something, it might as well stay.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, John Bradford wrote:

> Yes, you're right, from a stability point of view I was being a bit
> impractical.  Any idea how many developers are actually regularly
> testing code on 386s these days, by the way?

Embedded folks, can you tell us? I believe one of the low power CPUs looks
like a 386 w/o F.P. but with a few 486 instructions. Clarification?

CC list trimmed since this has turned from policy to technical.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> > > In the model I'm proposing, the 386 kernel would be missing the Athlon
> > > workarounds.
> > 
> > This is unworkable unless you also have all the existing models where
> > you have fixes for later processors too. 
>
> I think John wants to have the default be that only code for the target
> CPU be included in the kernel when built for a given CPU.

Yes, that's what I meant.

> There's no
> reason why someone building for 386 would want code for other CPUs at all,
> vendors and people who want to run a single suboptimal kernel on multiple
> machines.
>
> My own suggestion is that the default could continue to be "include
> anything which doesn't break or drastically slow the target CPU," and then
> have a flag value to exclude fixups or enhancements for other CPUs. This
> allows existing code to be gradully marked for simplification by embedded
> users or those who just want to avoid overhead in their kernel.

Ah, OK, on reflection that's probably more realistic from a practical
viewpoint, (see my other post in this thread).

John.

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003 richard.brunner@amd.com wrote:

> I think Alan brought up a very good point. Even if you
> use a generic kernel that avoids prefetch use on Athlon
> (which I am opposed to), it doesn't solve the problem
> of user space programs detecting that the ISA supports
> prefetch and using prefetch instructions and hitting the
> errata on Athlon.
> 
> The user space problem worries me more, because the expectation
> is that if CPUID says the program can use perfetch, it could
> and should regardless of what the kernel decided to do here.
> 
> Andi's patch solves both the kernel space and the user space
> issues in a pretty small footprint.

Clearly AMD would like to avoid having PIV and Athlon optimized kernels,
and to default to adding unnecessary size to the PIV kernel to support
errata in the Athlon. But fighting against having a config which produces
a smaller and faster kernel for all non-Athlon users and all embedded or
otherwise size limited users seems to be just a marketing thing so P4 code
will seem to work correctly on Athlon.

Vendors will build a kernel which runs as well as possible on as many CPUs
as possible, but users who build their own kernel want to build a kernel
for a particular config in most cases and should have the option. There
should be a "support Athlon prefetch" option as well, which turns on
the fix only when it's needed, just as there is for P4 thermal throttling,
F.P. emulation, etc. Why shouldn't this be treated the same way as other
features already in the config menu?

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Alan Cox wrote:

> On Llu, 2003-09-15 at 13:11, Bill Davidsen wrote:
> > The code to disable prefetch on Athlon is 300 bytes and hurts your PIV?
> > Really? I'll dig back through the code, but I recall it as adding or
> > deleting an entry in a table to enable prefetch. If it's affecting PIV the
> > code to use prefetch is seriously broken.
> 
> Big time. Its over 300 bytes long because its embedded in each inline
> prefetch and it causes a branch misprediction almost every time. Amazing
> what you find when you actually measure stuff 8)
> 
> So right now, its faster on PIV to delete the prefetch than use the
> current hack, and adding the Athlon fix makes Athlon and PIV faster and
> total memory size lower.

Of course if you have PIV kernel you don't need any of the Athlon code at
all, so the size and performance penalty is zero. And if you build for a
CPU which doesn't have prefetch you don't need any of that code at all.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Alan Cox wrote:

> On Llu, 2003-09-15 at 11:54, John Bradford wrote:
> > In the model I'm proposing, the 386 kernel would be missing the Athlon
> > workarounds.
> 
> This is unworkable unless you also have all the existing models where
> you have fixes for later processors too. 

I think John wants to have the default be that only code for the target
CPU be included in the kernel when built for a given CPU. There's no
reason why someone building for 386 would want code for other CPUs at all,
vendors and people who want to run a single suboptimal kernel on multiple
machines.

My own suggestion is that the default could continue to be "include
anything which doesn't break or drastically slow the target CPU," and then
have a flag value to exclude fixups or enhancements for other CPUs. This
allows existing code to be gradully marked for simplification by embedded
users or those who just want to avoid overhead in their kernel.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Its kind of irrelevant when by saying "Athlon" you've added 128 byte
                                        ^^^^^^^
> alignment to all the cache friendly structure padding. There are systems
> where memory matters, but spending a week chasing 300 bytes when you can
> knock out 50K is a waste of everyones time. Do the 40K problems first

I suspect Alan meant "P4" above, Athlon only adds 64byte padding.
Default is 32 byte for 686.

But it's interesting how a typo can serve as a basis for much further
discussion in this thread @)

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, John Bradford wrote:

> > I still like the idea of a single config variable to remove all special
> > case code for non-configured CPUs, call it NO_BLOAT or MINIMALIST_KERNEL
> > or EMBEDDED_HELPER as you will. The embedded folks would then have a good
> > handle to do the work and identify sections to be so identified.
> 
> Removing the code for non-configured CPUs should be the default.  It's
> common sense - if you configure a kernel to support Athlons only, why
> have PIV workarounds in there, unless you're actually debugging a
> kernel problem?

If we adopt a bit-per-CPUtype or similar approach maybe. But then you have
to go back and test each code section to see if it applies to multiple
types. I'm happy to have existing code stay, as long as there's a way to
clean it up (or attempt to do so). I don't think making super clean is
compatible with stability, and I'd rather see sections marked as
architecture specific as the performance and embedded folks look for
places to clean up the kernel.

I'm not on a crusade to get the tiny kernel, just to (a) provide a path
for future work started by the config "feature removal" menu, and (b)
avoid inserting a chunk of very specific code without ifdefs, now that
developers have started thinking about putting the kernel on a diet. I
don't suggest we do anything which will break existing code, just not
introduce new bloat right now.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Mikael Pettersson wrote:

> On Mon, 15 Sep 2003 08:11:12 -0400 (EDT), Bill Davidsen <davidsen@tmr.com> wrote:
> > On Mon, 15 Sep 2003, Alan Cox wrote:
> > > That disable you talk about is bloat. It also trashes the performance of
> > > PIV boxes. In fact I checked out of interest - the disable hack
> > > currently being used is adding *over* 300 bytes to my kernel as its
> > > inlined repeatedly. So its larger, and it ruins performance for all
> > > processors.
> > 
> > The code to disable prefetch on Athlon is 300 bytes and hurts your PIV?
> > Really? I'll dig back through the code, but I recall it as adding or
> > deleting an entry in a table to enable prefetch. If it's affecting PIV the
> > code to use prefetch is seriously broken.
> 
> Bill, look in include/asm-i386/processor.h:
> 
> extern inline void prefetch(const void *x)
> {
>         if (cpu_data[0].x86_vendor == X86_VENDOR_AMD)
>                 return;         /* Some athlons fault if the address is bad */
>         alternative_input(ASM_NOP4,
>                           "prefetchnta (%1)",
>                           X86_FEATURE_XMM,
>                           "r" (x));
> }
> 
> A dynamic test at each occurrence. That's truly horrible.
> (And I'll hack it out of _my_ kernels ASAP. Can't imagine
> I missed that one.)

That's exactly the type of thing a MINIMAL_CODE flag could
drop unless Athlon support was enabled. However, the test should be at the
caller(s), to avoid a call/return in the first place.

To avoid really ugly source code it would probably be desirable to just
define a do_prefetch_if_supported() macro, and put any and all future
magic there. If the fix is used the test for AMD isn't needed, I would
just like to avoid having the test or the fix at all on my Intel systems.
And if I read correctly, that errata will be fixed in future Athlon CPUs,
so a better test will be needed anyway.

The code is appropriate for a generic X86 kernel, but not for one built to
one specific CPU. Everyone can be happy through the magin of Kconfig ;-)

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

I think Alan brought up a very good point. Even if you
use a generic kernel that avoids prefetch use on Athlon
(which I am opposed to), it doesn't solve the problem
of user space programs detecting that the ISA supports
prefetch and using prefetch instructions and hitting the
errata on Athlon.

The user space problem worries me more, because the expectation
is that if CPUID says the program can use perfetch, it could
and should regardless of what the kernel decided to do here.

Andi's patch solves both the kernel space and the user space
issues in a pretty small footprint.


] -Rich ...
] AMD Fellow
] richard.brunner at amd com 

> -----Original Message-----
> From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
> Sent: Monday, September 15, 2003 12:46 AM

> You also need it for userspace prefetch fault fixup for a 
> kernel without
> CONFIG_MK7 to run stuff perfectly on Athlon.
> 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> > In the model I'm proposing, the 386 kernel would be missing the Athlon
> > workarounds.
>
> This is unworkable unless you also have all the existing models where
> you have fixes for later processors too. 

I'm pretty sure we're talking about two different things - I don't
understand what you mean about all the exisiting models.  Are you
saying that with Adrian's patch we still can't simply include this
workaround [1] if and only if the user has included Athlon support,
rather than CPU<=Athlon?

[1] Obviously the point isn't just applicable to this work around, but
this is a new one going in.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

Chris Meadors wrote:

>On Mon, 2003-09-15 at 08:38, Nick Piggin wrote:
>
>
>>OK, the reason why I don't like the sound of this is because the size
>>of your option set has now been squared, and its no longer "make these
>>CPUs work".
>>
>
>The way I see the config option working is, having a sub-menu that says
>something like, "Select the CPU the kernel will be run on."  From there
>you can pick one CPU.  This sets the in kernel optimizations for that
>CPU, along with the work arounds (obviously).  It also sets the compiler
>flags for the padding, and "-march=[CPU]".  Then in a sub-menu of this
>menu, there is an "Advanced processor selection".  The help text would
>be something like, "In addition to the primary processor selected above,
>also allow this kernel to be booted on the processors selected below. 
>Selecting this option disables some of the optimizations for the primary
>processor."  Just turning on that option would change the "-march=" to
>"-mcpu=".  Then in the menu one could select from any of the CPUs
>listed.  Each one would enable the work around code to allow the built
>kernel to run correctly on a machine with a different CPU.
>
>I see this as sort of like the advanced partition selection, or the
>compiled in fonts.
>

Look at the complexity though!

>
>>I can see an argument for cache line size but thats about it. I can't
>>think of my optimisations that should be done on one architecture but
>>not another.
>>
>
>Don't forget the compiler optimization flags.  A kernel built with
>"-march" may not run on any other CPUs.
>

Yeah, I really meant _should_, not must. In other words, something that
is an optimisation for one architecutre, but a pessimisation for
another.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Llu, 2003-09-15 at 13:11, Bill Davidsen wrote:
> The code to disable prefetch on Athlon is 300 bytes and hurts your PIV?
> Really? I'll dig back through the code, but I recall it as adding or
> deleting an entry in a table to enable prefetch. If it's affecting PIV the
> code to use prefetch is seriously broken.

Big time. Its over 300 bytes long because its embedded in each inline
prefetch and it causes a branch misprediction almost every time. Amazing
what you find when you actually measure stuff 8)

So right now, its faster on PIV to delete the prefetch than use the
current hack, and adding the Athlon fix makes Athlon and PIV faster and
total memory size lower.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Chris Meadors]

On Mon, 2003-09-15 at 08:38, Nick Piggin wrote:

> OK, the reason why I don't like the sound of this is because the size
> of your option set has now been squared, and its no longer "make these
> CPUs work".

The way I see the config option working is, having a sub-menu that says
something like, "Select the CPU the kernel will be run on."  From there
you can pick one CPU.  This sets the in kernel optimizations for that
CPU, along with the work arounds (obviously).  It also sets the compiler
flags for the padding, and "-march=[CPU]".  Then in a sub-menu of this
menu, there is an "Advanced processor selection".  The help text would
be something like, "In addition to the primary processor selected above,
also allow this kernel to be booted on the processors selected below. 
Selecting this option disables some of the optimizations for the primary
processor."  Just turning on that option would change the "-march=" to
"-mcpu=".  Then in the menu one could select from any of the CPUs
listed.  Each one would enable the work around code to allow the built
kernel to run correctly on a machine with a different CPU.

I see this as sort of like the advanced partition selection, or the
compiled in fonts.

> I can see an argument for cache line size but thats about it. I can't
> think of my optimisations that should be done on one architecture but
> not another.

Don't forget the compiler optimization flags.  A kernel built with
"-march" may not run on any other CPUs.

-- 
Chris

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Llu, 2003-09-15 at 11:54, John Bradford wrote:
> In the model I'm proposing, the 386 kernel would be missing the Athlon
> workarounds.

This is unworkable unless you also have all the existing models where
you have fixes for later processors too. 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> I still like the idea of a single config variable to remove all special
> case code for non-configured CPUs, call it NO_BLOAT or MINIMALIST_KERNEL
> or EMBEDDED_HELPER as you will. The embedded folks would then have a good
> handle to do the work and identify sections to be so identified.

Removing the code for non-configured CPUs should be the default.  It's
common sense - if you configure a kernel to support Athlons only, why
have PIV workarounds in there, unless you're actually debugging a
kernel problem?

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

John Bradford wrote:

snip

>>No, debug the kernel while its running on the 386.
>>
>
>What if the 386 is a wristwatch, or similar embedded device?
>

You might be right. I'm not sure how the embedded software process goes.
I'd think that most of the time you'd be fine compiling different
kernels for each: you probably want a lot more options in your debug
kernel anyway.

I guess more specialised users would be able to edit the cache line
size themselves. I wouldn't be adverse to a kconfig setting under the
embedded menu though.

>
>>And what of my other
>>concerns?
>>
>
>Since nobody seemed to agree with me, I was sparing the list the
>bandwidth, but...
>

Ahh, they don't have to read it ;)

>
>>1. It doesn't appear to be simple and elegant.
>>
>
>Well, it's obviously not as simple as just using 128 byte padding all
>the time, but the basic idea of, 'choose required work-arounds, and
>optimisations independently of each other', is fairly simple, (in
>concept), and elegant, (as it lets you compile the most finely tuned
>binary).
>

OK, the reason why I don't like the sound of this is because the size
of your option set has now been squared, and its no longer "make these
CPUs work".

I can see an argument for cache line size but thats about it. I can't
think of my optimisations that should be done on one architecture but
not another.

>
>Maybe we are not thinking along the same lines.
>
>Up to now, selecting a CPU to compile for basically means, "Use
>compiler optimisations for this CPU, and don't care about
>compatibility with anything before it".  Adrian's patch to change this
>to an arbitrary bitmap selection of CPUs to support seems like a good
>idea to me for two reasons, firstly with increasing numbers of
>work-arounds, it's silly to include them all in a kernel for a 386.
>Secondly, ever since we included support for optimising for the 686,
>the idea that a kernels compiled for progressively more recent CPUs
>would be faster than each other on the same hardware has been false -
>a kernel compiled for a Pentium is slower on a 686 than a kernel
>compiled for a 486 is.
>

I think its a great idea. I hope it gets included in some form. For an
end user (including distros) its a lot simpler and more logical than the
current scheme.

>
>So, if we move from selecting a range of CPUs to support, I.E. 386 ->
>whatever, to selecting individual CPUs, E.G. 386, PIV, and Athlon,
>there is no question about which workarounds we should include.  By
>the way, I am talking about including them at compile time, not
>checking at run time whether they are needed - maybe I wasn't clear
>about that.  I don't see the point in checking at runtime - any kernel
>

No I definitely agree. Except sometimes you'll have to check at runtime:
a kernel compiled for all cpus for example needs Andi's Athlon prefetch
scheme. You'd really want some good helpers to either do runtime check
or always, or never. Something like this optimises down OK if cpu and
archmask are constant.

static inline void ifcpu(const int cpumask, void (*func)(void))
{
        if ((cpumask&archmask) && ((~cpumask)&archmask)) {
                if (cpumask&current_cpu)
                        func();
        } else if (cpumask&archmask) {
                func();
        }
}

ifcpu(K7||K8, &prefetch_workaround);

You then need to get kconfig to generate cpu and archmask nicely.
You obviously still need to ifdef your prefetch_workaround to get the
space saving.

>
>that supports multiple CPUs is not optimial anyway, so why bother
>trying to optimise it at all?  I know there is another way of looking
>at it, that distributions will want a kernel that runs on anything,
>(well, these days, probably a 486 or higher CPU), that is not
>particularly sub-optimial on any CPU, so that users can just install
>it, and have it work.  In that case, I totally agree with you that 128
>byte padding is the most sensible way to go, but that is a
>distribution thing.  Anybody who compiles their own kernel is probably
>going to want to compile it for the processor it's destined to run on,
>rather than make a generic kernel, unless they are making a boot disk
>for emergencies, in which case performance is not usually an issue.
>So, the question of which workarounds to include is simple, but what
>to do about optimisation?  In the current model, where you are
>selecting a range of CPUs to support, (E.G. 386->Pentium), the
>question is answered by saying, OK, we'll optimise for the most recent
>processor in that range.  With an arbitrary selection, E.G. PIV and
>Athlon, which do you pad for?  Whichever is least harmful to
>performance on the others?  This is what I meant by simple and elegant
>- you just present independent choice to the user in the
>configurator.
>
>
>>2. It would drive developers nuts if it was used for anything other than
>>   a couple of critical functions (cache size would be one).
>>
>
>OK, well by using the 'optimisation' setting simply for setting cache
>size alone, you'd still get a nice tunable kernel.  Much better than
>just setting it to an arbitrary value.
>

I'll give that to you. I'm all for more tunability if it can be coded
nicely.

>
>>3. Are there valid situations where you would need it? This isn't a
>>   rhetorical question. Your example would be fine if somebody really
>>   needed to do that.
>>
>
>Personally, I compile specific kernels for all of my boxes separately.
>No box runs any kind of generic kernel in normal use, so I'd like to
>see as many unnecessary workarounds removed from the code as possible
>at compile time, and appropriate compiler optimisations for only the
>specific CPU the kernel is destined for.  That maximises kernel
>performance on that mahcine.  On the other hand, if I'm working on an
>embedded project with a 386 or a 486, I'm usually running the same
>environment on a more powerful box as well, for testing purposes, so I
>need workarounds for the, (E.G. Athlon), CPU in the development box,
>but I don't want performance optimisations for that faster CPU,
>especially if they have a negative effect on the embedded CPU.
>

Maybe for the cache size. Anything more and I think it gets too complex.
Then again, I don't know of any other optimisations with the properties
of the L1_CACHE_SHIFT.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Mikael Pettersson]

On Mon, 15 Sep 2003 08:11:12 -0400 (EDT), Bill Davidsen <davidsen@tmr.com> wrote:
> On Mon, 15 Sep 2003, Alan Cox wrote:
> > That disable you talk about is bloat. It also trashes the performance of
> > PIV boxes. In fact I checked out of interest - the disable hack
> > currently being used is adding *over* 300 bytes to my kernel as its
> > inlined repeatedly. So its larger, and it ruins performance for all
> > processors.
> 
> The code to disable prefetch on Athlon is 300 bytes and hurts your PIV?
> Really? I'll dig back through the code, but I recall it as adding or
> deleting an entry in a table to enable prefetch. If it's affecting PIV the
> code to use prefetch is seriously broken.

Bill, look in include/asm-i386/processor.h:

extern inline void prefetch(const void *x)
{
        if (cpu_data[0].x86_vendor == X86_VENDOR_AMD)
                return;         /* Some athlons fault if the address is bad */
        alternative_input(ASM_NOP4,
                          "prefetchnta (%1)",
                          X86_FEATURE_XMM,
                          "r" (x));
}

A dynamic test at each occurrence. That's truly horrible.
(And I'll hack it out of _my_ kernels ASAP. Can't imagine
I missed that one.)

/Mikael

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Alan Cox wrote:

> On Llu, 2003-09-15 at 04:55, Bill Davidsen wrote:
> > 1 - the code is not needed for Athlon, prefetch is turned off on broken
> >     CPUs now. A generic kernel runs fine on Athlon.
> 
> That disable you talk about is bloat. It also trashes the performance of
> PIV boxes. In fact I checked out of interest - the disable hack
> currently being used is adding *over* 300 bytes to my kernel as its
> inlined repeatedly. So its larger, and it ruins performance for all
> processors.

The code to disable prefetch on Athlon is 300 bytes and hurts your PIV?
Really? I'll dig back through the code, but I recall it as adding or
deleting an entry in a table to enable prefetch. If it's affecting PIV the
code to use prefetch is seriously broken.

And since this only buys 2-5% in the kernel, I really question your "ruins
performance" claim.

> 
> You also need it for userspace prefetch fault fixup for a kernel without
> CONFIG_MK7 to run stuff perfectly on Athlon.

You mean you have a program which enables userspace prefetch and doesn't
handle the botch? Or that you have programs which you compiled for PIV and
which don't run properly on Athlon. It's called misconfigured, they won't
run on 386 either.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, Alan Cox wrote:

> On Llu, 2003-09-15 at 07:32, John Bradford wrote:
> > That's a non-issue.  300 bytes matters a lot on some systems.  The
> > fact that there are drivers that are bloated is nothing to do with
> > it.
> 
> Its kind of irrelevant when by saying "Athlon" you've added 128 byte
> alignment to all the cache friendly structure padding. There are systems
> where memory matters, but spending a week chasing 300 bytes when you can
> knock out 50K is a waste of everyones time. Do the 40K problems first

If we were talking about cleaning the kernel I would agree, and perhaps we
are in the long run. But what is being done here is to add a big chunk of
new code which only benefitts Athlon, and to resist putting ifdefs around
it so the rest of world doesn't have to waste bytes on it.

The config options to reduce kernel size by shedding features for embedded
and similar are new options (relatively). This seems to be the first big
chunk of totally machine dependent code being added after Linux took the
first step back toward being trim again.

No one has come up with any reason why this code can't have an ifdef
around it, other than one claim that distros would not be able to use it
(are there any vendors who can't do a config?) and Andi who would have to
set the config to build one kernel for all his machines.

I have an Athlon, three more on order, and an Athlon laptop in eval. I'm
hardly against them, I just don't want the extra code floating around my
non-Athlon machines. Why is there resistance to making this code
conditional, it's certainly not generally useful, it's a bugfix.

I still like the idea of a single config variable to remove all special
case code for non-configured CPUs, call it NO_BLOAT or MINIMALIST_KERNEL
or EMBEDDED_HELPER as you will. The embedded folks would then have a good
handle to do the work and identify sections to be so identified.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Mon, 15 Sep 2003, John Bradford wrote:

    [... many good points ...]

> This makes it trivial to:
> 
> * Make a kernel for a distribution's initial install
>   Select all CPUs as supported, and optimise for 686.
> 
> * Make an optimised kernel for any system
>   Select only the target CPU as supported, and optimise for it
> 
> * Make a generic kernel for PIV, and Athlon
>   Select PIV and Athlon only as supported.  Optimise for either, or
>   optimise for 386, (yes, even though it is not supported), for a
>   small kernel, on the basis that it will maximise cache usage, and be
>   fairly optimal on both systems.

That last is a good point for sure, I have seen several posts indicating
that -Os is faster on small cache machines like old Celerons, it would be
a sensible choice for a distro, and make the kernel smaller as well.
Kernels are getting near the limits of some machines to boot them.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> >>>>>>>That's a non-issue.  300 bytes matters a lot on some systems.  The
> >>>>>>>fact that there are drivers that are bloated is nothing to do with
> >>>>>>>it.
> >>>>>>>
> >>>>>>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
> >>>>>>alignment to all the cache friendly structure padding.
> >>>>>>
> >>>>>My intention is that we won't have done 128 byte alignments just by
> >>>>>'supporting' Athlons, only if we want to run fast on Athlons.  A
> >>>>>distribution kernel that is intended to boot on all CPUs needs
> >>>>>workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
> >>>>>
> >>>>>Obviously using such a kernel for anything other than getting a system
> >>>>>up and running to compile a better kernel is a Bad Thing, but the
> >>>>>distributions could supply separate Athlon, PIV, and 386 _optimised_
> >>>>>kernels.
> >>>>>
> >>>>Why bother with that complexity? Just use 128 byte lines. This allows
> >>>>a decent generic kernel. The people who have space requirements would
> >>>>only compile what they need anyway.
> >>>>
> >>>So, basically, if you compile a kernel for a 386, but think that maybe
> >>>one day you might need to run it on an Athlon for debugging purposes,
> >>>you use 128 byte padding, because it's not too bad on the 386?  Seems
> >>>pretty wasteful to me when the obvious, simple, elegant solution is to
> >>>allow independent selection of workaround inclusion and optimisation.
> >>>Especially since half of the work has already been done.
> >>>
> >>I missed the "simple, elegant" part. Conceptually elegant maybe.
> >>
> >>If you mean to use the optimise option only to set cache line size, then
> >>that might be a bit saner.
> >>
> >>As far as the case study goes though: if you were worried about being
> >>wasteful, why wouldn't you compile just for the 386 and debug from that?
> >
> >In the model I'm proposing, the 386 kernel would be missing the Athlon
> >workarounds.
>
> No, debug the kernel while its running on the 386.

What if the 386 is a wristwatch, or similar embedded device?

> And what of my other
> concerns?

Since nobody seemed to agree with me, I was sparing the list the
bandwidth, but...

> 1. It doesn't appear to be simple and elegant.

Well, it's obviously not as simple as just using 128 byte padding all
the time, but the basic idea of, 'choose required work-arounds, and
optimisations independently of each other', is fairly simple, (in
concept), and elegant, (as it lets you compile the most finely tuned
binary).

Maybe we are not thinking along the same lines.

Up to now, selecting a CPU to compile for basically means, "Use
compiler optimisations for this CPU, and don't care about
compatibility with anything before it".  Adrian's patch to change this
to an arbitrary bitmap selection of CPUs to support seems like a good
idea to me for two reasons, firstly with increasing numbers of
work-arounds, it's silly to include them all in a kernel for a 386.
Secondly, ever since we included support for optimising for the 686,
the idea that a kernels compiled for progressively more recent CPUs
would be faster than each other on the same hardware has been false -
a kernel compiled for a Pentium is slower on a 686 than a kernel
compiled for a 486 is.

So, if we move from selecting a range of CPUs to support, I.E. 386 ->
whatever, to selecting individual CPUs, E.G. 386, PIV, and Athlon,
there is no question about which workarounds we should include.  By
the way, I am talking about including them at compile time, not
checking at run time whether they are needed - maybe I wasn't clear
about that.  I don't see the point in checking at runtime - any kernel
that supports multiple CPUs is not optimial anyway, so why bother
trying to optimise it at all?  I know there is another way of looking
at it, that distributions will want a kernel that runs on anything,
(well, these days, probably a 486 or higher CPU), that is not
particularly sub-optimial on any CPU, so that users can just install
it, and have it work.  In that case, I totally agree with you that 128
byte padding is the most sensible way to go, but that is a
distribution thing.  Anybody who compiles their own kernel is probably
going to want to compile it for the processor it's destined to run on,
rather than make a generic kernel, unless they are making a boot disk
for emergencies, in which case performance is not usually an issue.
So, the question of which workarounds to include is simple, but what
to do about optimisation?  In the current model, where you are
selecting a range of CPUs to support, (E.G. 386->Pentium), the
question is answered by saying, OK, we'll optimise for the most recent
processor in that range.  With an arbitrary selection, E.G. PIV and
Athlon, which do you pad for?  Whichever is least harmful to
performance on the others?  This is what I meant by simple and elegant
- you just present independent choice to the user in the
configurator.

> 2. It would drive developers nuts if it was used for anything other than
>    a couple of critical functions (cache size would be one).

OK, well by using the 'optimisation' setting simply for setting cache
size alone, you'd still get a nice tunable kernel.  Much better than
just setting it to an arbitrary value.

> 3. Are there valid situations where you would need it? This isn't a
>    rhetorical question. Your example would be fine if somebody really
>    needed to do that.

Personally, I compile specific kernels for all of my boxes separately.
No box runs any kind of generic kernel in normal use, so I'd like to
see as many unnecessary workarounds removed from the code as possible
at compile time, and appropriate compiler optimisations for only the
specific CPU the kernel is destined for.  That maximises kernel
performance on that mahcine.  On the other hand, if I'm working on an
embedded project with a 386 or a 486, I'm usually running the same
environment on a more powerful box as well, for testing purposes, so I
need workarounds for the, (E.G. Athlon), CPU in the development box,
but I don't want performance optimisations for that faster CPU,
especially if they have a negative effect on the embedded CPU.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> >>>>>That's a non-issue.  300 bytes matters a lot on some systems.  The
> >>>>>fact that there are drivers that are bloated is nothing to do with
> >>>>>it.
> >>>>>
> >>>>>
> >>>>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
> >>>>alignment to all the cache friendly structure padding.
> >>>>
> >>>>
> >>>My intention is that we won't have done 128 byte alignments just by
> >>>'supporting' Athlons, only if we want to run fast on Athlons.  A
> >>>distribution kernel that is intended to boot on all CPUs needs
> >>>workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
> >>>
> >>>Obviously using such a kernel for anything other than getting a system
> >>>up and running to compile a better kernel is a Bad Thing, but the
> >>>distributions could supply separate Athlon, PIV, and 386 _optimised_
> >>>kernels.
> >>>
> >>>
> >>Why bother with that complexity? Just use 128 byte lines. This allows
> >>a decent generic kernel. The people who have space requirements would
> >>only compile what they need anyway.
> >>
> >
> >So, basically, if you compile a kernel for a 386, but think that maybe
> >one day you might need to run it on an Athlon for debugging purposes,
> >you use 128 byte padding, because it's not too bad on the 386?  Seems
> >pretty wasteful to me when the obvious, simple, elegant solution is to
> >allow independent selection of workaround inclusion and optimisation.
> >Especially since half of the work has already been done.
> >
>
> I missed the "simple, elegant" part. Conceptually elegant maybe.
>
> If you mean to use the optimise option only to set cache line size, then
> that might be a bit saner.
>
> As far as the case study goes though: if you were worried about being
> wasteful, why wouldn't you compile just for the 386 and debug from that?

In the model I'm proposing, the 386 kernel would be missing the Athlon
workarounds.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

John Bradford wrote:

>>>>>>>That's a non-issue.  300 bytes matters a lot on some systems.  The
>>>>>>>fact that there are drivers that are bloated is nothing to do with
>>>>>>>it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
>>>>>>alignment to all the cache friendly structure padding.
>>>>>>
>>>>>>
>>>>>>
>>>>>My intention is that we won't have done 128 byte alignments just by
>>>>>'supporting' Athlons, only if we want to run fast on Athlons.  A
>>>>>distribution kernel that is intended to boot on all CPUs needs
>>>>>workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
>>>>>
>>>>>Obviously using such a kernel for anything other than getting a system
>>>>>up and running to compile a better kernel is a Bad Thing, but the
>>>>>distributions could supply separate Athlon, PIV, and 386 _optimised_
>>>>>kernels.
>>>>>
>>>>>
>>>>>
>>>>Why bother with that complexity? Just use 128 byte lines. This allows
>>>>a decent generic kernel. The people who have space requirements would
>>>>only compile what they need anyway.
>>>>
>>>>
>>>So, basically, if you compile a kernel for a 386, but think that maybe
>>>one day you might need to run it on an Athlon for debugging purposes,
>>>you use 128 byte padding, because it's not too bad on the 386?  Seems
>>>pretty wasteful to me when the obvious, simple, elegant solution is to
>>>allow independent selection of workaround inclusion and optimisation.
>>>Especially since half of the work has already been done.
>>>
>>>
>>I missed the "simple, elegant" part. Conceptually elegant maybe.
>>
>>If you mean to use the optimise option only to set cache line size, then
>>that might be a bit saner.
>>
>>As far as the case study goes though: if you were worried about being
>>wasteful, why wouldn't you compile just for the 386 and debug from that?
>>
>
>In the model I'm proposing, the 386 kernel would be missing the Athlon
>workarounds.
>

No, debug the kernel while its running on the 386. And what of my other
concerns?

1. It doesn't appear to be simple and elegant.
2. It would drive developers nuts if it was used for anything other than
   a couple of critical functions (cache size would be one).
3. Are there valid situations where you would need it? This isn't a
   rhetorical question. Your example would be fine if somebody really
   needed to do that.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

John Bradford wrote:

>>>That's a non-issue.  300 bytes matters a lot on some systems.  The
>>>fact that there are drivers that are bloated is nothing to do with
>>>it.
>>>
>>Its kind of irrelevant when by saying "Athlon" you've added 128 byte
>>alignment to all the cache friendly structure padding.
>>
>
>My intention is that we won't have done 128 byte alignments just by
>'supporting' Athlons, only if we want to run fast on Athlons.  A
>distribution kernel that is intended to boot on all CPUs needs
>workarounds for Athlon bugs, but it doesn't need 128 byte alignment.
>
>Obviously using such a kernel for anything other than getting a system
>up and running to compile a better kernel is a Bad Thing, but the
>distributions could supply separate Athlon, PIV, and 386 _optimised_
>kernels.
>

Why bother with that complexity? Just use 128 byte lines. This allows
a decent generic kernel. The people who have space requirements would
only compile what they need anyway.

>
>>There are systems
>>where memory matters, but spending a week chasing 300 bytes when you can
>>knock out 50K is a waste of everyones time. Do the 40K problems first
>>
>
>The 'select a single CPU to support and/optimise for' -> 'select a
>bitmap of CPUs to support' work is being done anyway though, so this
>is more or less just one single IFDEF, which is more like a few
>seconds work, rather than a week.
>

While I like Adrian's patch a lot from a functionality and user
simplicity point of view, the key to getting it merged is not to 
increase the complexity of the implementation. The only objections to
the concept came from people who didn't understand it AFAIK.

And too many combinations of ill defined things like this will make 
people (developers) go nuts.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> > That's a non-issue.  300 bytes matters a lot on some systems.  The
> > fact that there are drivers that are bloated is nothing to do with
> > it.
>
> Its kind of irrelevant when by saying "Athlon" you've added 128 byte
> alignment to all the cache friendly structure padding.

My intention is that we won't have done 128 byte alignments just by
'supporting' Athlons, only if we want to run fast on Athlons.  A
distribution kernel that is intended to boot on all CPUs needs
workarounds for Athlon bugs, but it doesn't need 128 byte alignment.

Obviously using such a kernel for anything other than getting a system
up and running to compile a better kernel is a Bad Thing, but the
distributions could supply separate Athlon, PIV, and 386 _optimised_
kernels.

> There are systems
> where memory matters, but spending a week chasing 300 bytes when you can
> knock out 50K is a waste of everyones time. Do the 40K problems first

The 'select a single CPU to support and/optimise for' -> 'select a
bitmap of CPUs to support' work is being done anyway though, so this
is more or less just one single IFDEF, which is more like a few
seconds work, rather than a week.

Also, a lot of the 40K problems are in drivers that embedded systems
simply don't use.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Llu, 2003-09-15 at 04:55, Bill Davidsen wrote:
> 1 - the code is not needed for Athlon, prefetch is turned off on broken
>     CPUs now. A generic kernel runs fine on Athlon.

That disable you talk about is bloat. It also trashes the performance of
PIV boxes. In fact I checked out of interest - the disable hack
currently being used is adding *over* 300 bytes to my kernel as its
inlined repeatedly. So its larger, and it ruins performance for all
processors.

You also need it for userspace prefetch fault fixup for a kernel without
CONFIG_MK7 to run stuff perfectly on Athlon.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Llu, 2003-09-15 at 07:32, John Bradford wrote:
> That's a non-issue.  300 bytes matters a lot on some systems.  The
> fact that there are drivers that are bloated is nothing to do with
> it.

Its kind of irrelevant when by saying "Athlon" you've added 128 byte
alignment to all the cache friendly structure padding. There are systems
where memory matters, but spending a week chasing 300 bytes when you can
knock out 50K is a waste of everyones time. Do the 40K problems first

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> > We just got a start on making Linux smaller to encourage embedded use, I
> > don't see adding 300+ bytes of wasted code so people can run
> > misconfigured kernels.
> > 
> > I rather have to patch this in for my Athlon kernels than have people
> > who aren't cutting corners trying to avoid building matching kernels
> > have to live with the overhead.
>
> Overhead? Really you could save more memory by cleaning up a lot of 
> drivers. Andi already said it before, there are better places to be 
> looking at.

That's a non-issue.  300 bytes matters a lot on some systems.  The
fact that there are drivers that are bloated is nothing to do with
it.

> Also 'patching' for Athlon kernels doesn't cut it for people who need to 
> distribute kernels which run on various hardware (such as distros). This 
> alone is benefit enough to justify this supposed 'bloat'.

No it's not.  Most distributions heavily patch the kernel anyway.

It should be possible, and straightforward, to compile a kernel which:

1. Supports, (I.E. has workarounds for), any combination of CPUs.
   E.G. a kernel which supports 386s, and Athlons _only_ would not
   need the F00F bug workaround.  Currently '386' kernels include it,
   because '386' means 'support 386 and above processors'.

2. Has compiler optimisations for one particular CPU.
   E.G. the 386 and Athlon supporting kernel above could have
   alignment optimised for either 386 or Athlon.

This makes it trivial to:

* Make a kernel for a distribution's initial install
  Select all CPUs as supported, and optimise for 686.

* Make an optimised kernel for any system
  Select only the target CPU as supported, and optimise for it

* Make a generic kernel for PIV, and Athlon
  Select PIV and Athlon only as supported.  Optimise for either, or
  optimise for 386, (yes, even though it is not supported), for a
  small kernel, on the basis that it will maximise cache usage, and be
  fairly optimal on both systems.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Bill Davidsen]

On Sun, 14 Sep 2003, Zwane Mwaikambo wrote:

> On Sun, 14 Sep 2003, bill davidsen wrote:
> 
> > We just got a start on making Linux smaller to encourage embedded use, I
> > don't see adding 300+ bytes of wasted code so people can run
> > misconfigured kernels.
> > 
> > I rather have to patch this in for my Athlon kernels than have people
> > who aren't cutting corners trying to avoid building matching kernels
> > have to live with the overhead.
> 
> Overhead? Really you could save more memory by cleaning up a lot of 
> drivers. Andi already said it before, there are better places to be 
> looking at.

The best way to remove overhead is not to add it. Putting in 300+ bytes of
code which is pure overhead on anything but an Athlon is silly. And to
justify it because it save effort for people who don't want to bother
building the correct distribution is pure hubris. The Athlon is not so
important that making it run a tiny bit faster justifies taking extra
space on every machine which doesn't benefit.
> 
> Also 'patching' for Athlon kernels doesn't cut it for people who need to 
> distribute kernels which run on various hardware (such as distros). This 
> alone is benefit enough to justify this supposed 'bloat'.

You clearly think that a kernel will not run on Athlon without this
patch. In real life kernel built for 386, 486, Ppro, etc will run
nicely on every CPU except those less featureful, including the
precious Athlon.

That being the case, there is no justification for forcing the code on
every build rather than putting ifdefs around it. Any vendor who wants
to make Athlon a tiny bit faster at the expense of making every other
system use more memory for code which is never executed can just use the
config option to include the support.

Let's try the facts agin:
1 - the code is not needed for Athlon, prefetch is turned off on broken
    CPUs now. A generic kernel runs fine on Athlon.
2 - the discussion is not on having the code or not, but on putting
    ifdefs around this code so it is only generated if wanted.

Let's not continue to pollute the kernel with architecture dependent
code, we have subtrees for that.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nick Piggin]

Zwane Mwaikambo wrote:

>On Sun, 14 Sep 2003, bill davidsen wrote:
>
>
>>We just got a start on making Linux smaller to encourage embedded use, I
>>don't see adding 300+ bytes of wasted code so people can run
>>misconfigured kernels.
>>
>>I rather have to patch this in for my Athlon kernels than have people
>>who aren't cutting corners trying to avoid building matching kernels
>>have to live with the overhead.
>>
>
>Overhead? Really you could save more memory by cleaning up a lot of 
>drivers. Andi already said it before, there are better places to be 
>looking at.
>
>Also 'patching' for Athlon kernels doesn't cut it for people who need to 
>distribute kernels which run on various hardware (such as distros). This 
>alone is benefit enough to justify this supposed 'bloat'.
>

Hi Zwane,

I still don't mind Adrian's patch (at least the concept). Its true, the
Athlon workaround is insignificant, but Adrian's patch allows the
possibility of compiling things like it out. Its also clearer to me than
the current scheme.

The only objections I have seen are from those who don't understand what
it does or implmentation issues. It might be the case that it can't be
done "nicely" without more support from kconfig and/or kbuild though.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Zwane Mwaikambo]

On Sun, 14 Sep 2003, bill davidsen wrote:

> We just got a start on making Linux smaller to encourage embedded use, I
> don't see adding 300+ bytes of wasted code so people can run
> misconfigured kernels.
> 
> I rather have to patch this in for my Athlon kernels than have people
> who aren't cutting corners trying to avoid building matching kernels
> have to live with the overhead.

Overhead? Really you could save more memory by cleaning up a lot of 
drivers. Andi already said it before, there are better places to be 
looking at.

Also 'patching' for Athlon kernels doesn't cut it for people who need to 
distribute kernels which run on various hardware (such as distros). This 
alone is benefit enough to justify this supposed 'bloat'.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030912213016.47a4e5de.ak@suse.de>,
Andi Kleen  <ak@suse.de> wrote:
| On Fri, 12 Sep 2003 21:05:06 +0200
| Martin Schlemmer <azarah@gentoo.org> wrote:
| 
| > I have long wondered if everything in arch/i386/kernel/cpu/ is
| > really linked in (meaning with no #ifdef as it now looks to be
| > at a quick peek), or if it was just easier to link them all,
| > but have non generic stuff (amd/cyrix/whatever specific code)
| > filtered by ifdef's.
| 
| It is (in MTRR drivers etc.), but the resulting overhead is small.
| 
| Currently I even link in Intel and Cyrix specific MTRR drivers on x86-64
| just to keep the common code common and clean.
| 
| Really, when you want to save code size you should look elsewhere. All the 
| CPU support code is pretty lean and in many cases is __init code anyways
| (= is discarded after boot time) 

If you want lean code you should look everywhere. I agree that init code
is free, and that there are worse offenders. But adding additional bloat
that the people who actually builds a kernel properly to fit their
hardware can't eliminate without patching, then perhaps that shouldn't
happen.

I'm not at all against having an option in the menu to "eliminate code
to support non-targeted processors." That could remove the code you so
want to have in so you don't have to build a properly configured kernel
for each machine.

How does "#if !defined(NO_BLOAT) || WANT_ARCH_IN_QUESTION feel for a
good thing to surround many parts of the kernel.

| 
| I can offer my old bloat-o-meter tool (ftp://ftp.firstfloor.org/pub/ak/perl/bloat-o-meter)
| It is great to look for bloat. Just run it with a 2.4 kernel and 2.6 kernel and compare
| the results. Then look at the top 50 bloat increasers. I bet with you that 
| you won't find anything touched in this thread among them.

The term "grandfathered" comes to mind, you are proposing to add new
bloat, and you justify that so you don't have to build a properly
configures kernel for each hardware configuration. I don't think anyone
will argue that all such code should be found and treated in the near
future, but if we did have a flag to prevent eggregious support for
non-selected hardware, I bet the embedded guys would contribute patches
over time to clean things up. And shouldn't 2.6 better than 2.4, instead
of "not much worse?"
| 
| I suspect for example if you just worked on making sysfs optional you would
| save a lot more.

As long as you don't have to add new code to support the functionality
in another way. The advantage is that if you can remove it you can
probably also make it modular, and that might be a nice thing during
development of embedded systems. Somewhat smaller but available?
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030912202851.3529e7e7.ak@suse.de>,
Andi Kleen  <ak@suse.de> wrote:
| On Fri, 12 Sep 2003 20:22:16 +0200
| Adrian Bunk <bunk@fs.tum.de> wrote:
| 
| 
| > 
| > But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
| > compiled for Athlon wouldn't run on a Pentium 4 even with 
| > CONFIG_X86_GENERIC.
| 
| It does. Just try it.
| 
| > 
| > Quoting arch/i386/Kconfig in -test5:
| > 
| > <--  snip  -->
| > 
| > config X86_USE_3DNOW
| >         bool
| >         depends on MCYRIXIII || MK7
| >         default y
| 
| That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.

And if that isn't all in init it's another good target for getting rid
of bloat. You suggested people look for other places, here's one.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030912182216.GK27368@fs.tum.de>,
Adrian Bunk  <bunk@fs.tum.de> wrote:
                [...]

| But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
| compiled for Athlon wouldn't run on a Pentium 4 even with 
| CONFIG_X86_GENERIC.
| 
| Quoting arch/i386/Kconfig in -test5:
| 
| <--  snip  -->
| 
| config X86_USE_3DNOW
|         bool
|         depends on MCYRIXIII || MK7
|         default y
| 
| <--  snip  -->
| 
| My patch in the mail
| 
|   RFC: [2.6 patch] better i386 CPU selection
| 
| tries to solve these problem with a different approach (the user selects 
| all CPUs he wants to support).

If you have managed to work around all the conflicting capabilities
without leaving the kernel way suboptimal on some, I salute your better
solution.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030912195606.24e73086.ak@suse.de>,
Andi Kleen  <ak@suse.de> wrote:
| On 12 Sep 2003 11:32:42 -0600
| ebiederm@xmission.com (Eric W. Biederman) wrote:
| 
| 
| > There may be better places to attack.  But new code is what is up for
| > examination and is easiest to fix.
| 
| With is_prefetch:
| 
|    text    data     bss     dec     hex filename
|    2782       4       0    2786     ae2 arch/i386/mm/fault.o
| 
| Without is_prefetch:
| 
|  text    data     bss     dec     hex filename
|    2446       4       0    2450     992 arch/i386/mm/fault.o
| 
| Difference 332 bytes
| 
| If you start your attack on 332 bytes then IMHO you have your priorities wrong ;-)
| 
| The main reason I'm really against this is that currently the P4 kernels work
| fine on Athlon. Just when is_prefetch is not integrated in them there will 
| be an mysterious oops once every three months in the kernel in prefetch
| on Athlon.
|  
| That would be bad. The alternative would be to prevent the P4 kernel
| from booting on the Athlon at all, but doing that for 332 bytes
| would seem a bit silly.

I am really missing something here, why is it you want to run a P4
kernel on Athlon? And why is it good to push bloat into the kernel and
then tell people who really care about size to go peddle their papers
elsewhere? There is perfectly good code in the kernel now to disable
prefetch on Athlon, leave it in unless the kernel is built for Athlon
support. A P4 kernel should run fine on an Athlon already.

We just got a start on making Linux smaller to encourage embedded use, I
don't see adding 300+ bytes of wasted code so people can run
misconfigured kernels.

I rather have to patch this in for my Athlon kernels than have people
who aren't cutting corners trying to avoid building matching kernels
have to live with the overhead.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Pavel Machek]

Hi!

> > >  > > What's wrong with the current status quo that just says 
> > >  > > "Athlon prefetch is broken"?
> > >  > It doesn't fix user space for once.
> > > 
> > > And for another, it cripples the earlier athlons which 
> > > don't have this
> > > errata. Andi's fix at least makes prefetch work again on 
> > > those boxes.
> > > It's also arguable that prefetch() helps the older K7's 
> > > more than the
> > > affected ones.
> > 
> > All Athlons have this Errata. I can trigger it on an old
> > 900Mhz pre XP Athlon too. You just have to use 3dnow prefetch
> > instead of SSE prefetch.
> > 
> > BTW the older Athlons currently don't use prefetch because 
> > the alternative
> > patcher does not handle 3dnow style prefetch.
> > 
> 
> Avoiding prefetch for all Athlons and earlier Opterons/Athlon64
> even in the kernel can really tank performance. And as Andi says
> it still doesn't solve user mode from hitting the errata.

How much speedups can be expected from prefetch? 5%?
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[John Bradford]

> From linux-kernel-owner+john=40bradfords.org.uk@vger.kernel.org  Fri Sep 12 21:59:55 2003
> Envelope-To: john@bradfords.org.uk
> Date: 	Fri, 12 Sep 2003 22:00:23 +0200
> From: Adrian Bunk <bunk@fs.tum.de>
> To: Andi Kleen <ak@suse.de>
> Cc: Martin Schlemmer <azarah@gentoo.org>, jgarzik@pobox.com,
>    ebiederm@xmission.com, akpm@osdl.org, richard.brunner@amd.com,
>    linux-kernel@vger.kernel.org, torvalds@osdl.org
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> References: <3F60837D.7000209@pobox.com> <20030911162634.64438c7d.ak@suse.de> <3F6087FC.7090508@pobox.com> <m1vfrxlxol.fsf@ebiederm.dsl.xmission.com> <20030912195606.24e73086.ak@suse.de> <3F62098F.9030300@pobox.com> <20030912182216.GK27368@fs.tum.de> <20030912202851.3529e7e7.ak@suse.de> <1063393505.3371.207.camel@workshop.saharacpt.lan> <20030912213016.47a4e5de.ak@suse.de>
> Mime-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> In-Reply-To: <20030912213016.47a4e5de.ak@suse.de>
> User-Agent: Mutt/1.4.1i
> Sender: linux-kernel-owner@vger.kernel.org
> Precedence: bulk
> X-Mailing-List: 	linux-kernel@vger.kernel.org
>
> On Fri, Sep 12, 2003 at 09:30:16PM +0200, Andi Kleen wrote:
> >...
> > I think it's useful to keep kernels booting everywhere, it makes it a lot easier
> > to test a single kernel on multiple systems.
>
> Different people have different needs:
>
> Sometimes you want kernels booting everywhere, e.g. a distribution might
> want to support all CPUs from an 386 to an Opteron with one kernel for
> their boot floppies.
>
> For a system administrator with only Pentium 3 and Pentum 4 machines
> support for 386 and Opteron isn't of much worth.
>
> In some embedded systems people are happy about every kB their kernel is 
> smaller.

As I understand it:

* For maximum optimisation, compile for your specific CPU.
* To get compatibility for everything except a 386 CPU, with almost
  optimum performance, compile for 486.
* To get full compatibility, with possibly significantly less than
  optimum performance, compile for 386.
* Only optimise for Pentium if the kernel is intended to be run on a
  Pentium CPU, as the resulting code is less optimal for many other
  processors than code optimised for a 486.

Recently, there have been efforts to make a universally compatible
kernel, with optimisations for all CPUs that don't significantly
adversely affect any other CPUs.  The logic behind this seems to be
that users can then use their distribution's kernel on any CPU, and
get reasonable performance.

Nothing wrong with that in principle, but there are also a lot of
users who are happy to compile one kernel for one machine, and want it
as optimised as possible.  This often includes embedded systems
developers, but certainly isn't limited to them.

John.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Adrian Bunk]

On Fri, Sep 12, 2003 at 09:30:16PM +0200, Andi Kleen wrote:
>...
> I think it's useful to keep kernels booting everywhere, it makes it a lot easier
> to test a single kernel on multiple systems.

Different people have different needs:

Sometimes you want kernels booting everywhere, e.g. a distribution might
want to support all CPUs from an 386 to an Opteron with one kernel for
their boot floppies.

For a system administrator with only Pentium 3 and Pentum 4 machines
support for 386 and Opteron isn't of much worth.

In some embedded systems people are happy about every kB their kernel is 
smaller.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Martin Schlemmer]

On Fri, 2003-09-12 at 21:30, Andi Kleen wrote:
> On Fri, 12 Sep 2003 21:05:06 +0200
> Martin Schlemmer <azarah@gentoo.org> wrote:

> Ok, so how many instructions was added by this ?  Or is it
> 
> None at all, Mr Inquisitor. It is all patched at early boot time.
> 

Thanks :P

> > Ok, so maybe my opinion about X86_GENERIC is not as intended, but
> > then IMHO, it should be 'fixed'.  I could not care less if my kernel
>
> X86_GENERIC has nothing to do with all this. All it does is 
> to always force the cache line padding to 128 byte. 
> 

Ok, thanks

>  
> > I have long wondered if everything in arch/i386/kernel/cpu/ is
> > really linked in 
> 
> It is (in MTRR drivers etc.), but the resulting overhead is small.
> 
> Really, when you want to save code size you should look elsewhere. All the 
> CPU support code is pretty lean and in many cases is __init code anyways
> (= is discarded after boot time) 
> 
> I can offer my old bloat-o-meter tool (ftp://ftp.firstfloor.org/pub/ak/perl/bloat-o-meter)


I am not really bothered about code size, as I tried (?) to say.

>  
> > This is just me, but why then don't we then just drop the specific
> > arch selection, and just have generics instead of pulling a sock
> > over the user's eyes ?
> 
> It is doing a lot of optimizations for the specific CPU. For example
> it tells gcc to compile for that CPU which can make a big difference (P4 prefers
> very different code compared to P3 or Athlon). Or it sets the paddings correctly
> for the CPU, which can make a very big difference in .text size. So when you
> select CONFIG_MPENTIUM4 you will get a kernel that will perform optimally for P4.
> 

> Then 2.6 added SSE1 prefetch support which made the P4 kernel not boot on
> anything that didn't support SSE1 (like older Athlon before XP). I fixed
> that then with dynamically patching the prefetches. The overhead at runtime
> is zero because it is patched at boot, the .text overhead for the patch 
> tables is minimal.
> 

Ok, thanks.

> So basically the 2.6 alternative() stuff just restored the 2.4 de-facto situation 
> in 2.6, and improved it slightly because the Athlon kernel also now works everywhere.
> 
> I think it's useful to keep kernels booting everywhere, it makes it a lot easier
> to test a single kernel on multiple systems.
> 

Yes, given.  I just don't think most people know or look at it the
same.  I for one usually just try not to add anything not needed
at the cost of some minor speed regression, as things many times
especially in a big project sometimes tends to get out of hand,
as all those minor regressions added is one big regression.  I do
however admit that I will be a bit out of my league trying to
judge in this case, but it still would be interesting ....

I however will not keep this up if you guys say its is fine.
Two things however does still does bother:

1)  What will it look like after being put through some benchmarks
just to verify.  I may however just need to get to bed early for
a change =)

2)  Will it be that difficult to also patch it in at boot like
the rest.  Once again it might just be paranoia from my side 8)


Thanks for the more in depth info.

Regards,

-- 
Martin Schlemmer

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Fri, 12 Sep 2003 21:05:06 +0200
Martin Schlemmer <azarah@gentoo.org> wrote:


> Ok, so how many instructions was added by this ?  Or is it

None at all, Mr Inquisitor. It is all patched at early boot time.

> Ok, so maybe my opinion about X86_GENERIC is not as intended, but
> then IMHO, it should be 'fixed'.  I could not care less if my kernel
> only boot just on my box, never mind on another P4 - I just want
> the most optimised on possible.  Sure, some guys want a more generic
> kernel - get X86_GENERIC to work for them.  Same for distro's.

X86_GENERIC has nothing to do with all this. All it does is 
to always force the cache line padding to 128 byte. 

This means that when you have an SMP kernel, no matter compiled
for what CPU, it will not run like crap on a P4 Xeon based SMP system.
The cost is some more memory usage for more padding (Athlon is fine 
with 64 byte padding, P3 is fine with 32byte padding). If you don't
want that just don't enable it.
 
> I have long wondered if everything in arch/i386/kernel/cpu/ is
> really linked in (meaning with no #ifdef as it now looks to be
> at a quick peek), or if it was just easier to link them all,
> but have non generic stuff (amd/cyrix/whatever specific code)
> filtered by ifdef's.

It is (in MTRR drivers etc.), but the resulting overhead is small.

Currently I even link in Intel and Cyrix specific MTRR drivers on x86-64
just to keep the common code common and clean.

Really, when you want to save code size you should look elsewhere. All the 
CPU support code is pretty lean and in many cases is __init code anyways
(= is discarded after boot time) 

I can offer my old bloat-o-meter tool (ftp://ftp.firstfloor.org/pub/ak/perl/bloat-o-meter)
It is great to look for bloat. Just run it with a 2.4 kernel and 2.6 kernel and compare
the results. Then look at the top 50 bloat increasers. I bet with you that 
you won't find anything touched in this thread among them.

I suspect for example if you just worked on making sysfs optional you would
save a lot more.
 
> This is just me, but why then don't we then just drop the specific
> arch selection, and just have generics instead of pulling a sock
> over the user's eyes ?

It is doing a lot of optimizations for the specific CPU. For example
it tells gcc to compile for that CPU which can make a big difference (P4 prefers
very different code compared to P3 or Athlon). Or it sets the paddings correctly
for the CPU, which can make a very big difference in .text size. So when you
select CONFIG_MPENTIUM4 you will get a kernel that will perform optimally for P4.

To give a bit of perspective:

On 2.4 the situation was: 

- Athlon kernel would only boot on Athlon/K6 due to 3dnow prefetches
- P4 kernel would boot everywhere
- P3 kernel would boot everywhere

(ignoring really old CPUs or oddballs like C3 without CMOV support) 

(also note booting just means booting without oops, not being optimal for the specific CPU.
Being optimal is very different)

Then 2.6 added SSE1 prefetch support which made the P4 kernel not boot on
anything that didn't support SSE1 (like older Athlon before XP). I fixed
that then with dynamically patching the prefetches. The overhead at runtime
is zero because it is patched at boot, the .text overhead for the patch 
tables is minimal.

So basically the 2.6 alternative() stuff just restored the 2.4 de-facto situation 
in 2.6, and improved it slightly because the Athlon kernel also now works everywhere.

I think it's useful to keep kernels booting everywhere, it makes it a lot easier
to test a single kernel on multiple systems.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Fri, 12 Sep 2003 20:48:01 +0200
Adrian Bunk <bunk@fs.tum.de> wrote:


> > That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.
> 
> Quoting e.g. arch/i386/lib/memcpy.c:
> 
> <--  snip  -->
> 
> void * memcpy(void * to, const void * from, size_t n)
> {
> #ifdef CONFIG_X86_USE_3DNOW
>         return __memcpy3d(to, from, n);
> #else
>         return __memcpy(to, from, n);
> #endif


No, it really works. The "3d" copy code uses actually MMX, which both the P4 and the K7 support fine

The only 3dnow! thing in there is that it uses 3dnow style prefetches (original MMX didn't 
have prefetches), but these are handled in a transparent way since a long time. The code just 
has an exception handler and patches them away if the prefetch ever faulted with an illegal 
instruction:


arch/i386/lib/mmx.c:

...
        __asm__ __volatile__ (
                "1: prefetch (%0)\n"            /* This set is 28 bytes */
                "   prefetch 64(%0)\n"
                "   prefetch 128(%0)\n"
                "   prefetch 192(%0)\n"
                "   prefetch 256(%0)\n"
                "2:  \n"
                ".section .fixup, \"ax\"\n"
                "3: movw $0x1AEB, 1b\n" /* jmp on 26 bytes */
                "   jmp 2b\n"
                ".previous\n"

I admit the remaining #ifdefs and comments are a bit misleading though.


-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Martin Schlemmer]

On Fri, 2003-09-12 at 20:28, Andi Kleen wrote:
> On Fri, 12 Sep 2003 20:22:16 +0200
> Adrian Bunk <bunk@fs.tum.de> wrote:
> 
> 
> > 
> > But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
> > compiled for Athlon wouldn't run on a Pentium 4 even with 
> > CONFIG_X86_GENERIC.
> 
> It does. Just try it.
> 
> > 
> > Quoting arch/i386/Kconfig in -test5:
> > 
> > <--  snip  -->
> > 
> > config X86_USE_3DNOW
> >         bool
> >         depends on MCYRIXIII || MK7
> >         default y
> 
> That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.
> 

Ok, so how many instructions was added by this ?  Or is it
just in the init code ?  What else just add 'just another
one or two instructions' to common paths because of this?

Which ever way, the point I and some of the others (besides the
additional one from the embedded guys) want to make, is if I
select the CPU to be Pentium4, it means I want a kernel that is
optimised for my P4, without extra crap that I do not need.  Sure,
its an extra instruction here, two there, etc - but when will it
be too much ?  Is this not maybe the fabled 'slight slowdown' that
so many  people complain about from round the 2.5.[67]'s ?

Ok, so maybe my opinion about X86_GENERIC is not as intended, but
then IMHO, it should be 'fixed'.  I could not care less if my kernel
only boot just on my box, never mind on another P4 - I just want
the most optimised on possible.  Sure, some guys want a more generic
kernel - get X86_GENERIC to work for them.  Same for distro's.

I have long wondered if everything in arch/i386/kernel/cpu/ is
really linked in (meaning with no #ifdef as it now looks to be
at a quick peek), or if it was just easier to link them all,
but have non generic stuff (amd/cyrix/whatever specific code)
filtered by ifdef's.

This is just me, but why then don't we then just drop the specific
arch selection, and just have generics instead of pulling a sock
over the user's eyes ?


Thanks,

-- 
Martin Schlemmer

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Adrian Bunk]

On Fri, Sep 12, 2003 at 08:28:51PM +0200, Andi Kleen wrote:
> On Fri, 12 Sep 2003 20:22:16 +0200
> Adrian Bunk <bunk@fs.tum.de> wrote:
> 
> 
> > 
> > But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
> > compiled for Athlon wouldn't run on a Pentium 4 even with 
> > CONFIG_X86_GENERIC.
> 
> It does. Just try it.
> 
> > 
> > Quoting arch/i386/Kconfig in -test5:
> > 
> > <--  snip  -->
> > 
> > config X86_USE_3DNOW
> >         bool
> >         depends on MCYRIXIII || MK7
> >         default y
> 
> That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.

Quoting e.g. arch/i386/lib/memcpy.c:

<--  snip  -->

void * memcpy(void * to, const void * from, size_t n)
{
#ifdef CONFIG_X86_USE_3DNOW
        return __memcpy3d(to, from, n);
#else
        return __memcpy(to, from, n);
#endif
}

<--  snip  -->

This is hardly dynamic.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
>>config X86_USE_3DNOW
>>        bool
>>        depends on MCYRIXIII || MK7
>>        default y
> 
> 
> All 3dnow! code is dynamically patched depending on the CPUID.


Note that the people who care most about x86 kernel size, such as people 
running on brand new 486 and 586 embedded processors (AMD Elan, ...) are 
precisely the people that don't need the "convenience" of having the 
feature dynamically patched into the kernel.

Removal of CONFIG_SSE2 was wrong for this same reason.

300 bytes here, 300 bytes there, and all this code is adding up quick.

We have CONFIG_EMBEDDED to hide such things from your sight, so you 
don't even have to worry about them...  they will always be dynamically 
patched in your kernels :)

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
> That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.


Ug.  Why compile in 3dnow code when you don't need it?  There is a 
sizeable number of processors without 3dnow...

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Fri, 12 Sep 2003 20:22:16 +0200
Adrian Bunk <bunk@fs.tum.de> wrote:


> 
> But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
> compiled for Athlon wouldn't run on a Pentium 4 even with 
> CONFIG_X86_GENERIC.

It does. Just try it.

> 
> Quoting arch/i386/Kconfig in -test5:
> 
> <--  snip  -->
> 
> config X86_USE_3DNOW
>         bool
>         depends on MCYRIXIII || MK7
>         default y

That's obsolete and could be removed. All 3dnow! code is dynamically patched depending on the CPUID.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Adrian Bunk]

On Fri, Sep 12, 2003 at 01:59:43PM -0400, Jeff Garzik wrote:
> Andi Kleen wrote:
> >The main reason I'm really against this is that currently the P4 kernels 
> >work
> >fine on Athlon. Just when is_prefetch is not integrated in them there will 
> >be an mysterious oops once every three months in the kernel in prefetch
> >on Athlon.
> 
> 
> Booting a P4 kernel _without_ CONFIG_X86_GENERIC on an Athlon would be a 
> user bug.

But even CONFIG_X86_GENERIC doesn't do what you expect. A kernel 
compiled for Athlon wouldn't run on a Pentium 4 even with 
CONFIG_X86_GENERIC.

Quoting arch/i386/Kconfig in -test5:

<--  snip  -->

config X86_USE_3DNOW
        bool
        depends on MCYRIXIII || MK7
        default y

<--  snip  -->

My patch in the mail

  RFC: [2.6 patch] better i386 CPU selection

tries to solve these problem with a different approach (the user selects 
all CPUs he wants to support).

> 	Jeff

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Mike Fedyk]

On Fri, Sep 12, 2003 at 11:32:42AM -0600, Eric W. Biederman wrote:
> The size of a minimal bzImage (the stuff you can't compile out) has
> increased by roughly 400KB since 1.0 200KB since 2.2 and 100KB since 2.4.
> 
> So please pardon those of us who complain at things that can't be
> configured out.  The x86 kernel is on the edge of becoming useless
> in some embedded scenarios because it is so fat.
> 
> When we can compile out code, we can at least narrow down where the
> problems are.
> 
> I have to agree with Jeff the LinuxBIOS stuff is  crippled right now
> because of this.
> 
> There may be better places to attack.  But new code is what is up for
> examination and is easiest to fix.

Yes, but is there enough framework to make this reasonable?

Is config generic with its new semantics (different from the origional
author) good enough for this?

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
> The main reason I'm really against this is that currently the P4 kernels work
> fine on Athlon. Just when is_prefetch is not integrated in them there will 
> be an mysterious oops once every three months in the kernel in prefetch
> on Athlon.


Booting a P4 kernel _without_ CONFIG_X86_GENERIC on an Athlon would be a 
user bug.

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On 12 Sep 2003 11:32:42 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:


> There may be better places to attack.  But new code is what is up for
> examination and is easiest to fix.

With is_prefetch:

   text    data     bss     dec     hex filename
   2782       4       0    2786     ae2 arch/i386/mm/fault.o

Without is_prefetch:

 text    data     bss     dec     hex filename
   2446       4       0    2450     992 arch/i386/mm/fault.o

Difference 332 bytes

If you start your attack on 332 bytes then IMHO you have your priorities wrong ;-)

The main reason I'm really against this is that currently the P4 kernels work
fine on Athlon. Just when is_prefetch is not integrated in them there will 
be an mysterious oops once every three months in the kernel in prefetch
on Athlon.
 
That would be bad. The alternative would be to prevent the P4 kernel
from booting on the Athlon at all, but doing that for 332 bytes
would seem a bit silly.

-Andi

P.S.: If you really want to shrink 2.6 I would start with making sysfs optional.
That would likely help much more than micro optimizing non bloated parts.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Eric W. Biederman]

Jeff Garzik <jgarzik@pobox.com> writes:

> >> If I disabled CONFIG_X86_GENERIC and select CONFIG_MPENTIUM4, I darned well
> >> better not get any Athlon code.  The cpu setup code in particular I want to
> >> conditionalize, and there are other bits that need work... but for the most
> >> part it works as intended.
> > Now that's becomming silly. It's alttogether only a few KB and all
> > __init code anyways.
> 
> 
> If you're doing crazy LinuxBIOS stuff where flash size is limited, it makes a
> lot of sense.  (and I do such crazy things)  The core 2.6 kernel has really
> bloated with optional features, IMO.

The size of a minimal bzImage (the stuff you can't compile out) has
increased by roughly 400KB since 1.0 200KB since 2.2 and 100KB since 2.4.

So please pardon those of us who complain at things that can't be
configured out.  The x86 kernel is on the edge of becoming useless
in some embedded scenarios because it is so fat.

When we can compile out code, we can at least narrow down where the
problems are.

I have to agree with Jeff the LinuxBIOS stuff is  crippled right now
because of this.

There may be better places to attack.  But new code is what is up for
examination and is easiest to fix.

Eric

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Martin Schlemmer]

On Thu, 2003-09-11 at 06:55, richard.brunner@amd.com wrote:
> Jun,
> 
> I have to agree with what Andi says. It is in a slow path,
> and we want to guard against user programs that could hit it.
> Making it conditional doesn't buy a lot and would cause lots of
> re-validation of the patch that we would like
> to avoid so we can get this in to the 2.6 kernel ASAP. 
> Don't worry! I am pretty certain the patch won't impact the 
> performance of the 2.6 kernel on processors from other vendors ;-)
> 

Would be nice if somebody with more knowledge about all the
benchmarks and a few non AMD processor machines could test
to see if its really that non critical (*hint* *hint*).  Yes,
I am not a kernel dev, but I do know that one or two slowdowns
in an already slow path is not that critical - problem now is
just that 5 or 6 might be an issue =)


Thanks,

-- 
Martin Schlemmer

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <1063313075.3881.4.camel@dhcp23.swansea.linux.org.uk>,
Alan Cox  <alan@lxorguk.ukuu.org.uk> wrote:
| On Iau, 2003-09-11 at 20:56, bill davidsen wrote:
| > | When we know at compile time it's not needed, it should not be enabled.
| > 
| > Clearly that's right. This buys nothing on CPUs which don't have the
| > problem, why have *any* overhead in size and speed? It's too bad that
| > people have to read around all that code, they don't need to give it a
| > home in their RAM and execute it as well.
| 
| We have always built kernels that contained the support for older chips.
| A 586 kernel for example is minutely slowed by the fact it will run on
| the Pentium Pro.
| 
| If someone wants to put in clear "this CPU only" stuff as well then
| fine, but with my distributor hat on I defy you to benchmark the
| difference in the real world between PIV and PIV with 100 bytes of extra
| workaround code.

I was not even thinking of CPU overhead, embedded applications benchmark
changes with "size" rather than "/bin/time" and this code is relatively
large and unlikely to be useful on any embedded system.

| You could get that much code back by spending 10 minutes tidying some
| random other piece of code you use, or shortening a couple of printk
| messages.

Perhaps for 2.7 a good case could be made for i18n on some of the
printk's, including some very small messages indeed for the embedded
folks. Most are not overly verbose now, but could be replaced by
abbreviations, or a magic message number.

I'm not against the fix, I do 5-6 hours a week on a K7-1400 original
Athlon, and I'll be running applications on some Duron boxen late this
year. I just find the work done to support embedded applications
evidence that 2.6 will be better than 2.6 without nearly as much
hacking. This code is large enough that a few ifdefs will have less
effect size on the kernel source than the lack of them will on the
binary.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Mike Fedyk]

On Thu, Sep 11, 2003 at 09:44:35PM +0100, Alan Cox wrote:
> On Iau, 2003-09-11 at 20:56, bill davidsen wrote:
> > | When we know at compile time it's not needed, it should not be enabled.
> > 
> > Clearly that's right. This buys nothing on CPUs which don't have the
> > problem, why have *any* overhead in size and speed? It's too bad that
> > people have to read around all that code, they don't need to give it a
> > home in their RAM and execute it as well.
> 
> We have always built kernels that contained the support for older chips.
> A 586 kernel for example is minutely slowed by the fact it will run on
> the Pentium Pro.
> 
> If someone wants to put in clear "this CPU only" stuff as well then
> fine, but with my distributor hat on I defy you to benchmark the
> difference in the real world between PIV and PIV with 100 bytes of extra
> workaround code.
> 
> You could get that much code back by spending 10 minutes tidying some
> random other piece of code you use, or shortening a couple of printk
> messages.

True, but then why have config options at all?

Why not just put this patch in as is, since it fits with the rest of the way
things are done now, and put it in the to do list for the kernel janitors to
split up the workarounds per cpu, and make config options so they can be
enabled for the specific cpu, and make the generic case just enable a bunch
of those individual cpu config options.

This change won't make it in 2.6, so it belongs in the to do until it has
action taken on it.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Iau, 2003-09-11 at 20:56, bill davidsen wrote:
> | When we know at compile time it's not needed, it should not be enabled.
> 
> Clearly that's right. This buys nothing on CPUs which don't have the
> problem, why have *any* overhead in size and speed? It's too bad that
> people have to read around all that code, they don't need to give it a
> home in their RAM and execute it as well.

We have always built kernels that contained the support for older chips.
A 586 kernel for example is minutely slowed by the fact it will run on
the Pentium Pro.

If someone wants to put in clear "this CPU only" stuff as well then
fine, but with my distributor hat on I defy you to benchmark the
difference in the real world between PIV and PIV with 100 bytes of extra
workaround code.

You could get that much code back by spending 10 minutes tidying some
random other piece of code you use, or shortening a couple of printk
messages.

 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030911162421.419f4432.ak@suse.de>,
Andi Kleen  <ak@suse.de> wrote:
| On Thu, 11 Sep 2003 15:14:02 +0100
| Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
| 
| > 
| > There is *one* change needed. We shouldnt call is_prefetch unless the
| > current CPU is an Athlon. That way its a performance improvement for
| > PIV, and Athlon in general, fixes the bug and causes no fancy athlon
| > fixup code for non AMD processors.
| 
| I considered that when writing the patch, but: is_prefetch is a single byte 
| memory access for something already in cache. Checking for an Athlon
| CPU needs two memory accesses in boot_cpu_data at least (checking vendor
| and model) 

You are still missing the point, what's needed is not a better way to do
useless work, it's a way to avoid doing it at all. This code should only
be built for CPU's which need it, no accesses needed except in .config.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <20030911165826.06f2fd16.ak@suse.de>,
Andi Kleen  <ak@suse.de> wrote:

| If you really want to save text space just use .bz2 compression 
| or compile the kernel with -Os. There are also other subsystems
| that would benefit much more (better effort/cost ratio) than adding
| micro #ifdefs to core code.

Good idea, let's put stuff like this in hardware-dependent includes, and
just have a single line in the core code  like
  check_special_pfault_cases;
and that documents what is happening as well as avoiding reading around
it. It seems silly to leave a big hunk of code in when developers are
making efforts to drop cruft and keep Linux practical for embedded
systems.

People were willing to drop the whole prefetch feature, I don't see that
micro ifdefs are a bad thing, it's just that thought needs to go into
making the code readable.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[bill davidsen]

In article <3F6087FC.7090508@pobox.com>,
Jeff Garzik  <jgarzik@pobox.com> wrote:
| Andi Kleen wrote:
| > It was not created for that (I know that because I created it ;-)
| 
| hehe
| 
| 
| > X86_GENERIC is merely an optimization hint (currently it only changes the cache
| > line size hint) It does not change anything related to correctness. Everything
| > that handles correctness is checked unconditionally.
| 
| When, building non-Pentium4-related code when CONFIG_MPENTIUM4 && 
| !CONFIG_X86_GENERIC, it's OK that the code is incorrect for (picking 
| example) AMD processors.
| 
| It would be a user bug to boot that on an AMD box, just like it would be 
| user bug to boot a CONFIG_M586 kernel on an ancient 386.
| 
| 
| > is_prefetch is a correctness thing.
| 
| When we know at compile time it's not needed, it should not be enabled.

Clearly that's right. This buys nothing on CPUs which don't have the
problem, why have *any* overhead in size and speed? It's too bad that
people have to read around all that code, they don't need to give it a
home in their RAM and execute it as well.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Andi Kleen wrote:
> There are a lot of conditional branches in the signal path. If you
> don't believe me I can send you simics full instruction traces of it.

Oh, I know.  The signal machinery is disturbingly branch-heavy.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 18:48:39 +0100
Jamie Lokier <jamie@shareable.org> wrote:

> Andi Kleen wrote:
> > signal exception path is thousands of cycles, we're talking about tens
> > of cycles here.
> 
> <hand-waving>
> 
> Tens vs thousands == percentage points.

It is more thousands than tens :-)

[just the page fault alone is quite costly]

> 
> Isn't it about 20 cycles per mispredicted branch on a P4?
> 
> Five of those and we're talking several percent slowdown, ridiculous
> as it seems.

There are a lot of conditional branches in the signal
path. If you don't believe me I can send you simics full instruction traces of it.
I'm not going to believe that 2-3 more make a significant difference.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Andi Kleen wrote:
> signal exception path is thousands of cycles, we're talking about tens
> of cycles here.

<hand-waving>

Tens vs thousands == percentage points.

Isn't it about 20 cycles per mispredicted branch on a P4?

Five of those and we're talking several percent slowdown, ridiculous
as it seems.

</hand-waving>

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 18:32:45 +0100
Jamie Lokier <jamie@shareable.org> wrote:

> Andi Kleen wrote:
> > > 	if ((addr & 3) == 0)
> > > 		return 0;
> > 
> > Maybe. But gcc generates quite good code (binary decision tree) code for
> > the switch() statement already so I don't think it is worth it to go 
> > through complications just to avoid that.
> > 
> > The decoder looks big in C, but when you take a look at its hot path in 
> > assembly it is quite fast.
> 
> I wonder.  No part of the signal path looks especially slow, and yet
> as a whole it isn't that fast.  The odd percentage slowdown here and
> there is quite bothersome.
> 
> GCC generates a decision tree.  You realise that about half of the
> branches in that tree will be mispredicted?

True. Doing a BTSL in a bitmap would be probably faster to check
bytes in a switch. Or maybe gcc should do that, but it doesn't.

But I doubt the small performance advantage of that for an still quite 
obscure path is worth the code uglification that would be require to do
it without gcc support.

To put it into perspective:

signal exception path is thousands of cycles, we're talking about tens
of cycles here.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Andi Kleen wrote:
> > 	if ((addr & 3) == 0)
> > 		return 0;
> 
> Maybe. But gcc generates quite good code (binary decision tree) code for
> the switch() statement already so I don't think it is worth it to go 
> through complications just to avoid that.
> 
> The decoder looks big in C, but when you take a look at its hot path in 
> assembly it is quite fast.

I wonder.  No part of the signal path looks especially slow, and yet
as a whole it isn't that fast.  The odd percentage slowdown here and
there is quite bothersome.

GCC generates a decision tree.  You realise that about half of the
branches in that tree will be mispredicted?

-- Jamie

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

> -----Original Message-----
> From: Andi Kleen [mailto:ak@suse.de] 
> Sent: Thursday, September 11, 2003 7:29 AM
> 
> On Thu, 11 Sep 2003 15:14:51 +0100
> Dave Jones <davej@redhat.com> wrote:
> 
> > On Thu, Sep 11, 2003 at 04:01:08PM +0200, Andi Kleen wrote:
> > 
> >  > > What's wrong with the current status quo that just says 
> >  > > "Athlon prefetch is broken"?
> >  > It doesn't fix user space for once.
> > 
> > And for another, it cripples the earlier athlons which 
> > don't have this
> > errata. Andi's fix at least makes prefetch work again on 
> > those boxes.
> > It's also arguable that prefetch() helps the older K7's 
> > more than the
> > affected ones.
> 
> All Athlons have this Errata. I can trigger it on an old
> 900Mhz pre XP Athlon too. You just have to use 3dnow prefetch
> instead of SSE prefetch.
> 
> BTW the older Athlons currently don't use prefetch because 
> the alternative
> patcher does not handle 3dnow style prefetch.
> 

Avoiding prefetch for all Athlons and earlier Opterons/Athlon64
even in the kernel can really tank performance. And as Andi says
it still doesn't solve user mode from hitting the errata.
 
] -Rich ...
] AMD Fellow

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

Potentially, any address can be a candidate. If you assume a sequential
access pattern for data, addresses toward the back of the cache-line
(but not the last few bytes) are less likely to hit the errata
because hopefully the page that the cacheline is in is 
already "faulted in". But that is no guarantee.


] -Rich ...
] AMD Fellow
] richard.brunner at amd com 

> -----Original Message-----
> From: Jamie Lokier [mailto:jamie@shareable.org] 
> Sent: Thursday, September 11, 2003 9:59 AM
> To: Andi Kleen
> Cc: Brunner, Richard; linux-kernel@vger.kernel.org; 
> akpm@osdl.org; torvalds@osdl.org
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> 
> Andi Kleen wrote:
> > +static int is_prefetch(struct pt_regs *regs, unsigned long addr)
> 
> Do I understand that certain values of "addr" can't be due to the
> erratum?
> 
> In which case, could you skip most of the is_prefetch() instruction
> decoder with a test like this?:
> 
> 	if ((addr & 3) == 0)
> 		return 0;
> 
> I'm not sure from the description of the erratum what, exactly, are
> the possible addresses which can appear in the fault information.
> 
> -- Jamie
> 

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

If the program is doing lots of page faults to SEGV signals 
intentionally, each of those transitions (exception to-> kernel ->
process -> signal-back to user) is already pretty expensive.
I would think that the slight overhead that is_prefetch() 
adds in this case is pretty low in comparision because 
the transitions themselves are very expensive in cycles. 
Most of the time, is_prefetch should be able to tell if 
it is a prefetch from just reading the first byte 
(one memory access).

We can measure this for such programs if need be, but, I would bet
we won't see any difference.

Having said that, I'm agnostic to whether is_prefetch() 
gets compiled out for non-AMD processors. I defer to 
all the kernel experts here if that is feasible or not.


] -Rich ...
] AMD Fellow
] richard.brunner at amd com 

> -----Original Message-----
> From: Jamie Lokier [mailto:jamie@shareable.org] 
> Sent: Thursday, September 11, 2003 9:55 AM
> To: Brunner, Richard
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> 
> richard.brunner@amd.com wrote:
> > Don't worry! I am pretty certain the patch won't impact the 
> > performance of the 2.6 kernel on processors from other vendors ;-)
> 
> is_prefetch() will slow down programs which depend on lots of SEGV
> signals: those garbage collectors which use mprotect and SIGSEGV to
> track dirty pages.
> 
> I wonder how much it will slow them down.
> 
> -- Jamie
> 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 17:58:45 +0100
Jamie Lokier <jamie@shareable.org> wrote:

> Andi Kleen wrote:
> > +static int is_prefetch(struct pt_regs *regs, unsigned long addr)
> 
> Do I understand that certain values of "addr" can't be due to the
> erratum?
> 
> In which case, could you skip most of the is_prefetch() instruction
> decoder with a test like this?:
> 
> 	if ((addr & 3) == 0)
> 		return 0;

Maybe. But gcc generates quite good code (binary decision tree) code for
the switch() statement already so I don't think it is worth it to go 
through complications just to avoid that.

The decoder looks big in C, but when you take a look at its hot path in 
assembly it is quite fast.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

Andi Kleen wrote:
> +static int is_prefetch(struct pt_regs *regs, unsigned long addr)

Do I understand that certain values of "addr" can't be due to the
erratum?

In which case, could you skip most of the is_prefetch() instruction
decoder with a test like this?:

	if ((addr & 3) == 0)
		return 0;

I'm not sure from the description of the erratum what, exactly, are
the possible addresses which can appear in the fault information.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jamie Lokier]

richard.brunner@amd.com wrote:
> Don't worry! I am pretty certain the patch won't impact the 
> performance of the 2.6 kernel on processors from other vendors ;-)

is_prefetch() will slow down programs which depend on lots of SEGV
signals: those garbage collectors which use mprotect and SIGSEGV to
track dirty pages.

I wonder how much it will slow them down.

-- Jamie

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
> On Thu, 11 Sep 2003 10:34:36 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>>X86_GENERIC is merely an optimization hint (currently it only changes the cache
>>>line size hint) It does not change anything related to correctness. Everything
>>>that handles correctness is checked unconditionally.
>>
>>When, building non-Pentium4-related code when CONFIG_MPENTIUM4 && 
>>!CONFIG_X86_GENERIC, it's OK that the code is incorrect for (picking 
>>example) AMD processors.
> 
> 
> No 2.6 changed that. On 2.6 you can exchange the kernels.
> 
> [that was mainly done for distributions, but helps other users too]


If distributions are not building with CONFIG_X86_GENERIC, then they're 
broken.  So it was rather pointless for 2.6 to "change that."

Luckily, CONFIG_X86_GENERIC allows us the opportunity to be _less_ 
generic when it's not defined, regardless of what you originally 
intended ;-)

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 10:34:36 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> 
> > X86_GENERIC is merely an optimization hint (currently it only changes the cache
> > line size hint) It does not change anything related to correctness. Everything
> > that handles correctness is checked unconditionally.
> 
> When, building non-Pentium4-related code when CONFIG_MPENTIUM4 && 
> !CONFIG_X86_GENERIC, it's OK that the code is incorrect for (picking 
> example) AMD processors.

No 2.6 changed that. On 2.6 you can exchange the kernels.

[that was mainly done for distributions, but helps other users too]
 
> If you're doing crazy LinuxBIOS stuff where flash size is limited, it 
> makes a lot of sense.  (and I do such crazy things)  The core 2.6 kernel 
> has really bloated with optional features, IMO.

If you really want to save text space just use .bz2 compression 
or compile the kernel with -Os. There are also other subsystems
that would benefit much more (better effort/cost ratio) than adding
micro #ifdefs to core code.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
> It was not created for that (I know that because I created it ;-)

hehe


> X86_GENERIC is merely an optimization hint (currently it only changes the cache
> line size hint) It does not change anything related to correctness. Everything
> that handles correctness is checked unconditionally.

When, building non-Pentium4-related code when CONFIG_MPENTIUM4 && 
!CONFIG_X86_GENERIC, it's OK that the code is incorrect for (picking 
example) AMD processors.

It would be a user bug to boot that on an AMD box, just like it would be 
user bug to boot a CONFIG_M586 kernel on an ancient 386.


> is_prefetch is a correctness thing.

When we know at compile time it's not needed, it should not be enabled.


>>If I disabled CONFIG_X86_GENERIC and select CONFIG_MPENTIUM4, I darned 
>>well better not get any Athlon code.  The cpu setup code in particular I 
>>want to conditionalize, and there are other bits that need work... but 
>>for the most part it works as intended.
> 
> 
> Now that's becomming silly. It's alttogether only a few KB and all
> __init code anyways.


If you're doing crazy LinuxBIOS stuff where flash size is limited, it 
makes a lot of sense.  (and I do such crazy things)  The core 2.6 kernel 
has really bloated with optional features, IMO.

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 15:28:09 +0100
Dave Jones <davej@redhat.com> wrote:

> On Thu, Sep 11, 2003 at 04:24:21PM +0200, Andi Kleen wrote:
>  > I considered that when writing the patch, but: is_prefetch is a single byte 
>  > memory access for something already in cache. Checking for an Athlon
>  > CPU needs two memory accesses in boot_cpu_data at least (checking vendor
>  > and model) 
> 
> You only need to check it once when the path is first taken, and then
> set a variable that makes you exit as soon as you enter it again.

Checking the variable also an memory access. 

is_prefetch does a few more instructions around the memory access, but these
are completely left in the noise.

The is_prefetch check is likely faster even than checking that variable because
the chances that the EIP is already in cache are much higher than some rarely
used variable.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 15:14:51 +0100
Dave Jones <davej@redhat.com> wrote:

> On Thu, Sep 11, 2003 at 04:01:08PM +0200, Andi Kleen wrote:
> 
>  > > What's wrong with the current status quo that just says "Athlon prefetch
>  > > is broken"?
>  > It doesn't fix user space for once.
> 
> And for another, it cripples the earlier athlons which don't have this
> errata. Andi's fix at least makes prefetch work again on those boxes.
> It's also arguable that prefetch() helps the older K7's more than the
> affected ones.

All Athlons have this Errata. I can trigger it on an old
900Mhz pre XP Athlon too. You just have to use 3dnow prefetch
instead of SSE prefetch.

BTW the older Athlons currently don't use prefetch because the alternative
patcher does not handle 3dnow style prefetch.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Dave Jones]

On Thu, Sep 11, 2003 at 04:24:21PM +0200, Andi Kleen wrote:
 > I considered that when writing the patch, but: is_prefetch is a single byte 
 > memory access for something already in cache. Checking for an Athlon
 > CPU needs two memory accesses in boot_cpu_data at least (checking vendor
 > and model) 

You only need to check it once when the path is first taken, and then
set a variable that makes you exit as soon as you enter it again.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 10:15:25 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Andi Kleen wrote:
> > It could be done but ... we are moving more and more to generic kernels
> > (e.g. see the alternative patch code which is enabled unconditionally)
> > So that when you have a kernel it will boot on near all modern CPUs.
> 
> 
> Only with CONFIG_X86_GENERIC.  That's precisely why CONFIG_X86_GENERIC 
> was created.

It was not created for that (I know that because I created it ;-)

X86_GENERIC is merely an optimization hint (currently it only changes the cache
line size hint) It does not change anything related to correctness. Everything
that handles correctness is checked unconditionally.

is_prefetch is a correctness thing.

> 
> If I disabled CONFIG_X86_GENERIC and select CONFIG_MPENTIUM4, I darned 
> well better not get any Athlon code.  The cpu setup code in particular I 
> want to conditionalize, and there are other bits that need work... but 
> for the most part it works as intended.

Now that's becomming silly. It's alttogether only a few KB and all
__init code anyways.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 15:14:02 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> 
> There is *one* change needed. We shouldnt call is_prefetch unless the
> current CPU is an Athlon. That way its a performance improvement for
> PIV, and Athlon in general, fixes the bug and causes no fancy athlon
> fixup code for non AMD processors.

I considered that when writing the patch, but: is_prefetch is a single byte 
memory access for something already in cache. Checking for an Athlon
CPU needs two memory accesses in boot_cpu_data at least (checking vendor
and model) 

So checking for Athlon first would be actually more expensive for other CPUs.

The switch is left in the noise, gcc compiles it to very efficient code.

This ignores the possibility of a recursive fault in the __get_user, but these
are rather unlikely. Normal page-on-demand code faults are handled 
before is_prefetch is reached.

-Andi 

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Jeff Garzik]

Andi Kleen wrote:
> It could be done but ... we are moving more and more to generic kernels
> (e.g. see the alternative patch code which is enabled unconditionally)
> So that when you have a kernel it will boot on near all modern CPUs.


Only with CONFIG_X86_GENERIC.  That's precisely why CONFIG_X86_GENERIC 
was created.

If I disabled CONFIG_X86_GENERIC and select CONFIG_MPENTIUM4, I darned 
well better not get any Athlon code.  The cpu setup code in particular I 
want to conditionalize, and there are other bits that need work... but 
for the most part it works as intended.

	Jeff

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Dave Jones]

On Thu, Sep 11, 2003 at 04:01:08PM +0200, Andi Kleen wrote:

 > > What's wrong with the current status quo that just says "Athlon prefetch
 > > is broken"?
 > It doesn't fix user space for once.

And for another, it cripples the earlier athlons which don't have this
errata. Andi's fix at least makes prefetch work again on those boxes.
It's also arguable that prefetch() helps the older K7's more than the
affected ones.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Alan Cox]

On Iau, 2003-09-11 at 14:54, Linus Torvalds wrote:
> This patch is fragile and looks pointless.
> 
> What's wrong with the current status quo that just says "Athlon prefetch
> is broken"?

Firstly performance, secondly user space exceptions. We work around lots
of other broken CPU things this one triggers a path that only matters on
Athlon.

There is *one* change needed. We shouldnt call is_prefetch unless the
current CPU is an Athlon. That way its a performance improvement for
PIV, and Athlon in general, fixes the bug and causes no fancy athlon
fixup code for non AMD processors.

Alan

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, 11 Sep 2003 06:54:53 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> This patch is fragile and looks pointless.

Why do you think it is fragile?  I don't see anything fragile in it.

> 
> What's wrong with the current status quo that just says "Athlon prefetch
> is broken"?

It doesn't fix user space for once.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Linus Torvalds]

This patch is fragile and looks pointless.

What's wrong with the current status quo that just says "Athlon prefetch
is broken"?

		Linus

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[dada1]

>
> I don't have any. But it would be very similar to the in kernel checking
> code (see the is_prefetch function in my patches). Just you feed it
> the fields from sigcontext in the signal handler and replace __get_user
> with a normal memory access.

OK will try... but how test it... sounds not easy.

> > I do use preftechnta instructions on my programs, and this errata could
> > explain some strange crashes.
>
> The bogus faults are very easy to diagnose. When you have a core dump
> and disassemble the faulting instruction (in gdb x/i $eip) and it is
> a prefetch (prefetch/prefetchw/prefetchnt*) then it could be that.

Well, the program is using more than 2Go ram... the core is not written to
disk as the machine hangs just *after*

And this is a remote machine in a Datacenter.

>
> If it is a different instruction it is unrelated.
>
> It would also only happen when you prefetch ever on unmapped addresses.

NULL for example ?

Typical example of code ;

T_cell *ptr, *next ;
for (ptr = list.head ; ptr != NULL ; ptr = next) {
   next = ptr->next ;
   prefetch(next) ;
   some_work(ptr) ;
   }

I may replace NULL by &FakeMappedData   (allways present in memory)

>
> That sounds like an unrelated issue.
>
> When user space crashes on this the kernel is unaffected.

This is not a kernel crash. But total freeze as all memory is used by
network buffers, in no more than 10 seconds.
This application receive smalls TCP messages (about 30 bytes), but the
network stacks allocates 4KB buffers to store this little messages.

No oops, but what can we do, if the freeze lasts eternity ?

I posted a test application some days ago about this problem and got no
answers/feedback.

>
> In case the 2.6 kernel crashes on this (2.4 does not trigger it)
> then you can also run the oops through ksymoops and check if the
> faulting instruction is prefetch. If it isn't  then it is something else.
>
> Network buffers using up all of low mem and then crash
> is likely some OOM handling problem. If you're on 2.4 try an -aa kernel,
> they handle this much better than the marcelo tree. If it's 2.6 then
> I would recommend posting oopses on this list, maybe someone can fix
> it. I suspect 2.6's OOM handling could be still improved.
>
> -Andi
> -

Eric

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Thu, Sep 11, 2003 at 06:58:23AM +0200, dada1 wrote:
> 
> From: "Andi Kleen" <ak@muc.de>
> >
> > Of course they may want to also fix it in a different way to run on older
> > kernels (e.g. handling the signal in user space or avoiding the
> conditions).
> > But doing it centrally in the kernel is a bit cleaner and at some point
> > people have to update their kernels anyways.
> 
> Could you be kind enough to post here the example code for a SIGSEGV handler
> that would be necessary for old kernels ?

I don't have any. But it would be very similar to the in kernel checking
code (see the is_prefetch function in my patches). Just you feed it
the fields from sigcontext in the signal handler and replace __get_user 
with a normal memory access.

> 
> I do think it woul help some people like me, for the future googling on the
> prefetch errata.
> 
> I do use preftechnta instructions on my programs, and this errata could
> explain some strange crashes.

The bogus faults are very easy to diagnose. When you have a core dump
and disassemble the faulting instruction (in gdb x/i $eip) and it is
a prefetch (prefetch/prefetchw/prefetchnt*) then it could be that.

If it is a different instruction it is unrelated.

It would also only happen when you prefetch ever on unmapped addresses.

> 
> As the program crashing is a huge multi-threaded network application, with
> up to 300000 opened TCP sockets, the SIGSEGV fault is usually followed by a
> system crash (networks buffers using all of lowmem)

That sounds like an unrelated issue.

When user space crashes on this the kernel is unaffected.

In case the 2.6 kernel crashes on this (2.4 does not trigger it)
then you can also run the oops through ksymoops and check if the 
faulting instruction is prefetch. If it isn't  then it is something else.

Network buffers using up all of low mem and then crash 
is likely some OOM handling problem. If you're on 2.4 try an -aa kernel, 
they handle this much better than the marcelo tree. If it's 2.6 then 
I would recommend posting oopses on this list, maybe someone can fix 
it. I suspect 2.6's OOM handling could be still improved.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[dada1]

From: "Andi Kleen" <ak@muc.de>
>
> Of course they may want to also fix it in a different way to run on older
> kernels (e.g. handling the signal in user space or avoiding the
conditions).
> But doing it centrally in the kernel is a bit cleaner and at some point
> people have to update their kernels anyways.

Could you be kind enough to post here the example code for a SIGSEGV handler
that would be necessary for old kernels ?

I do think it woul help some people like me, for the future googling on the
prefetch errata.

I do use preftechnta instructions on my programs, and this errata could
explain some strange crashes.

As the program crashing is a huge multi-threaded network application, with
up to 300000 opened TCP sockets, the SIGSEGV fault is usually followed by a
system crash (networks buffers using all of lowmem)

Thanks
Eric Dumazet

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[richard.brunner]

Jun,

I have to agree with what Andi says. It is in a slow path,
and we want to guard against user programs that could hit it.
Making it conditional doesn't buy a lot and would cause lots of
re-validation of the patch that we would like
to avoid so we can get this in to the 2.6 kernel ASAP. 
Don't worry! I am pretty certain the patch won't impact the 
performance of the 2.6 kernel on processors from other vendors ;-)

                       Thanks!
                                          ]-Rich ...
                                          ]AMD Fellow
> From: Nakajima, Jun [mailto:jun.nakajima@intel.com] 
>
> > I would hate to break this again just to save a few hundred bytes in 
> > this function. Also the overhead is very low so it is also not 
> > interesting to make it conditional for speed reasons.
> 
> For maintenance and testing purposes, I think it's still 
> better to make it conditional. If the errata are fixed, you 
> might want to kill the condition depending on the stepping, 
> for example. During the transition time, you need to support 
> both the steppings until old ones go away (then remove the 
> workaround).
> 
> Thanks,
> Jun

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

"Nakajima, Jun" <jun.nakajima@intel.com> writes:

>> I would hate to break this again just to save a few hundred bytes in
>> this function. Also the overhead is very low so it is also not
>> interesting to make it conditional for speed reasons.
>
> For maintenance and testing purposes, I think it's still better to make
> it conditional. If the errata are fixed, you might want to kill the
> condition depending on the stepping, for example. During the transition
> time, you need to support both the steppings until old ones go away
> (then remove the workaround).

My understanding is that it will never be fixed on the K7 (current Athlons),
And these will be with us for a long time; more or less forever, just like
the f00f bug :-)

I would agree with you if the patch had some bad impact on common
paths, but I don't think that's the case here. It merely adds a cheap check
to an already slow path.

On the other hand the errata is so unlikely that I doubt it will be frequently
hit anyways. 2.6 kernel hitting it was just very very unlucky, and it
only did so very infrequently.

Just to fix the kernel we could have chosen a different workaround, like
adding an exception table entry to each prefetch and jumping back
(I did that on the x86-64 port originally). But this way is also not that bad
and it fixes hypothetical user space programs that could maybe hit it too.

Of course they may want to also fix it in a different way to run on older
kernels (e.g. handling the signal in user space or avoiding the conditions).
But doing it centrally in the kernel is a bit cleaner and at some point
people have to update their kernels anyways.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Wed, 10 Sep 2003 20:43:34 PDT, "Nakajima, Jun" said:

> For maintenance and testing purposes, I think it's still better to make
> it conditional. If the errata are fixed, you might want to kill the
> condition depending on the stepping, for example. During the transition
> time, you need to support both the steppings until old ones go away
> (then remove the workaround).

We've still got conditionals for 486 chips.  How long a transition till the old
ones of *this* chipset go away?


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

RE: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Nakajima, Jun]

> I would hate to break this again just to save a few hundred bytes in
> this function. Also the overhead is very low so it is also not
> interesting to make it conditional for speed reasons.

For maintenance and testing purposes, I think it's still better to make
it conditional. If the errata are fixed, you might want to kill the
condition depending on the stepping, for example. During the transition
time, you need to support both the steppings until old ones go away
(then remove the workaround).

Thanks,
Jun

> -----Original Message-----
> From: Andi Kleen [mailto:ak@suse.de]
> Sent: Wednesday, September 10, 2003 6:47 PM
> To: Andrew Morton
> Cc: Andi Kleen; richard.brunner@amd.com; linux-kernel@vger.kernel.org;
> torvalds@osdl.org
> Subject: Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata
> 
> On Wed, Sep 10, 2003 at 06:44:14PM -0700, Andrew Morton wrote:
> > Andi Kleen <ak@suse.de> wrote:
> > >
> > >  +static int is_prefetch(struct pt_regs *regs, unsigned long addr)
> >
> > Can we make this code go away if the configured CPU is, say, Intel?
> > (I couldn't find a sane CONFIG_ setting to use for this).
> 
> It could be done but ... we are moving more and more to generic
kernels
> (e.g. see the alternative patch code which is enabled unconditionally)
> So that when you have a kernel it will boot on near all modern CPUs.
> Currently Athlon and P4 kernels run on each other for example.
> 
> I would hate to break this again just to save a few hundred bytes in
> this function. Also the overhead is very low so it is also not
> interesting to make it conditional for speed reasons.
> 
> >
> > It might be vaguely interesting to add a user-visible counter for
this
> > event? If someone somehow comes up with an application which hits
the
> fault
> > frequently they will take a big performance hit.
> 
> In that case they can just profile the kernel. is_prefetch should
> show it then.
> 
> Of course someone can still add the counter if they want, I'm not
> opposed to it.
> 
> -Andi
> 
> -
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

On Wed, Sep 10, 2003 at 06:44:14PM -0700, Andrew Morton wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> >  +static int is_prefetch(struct pt_regs *regs, unsigned long addr)
> 
> Can we make this code go away if the configured CPU is, say, Intel?
> (I couldn't find a sane CONFIG_ setting to use for this).

It could be done but ... we are moving more and more to generic kernels
(e.g. see the alternative patch code which is enabled unconditionally)
So that when you have a kernel it will boot on near all modern CPUs.
Currently Athlon and P4 kernels run on each other for example.

I would hate to break this again just to save a few hundred bytes in 
this function. Also the overhead is very low so it is also not 
interesting to make it conditional for speed reasons.

> 
> It might be vaguely interesting to add a user-visible counter for this
> event? If someone somehow comes up with an application which hits the fault
> frequently they will take a big performance hit.

In that case they can just profile the kernel. is_prefetch should
show it then.

Of course someone can still add the counter if they want, I'm not
opposed to it.

-Andi

Re: [PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
>  +static int is_prefetch(struct pt_regs *regs, unsigned long addr)

Can we make this code go away if the configured CPU is, say, Intel?
(I couldn't find a sane CONFIG_ setting to use for this).

It might be vaguely interesting to add a user-visible counter for this
event? If someone somehow comes up with an application which hits the fault
frequently they will take a big performance hit.

[PATCH] 2.6 workaround for Athlon/Opteron prefetch errata

[Andi Kleen]

>     works just fine. (Andi will be posting them soon when he 
>     wakes up ;-)

He's still awake ;-)

Here is a patch to detect a prefect exception for 2.6.0test5/i386

I'm posting the versions for 2.4.22/x86-64 and 2.4.22/i386 in separate
mail. 2.6/x86-64 is not done yet, but will be in my next merge.

The patches only change the slow path in the page fault handler - 
PREFETCH is only checked for when the kernel would send a signal
anyways or print an oops. The check is also rather cheap so it is
unconditionally enabled.

It handles SSE2 prefetches and 3dnow! style prefetches.

The only tricky bit is that it has to be careful to avoid recursive
segfaults. The prefetch checker can cause another page fault when
it does __get_user, but these should not recurse more than once.

This is insured by the placement of the checks in the page fault handler and
only checking for prefetches if the fault came from user space
or the exception tables have been already checked.
To make this work without a per task exception nest counter
I had to change the SIGBUS handling path slightly. Now when
an get/put_user in kernel causes a SIGBUS it is not delivered
to user space. Instead you just get the standard EFAULT back.

It also removed Andrew's old workaround for the problem.

-Andi

--- linux-2.6.0test5-work/arch/i386/mm/fault.c-o	2003-05-27 03:00:20.000000000 +0200
+++ linux-2.6.0test5-work/arch/i386/mm/fault.c	2003-09-10 23:58:52.000000000 +0200
@@ -55,6 +55,77 @@
 	console_loglevel = loglevel_save;
 }
 
+/* Sometimes the CPU reports invalid exceptions on prefetch.
+   Check that here and ignore.
+   Opcode checker based on code by Richard Brunner */
+static int is_prefetch(struct pt_regs *regs, unsigned long addr)
+{ 
+	unsigned char *instr = (unsigned char *)(regs->eip);
+	int scan_more = 1;
+	int prefetch = 0; 
+	unsigned char *max_instr = instr + 15;
+
+	/* Avoid recursive faults. This makes kernel jumping to nirvana
+	   reporting work better. */
+	if (regs->eip == addr)
+		return 0; 
+
+	while (scan_more && instr < max_instr) { 
+		unsigned char opcode;
+		unsigned char instr_hi;
+		unsigned char instr_lo;
+
+		if (__get_user(opcode, instr))
+			break; 
+
+		instr_hi = opcode & 0xf0; 
+		instr_lo = opcode & 0x0f; 
+		instr++;
+
+		switch (instr_hi) { 
+		case 0x20:
+		case 0x30:
+			/* Values 0x26,0x2E,0x36,0x3E are valid x86
+			   prefixes.  In long mode, the CPU will signal
+			   invalid opcode if some of these prefixes are
+			   present so we will never get here anyway */
+			scan_more = ((instr_lo & 7) == 0x6);
+			break;
+			
+		case 0x40:
+			/* May be valid in long mode (REX prefixes) */
+			break; 
+			
+		case 0x60:
+			/* 0x64 thru 0x67 are valid prefixes in all modes. */
+			scan_more = (instr_lo & 0xC) == 0x4;
+			break;		
+		case 0xF0:
+			/* 0xF0, 0xF2, and 0xF3 are valid prefixes in all modes. */
+			scan_more = !instr_lo || (instr_lo>>1) == 1;
+			break;			
+		case 0x00:
+			/* Prefetch instruction is 0x0F0D or 0x0F18 */
+			scan_more = 0;
+			if (__get_user(opcode, instr)) 
+				break;
+			prefetch = (instr_lo == 0xF) &&
+				(opcode == 0x0D || opcode == 0x18);
+			break;			
+		default:
+			scan_more = 0;
+			break;
+		} 
+	}
+
+#if 0
+	if (prefetch)
+		printk("%s: prefetch caused page fault at %lx/%lx\n", current->comm,
+		       regs->eip, addr);
+#endif
+	return prefetch;
+}
+
 asmlinkage void do_invalid_op(struct pt_regs *, unsigned long);
 
 /*
@@ -110,7 +181,7 @@
 	 * atomic region then we must not take the fault..
 	 */
 	if (in_atomic() || !mm)
-		goto no_context;
+		goto bad_area_nosemaphore;
 
 	down_read(&mm->mmap_sem);
 
@@ -198,8 +269,12 @@
 bad_area:
 	up_read(&mm->mmap_sem);
 
+bad_area_nosemaphore:
 	/* User mode accesses just cause a SIGSEGV */
 	if (error_code & 4) {
+		if (is_prefetch(regs, address))
+			return;
+
 		tsk->thread.cr2 = address;
 		tsk->thread.error_code = error_code;
 		tsk->thread.trap_no = 14;
@@ -232,6 +307,9 @@
 	if (fixup_exception(regs))
 		return;
 
+ 	if (is_prefetch(regs, address))
+ 		return;
+
 /*
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.
@@ -286,10 +364,13 @@
 do_sigbus:
 	up_read(&mm->mmap_sem);
 
-	/*
-	 * Send a sigbus, regardless of whether we were in kernel
-	 * or user mode.
-	 */
+	/* Kernel mode? Handle exceptions or die */
+	if (!(error_code & 4))
+		goto no_context;
+
+	if (is_prefetch(regs, address))
+		return;
+
 	tsk->thread.cr2 = address;
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_no = 14;
@@ -298,10 +379,6 @@
 	info.si_code = BUS_ADRERR;
 	info.si_addr = (void *)address;
 	force_sig_info(SIGBUS, &info, tsk);
-
-	/* Kernel mode? Handle exceptions or die */
-	if (!(error_code & 4))
-		goto no_context;
 	return;
 
 vmalloc_fault:
--- linux-2.6.0test5-work/include/asm-i386/processor.h-o	2003-09-09 20:55:46.000000000 +0200
+++ linux-2.6.0test5-work/include/asm-i386/processor.h	2003-09-11 03:22:16.000000000 +0200
@@ -578,8 +578,6 @@
 #define ARCH_HAS_PREFETCH
 extern inline void prefetch(const void *x)
 {
-	if (cpu_data[0].x86_vendor == X86_VENDOR_AMD)
-		return;		/* Some athlons fault if the address is bad */
 	alternative_input(ASM_NOP4,
 			  "prefetchnta (%1)",
 			  X86_FEATURE_XMM,