[PATCH] Ensure PSR.ac is cleared for early userspace

[PATCH] Ensure PSR.ac is cleared for early userspace

[Luck, Tony]

Some kernels report a few unaligned exceptions during early userspace:

init(1): unaligned access to 0x6000000000089f0b, ip=0x40000000000e3581
init(1): unaligned access to 0x6000000000089f13, ip=0x40000000000e3620
init(1): unaligned access to 0x6000000000089f11, ip=0x40000000000e3640
init(1): unaligned access to 0x600000000008af1b, ip=0x40000000000e3581
init(1): unaligned access to 0x600000000008af23, ip=0x40000000000e3620
fsck(3193): unaligned access to 0x600000000001255b, ip=0x2000000000069491
fsck(3193): unaligned access to 0x6000000000012563, ip=0x2000000000069530
fsck(3193): unaligned access to 0x6000000000012561, ip=0x2000000000069550
mount(3205): unaligned access to 0x6000000000012c2b, ip=0x2000000000069491
mount(3205): unaligned access to 0x6000000000012c33, ip=0x2000000000069530
mount(3855): unaligned access to 0x6000000000012c6b, ip=0x2000000000069491
mount(3855): unaligned access to 0x6000000000012c73, ip=0x2000000000069530
mount(3855): unaligned access to 0x6000000000012c71, ip=0x2000000000069550

while others do not report these exceptions. A git bisection pointed
at an apparently innocent commit.  The real problem is that we do
not explicitly set or clear PSR.ac (we do clear it when executing
32-bit x86 binaries) so it is randomly set or clear in different
kernel builds.

Fix is to clear PSR.ac before transitioning to user code.


Signed-off-by: Tony Luck <tony.luck@intel.com>

---

Does the use of "rum" instruction need some paravirtualization? It is
not in a critical path (used a couple of times during boot).

diff --git a/arch/ia64/kernel/entry.S b/arch/ia64/kernel/entry.S
index 7ef0c59..595a0b6 100644
--- a/arch/ia64/kernel/entry.S
+++ b/arch/ia64/kernel/entry.S
@@ -499,6 +499,7 @@ GLOBAL_ENTRY(prefetch_stack)
 END(prefetch_stack)
 
 GLOBAL_ENTRY(kernel_execve)
+	rum IA64_PSR_AC
 	mov r15=__NR_execve			// put syscall number in place
 	break __BREAK_SYSCALL
 	br.ret.sptk.many rp

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Isaku Yamahata]

On Tue, Nov 11, 2008 at 05:35:50PM -0800, Luck, Tony wrote:
> Does the use of "rum" instruction need some paravirtualization? It is
> not in a critical path (used a couple of times during boot).

No, because rum isn't privileged instruction.
At lease xen/ia64 doesn't need it.

thanks,

> 
> diff --git a/arch/ia64/kernel/entry.S b/arch/ia64/kernel/entry.S
> index 7ef0c59..595a0b6 100644
> --- a/arch/ia64/kernel/entry.S
> +++ b/arch/ia64/kernel/entry.S
> @@ -499,6 +499,7 @@ GLOBAL_ENTRY(prefetch_stack)
>  END(prefetch_stack)
>  
>  GLOBAL_ENTRY(kernel_execve)
> +	rum IA64_PSR_AC
>  	mov r15=__NR_execve			// put syscall number in place
>  	break __BREAK_SYSCALL
>  	br.ret.sptk.many rp
> 

-- 
yamahata

RE: [PATCH] Ensure PSR.ac is cleared for early userspace

[Luck, Tony]

 GLOBAL_ENTRY(kernel_execve)
+       rum IA64_PSR_AC
        mov r15=__NR_execve                     // put syscall number in place
        break __BREAK_SYSCALL
        br.ret.sptk.many rp

I'm not overly happy with this because I still don't understand how
the PSR.ac bit became set in this context.  This patch papers
over the problem by clearing psr.ac ... but it shouldn't be
set at this point!

As far as I can see usage of psr.ac is as follows:

1) When Linux first boots the code in head.S transitions to
virtual mode ... this code sets PSR.ac=0.

2) Eight of the code paths in ivt.S set PSR.ac=1 before
transitioning to C code.  Thus interrupt handlers, page faults
and other traps are executed with .ac=1.  This is controlled by the
PSR_DEFAULT_BITS define ... which is (and I think always has)
been defined to set psr.ac.

[I'm a bit puzzled as to why we want strict alignment checking
in interrupt/fault handlers, but are satisfied with more relaxed
behaivour in base code]

I'm also puzzled why this issue only just surfaced, and why I
only see it on generic kernels.

The problem may be related to some interrupt behavior.  If I
tweak just the "__interrupt" code to not set psr.ac (but leave
the other seven fault handlers setting psr.ac=1) then I don't
see any unaligned traps in early init code.

-Tony

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[David Mosberger-Tang]

Tony,

Have you checked the IA64 ABI?  I remember there being a section
talking about the state in which signal handlers should be started.  I
don't remember off hand if it said anything about psr.ac.

  --david

On 11/14/08, Luck, Tony <tony.luck@intel.com> wrote:
>  GLOBAL_ENTRY(kernel_execve)
>  +       rum IA64_PSR_AC
>         mov r15=__NR_execve                     // put syscall number in place
>         break __BREAK_SYSCALL
>         br.ret.sptk.many rp
>
>  I'm not overly happy with this because I still don't understand how
>  the PSR.ac bit became set in this context.  This patch papers
>  over the problem by clearing psr.ac ... but it shouldn't be
>  set at this point!
>
>  As far as I can see usage of psr.ac is as follows:
>
>  1) When Linux first boots the code in head.S transitions to
>  virtual mode ... this code sets PSR.ac=0.
>
>  2) Eight of the code paths in ivt.S set PSR.ac=1 before
>  transitioning to C code.  Thus interrupt handlers, page faults
>  and other traps are executed with .ac=1.  This is controlled by the
>  PSR_DEFAULT_BITS define ... which is (and I think always has)
>  been defined to set psr.ac.
>
>  [I'm a bit puzzled as to why we want strict alignment checking
>  in interrupt/fault handlers, but are satisfied with more relaxed
>  behaivour in base code]
>
>  I'm also puzzled why this issue only just surfaced, and why I
>  only see it on generic kernels.
>
>  The problem may be related to some interrupt behavior.  If I
>  tweak just the "__interrupt" code to not set psr.ac (but leave
>  the other seven fault handlers setting psr.ac=1) then I don't
>  see any unaligned traps in early init code.
>
>  -Tony
>
> --
>  To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Mosberger Consulting LLC, http://www.mosberger-consulting.com/

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Tony Luck]

On Fri, Nov 14, 2008 at 7:26 PM, David Mosberger-Tang
<dmosberger@gmail.com> wrote:
>  In any case, as I remember, the idea was that the kernel would always
>  execute with PSR.AC=1 to avoid silent performance bugs and just since
>  the kernel should be alignment-clean, anyhow.  The fys-code doesn't
>  turn on PSR.AC, but I believe the heavy-weight syscall path does.  I
>  don't know why head.S turns off PSR.AC.  Seems like a bug to me.

Running base code and interrupt/trap code with different settings of PSR.ac
does sound like a bug ... but the question of what we should set it to deserves
a little more thought.

Perhaps the best option would be for test & development kernels to set
PSR.ac=1 to aid in tracking down any alignment problems in the code.
But production kernels should set PSR.ac=0 so that they don't take the
cost of a trap on unaligned access that the specific model of processor
can handle anyway.  If this is the right path, then it should become a
CONFIG option.

-Tony

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Rick Jones]

Tony Luck wrote:
> On Fri, Nov 14, 2008 at 7:26 PM, David Mosberger-Tang
> <dmosberger@gmail.com> wrote:
> 
>> In any case, as I remember, the idea was that the kernel would always
>> execute with PSR.AC=1 to avoid silent performance bugs and just since
>> the kernel should be alignment-clean, anyhow.  The fys-code doesn't
>> turn on PSR.AC, but I believe the heavy-weight syscall path does.  I
>> don't know why head.S turns off PSR.AC.  Seems like a bug to me.
> 
> 
> Running base code and interrupt/trap code with different settings of PSR.ac
> does sound like a bug ... but the question of what we should set it to deserves
> a little more thought.
> 
> Perhaps the best option would be for test & development kernels to set
> PSR.ac=1 to aid in tracking down any alignment problems in the code.
> But production kernels should set PSR.ac=0 so that they don't take the
> cost of a trap on unaligned access that the specific model of processor
> can handle anyway.  If this is the right path, then it should become a
> CONFIG option.

If the hypothesis is that test and development kernels would catch the 
alignment problems, then there shouldn't be any in the production 
kernels right?  However, if there _are_ any out there in production 
don't we want them to become visible?  Even on processors which can 
handle them it is still better to not have the unaligned acesses in the 
first place right?

rick jones

RE: [PATCH] Ensure PSR.ac is cleared for early userspace

[Luck, Tony]

> If the hypothesis is that test and development kernels would catch the
> alignment problems, then there shouldn't be any in the production
> kernels right?  However, if there _are_ any out there in production
> don't we want them to become visible?  Even on processors which can
> handle them it is still better to not have the unaligned acesses in the
> first place right?

I was thinking of the case where someone loads a module that has
not had the benefit of being developed in an environment where all
the unaligned accesses could be easily seen and removed.  It seems
(to me) that the user of that system would be happier to have the
flow of "unaligned access" messages on the console reduced by letting
the h/w handle the ones that it could.

It is definitely better to avoid the unaligned access completely,
but I'm not sure this is possible in practice.

-Tony

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Rick Jones]

Luck, Tony wrote:
>>If the hypothesis is that test and development kernels would catch the
>>alignment problems, then there shouldn't be any in the production
>>kernels right?  However, if there _are_ any out there in production
>>don't we want them to become visible?  Even on processors which can
>>handle them it is still better to not have the unaligned acesses in the
>>first place right?
> 
> 
> I was thinking of the case where someone loads a module that has
> not had the benefit of being developed in an environment where all
> the unaligned accesses could be easily seen and removed.  It seems
> (to me) that the user of that system would be happier to have the
> flow of "unaligned access" messages on the console reduced by letting
> the h/w handle the ones that it could.

Ignorance being bliss?

If this module was developed in an environment where the unaligned 
accesses could not be easily seen how does hiding the messages in 
production get them addressed?  Modules being developed in such 
environments would seem to suggest that the messages _should_ be seen in 
production?

rick jones

RE: [PATCH] Ensure PSR.ac is cleared for early userspace

[Luck, Tony]

> Ignorance being bliss?

If the module is only hitting unaligned addresses that the processor
can handle ... then ignorance might well be bliss.  The cost of
an un-reported misaligned access is anly a few cycles.  The cost
of the trap and trip to arch/ia64/kernel/unaligned.c is IIRC
somewhere in the 800-900 cycle range ... more for the printk.

If we had less of a sledgehammer to report the problem, then I'd
find it a lot easier to say that this should be enabled in
production systems.

> If this module was developed in an environment where the unaligned
> accesses could not be easily seen how does hiding the messages in
> production get them addressed?  Modules being developed in such
> environments would seem to suggest that the messages _should_ be seen in
> production?

Definitely a downside to my approach ... if the end users
don't see that they have been sold a crummy driver they
won't know that they should complain.

-Tony

RE: [PATCH] Ensure PSR.ac is cleared for early userspace

[Petr Tesarik]

Luck, Tony píše v Po 17. 11. 2008 v 12:58 -0800:
> > Ignorance being bliss?
> 
> If the module is only hitting unaligned addresses that the processor
> can handle ... then ignorance might well be bliss.  The cost of
> an un-reported misaligned access is anly a few cycles.  The cost
> of the trap and trip to arch/ia64/kernel/unaligned.c is IIRC
> somewhere in the 800-900 cycle range ... more for the printk.
> 
> If we had less of a sledgehammer to report the problem, then I'd
> find it a lot easier to say that this should be enabled in
> production systems.
> 
> > If this module was developed in an environment where the unaligned
> > accesses could not be easily seen how does hiding the messages in
> > production get them addressed?  Modules being developed in such
> > environments would seem to suggest that the messages _should_ be seen in
> > production?
> 
> Definitely a downside to my approach ... if the end users
> don't see that they have been sold a crummy driver they
> won't know that they should complain.

Well, then let's make it a boot option (or a sysctl). The default must
be psr.ac = 1, but if the admin sees "unaligned access" messages and
knows that there is no way of getting a (timely) fix for that particular
production system, s/he should be able to turn them off.

IMO this approach solves both issues, because:

  1. Everebody is aware of the performance hit
  2. The cost of it can be minimized

Just my two cents,
Petr Tesarik

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Matthew Wilcox]

On Tue, Nov 18, 2008 at 08:06:53AM +0100, Petr Tesarik wrote:
> Well, then let's make it a boot option (or a sysctl). The default must
> be psr.ac = 1, but if the admin sees "unaligned access" messages and
> knows that there is no way of getting a (timely) fix for that particular
> production system, s/he should be able to turn them off.

Or how about ia64 does what every other Linux port does and turn off
alignment warnings altogether?  Are they *particularly* expensive
on Itanium?  I bet they aren't.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Ensure PSR.ac is cleared for early userspace

[Robin Holt]

On Tue, Nov 18, 2008 at 12:12:55AM -0700, Matthew Wilcox wrote:
> On Tue, Nov 18, 2008 at 08:06:53AM +0100, Petr Tesarik wrote:
> > Well, then let's make it a boot option (or a sysctl). The default must
> > be psr.ac = 1, but if the admin sees "unaligned access" messages and
> > knows that there is no way of getting a (timely) fix for that particular
> > production system, s/he should be able to turn them off.
> 
> Or how about ia64 does what every other Linux port does and turn off
> alignment warnings altogether?  Are they *particularly* expensive
> on Itanium?  I bet they aren't.

They are expensive enough that our customers complain.  Many of SGI's
customers are concerned about that last 1% of performance.

Having a way to quickly track down the customer's issue without having to
recreate their exact setup is helpful.  The last three kernel unaligned
access issues I can recall being reported by our customers were in the
IDE(1) and IPv4(2) areas.  Neither of these areas is SGI specific and
the three reports were customer workload specific.

I doubt we would have gotten these isolated without frustrating the
customer with cycles of try this, try this, oh and try this, now try
this kernel with different options and you need to rebuild all those
other things now too.

As it was, we merely took their console log and /proc/kallsyms and lined
up the source of the problem, scratched our heads for a bit, and isolated
the issue.

On a kernel modules note, we are seeing more and more of our customers
running stuff like panasys, lustre, etc which have external kernel
modules built either by the customer's support staff or other vendors.
Putting custom kernels on a site is becoming much more difficult.  If we
have a choice, please consider failing on the side of easily isolating
problems using a standard config.

I like Petr's suggestion of making it a sysctl/boot option with the
default being on.

Thanks,
Robin

RE: [PATCH] Ensure PSR.ac is cleared for early userspace

[Luck, Tony]

> Well, then let's make it a boot option (or a sysctl). The default must
> be psr.ac = 1, but if the admin sees "unaligned access" messages and
> knows that there is no way of getting a (timely) fix for that particular
> production system, s/he should be able to turn them off.

Nice in theory ... but since the setting of psr.ac happens as an immediate
value in the instructions in all the trap handlers, this would require
yet another batch of kernel patching its own code ... which we do plenty
of already ... but I'm not feeling in the mood to add more for this.

I've been swayed by Rick's arguments ... we don't appear to have
much of a problem at the moment with kernel unaligned accesses, so
let's try to keep it that way.  I'm putting in a patch that turns
on psr.ac in head.S so we do strict aligment checking all the time
in the kernel.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þ&ºãø§¶\x17›¡Ü¨}©ž²Æ zÚ&j:+v‰¨þø\x1e¯ù\x1e®w¥þŠà2ŠÞ™¨è­Ú&¢)ß¡«a¶Úÿÿûàz¿äz¹Þ—ú+ƒùšŽŠÝ¢jÿŠwèþ^[f