| Message ID | 1258555922-2064-6-git-send-email-jslaby@novell.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
From: Jiri Slaby <jslaby@novell.com> Date: Wed, 18 Nov 2009 15:51:52 +0100 > Make sure compiler won't do weird things with limits. E.g. fetching > them twice may return 2 different values after writable limits are > implemented. > > Signed-off-by: Jiri Slaby <jslaby@novell.com> Acked-by: David S. Miller <davem@davemloft.net> But I wonder have we really seen the compiler create this kind of situation? Or is this patch series based upon the fact that it "could happen"? -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 18 Nov 2009, David Miller wrote: > > But I wonder have we really seen the compiler create this > kind of situation? Or is this patch series based upon the > fact that it "could happen"? We have seen things like that in practice - where the compiler re-loads a value twice, rather than use a copy like the source code did. That said, it's rare, to the point of being _almost_ unheard of. It's much more common that gcc generates bad code by doing the reverse (trying to keep things in registers and spilling, instead of just re-generating the value). There are very very few cases where ACCESS_ONCE() actually matters for correctness. Because in practice, the value is either modified some way (and spilling it is cheaper than re-computing the modification), or there's just some operation that might act as a memory barrier and alias the original memory location so gcc wouldn't dare re-load anyway. However, one of the nice things about ACCESS_ONCE() is that it's also a big flag for "this value is loaded without locking, on purpose". So even if it doesn't then actually change code generation significantly (most common end result especially on x86 that has most ALU instructions taking memory operations: gcc generates slightly worse code due to getting nervous about 'volatile' and not combining instructions), it's a big honking piece of programmer documentation: look out! It's basically a heads-up for lockless programming like RCU. As such, it can be something scary, but when it's done right, it's a good thing. And I think that for rlimits, we do have a good reason to say "sure, somebody else may change the limit values concurrently, but we don't really care: we just want _one_ value, whether it's the old or the new one". That said, the patch you Ack'ed is in the series of patches that I hated, and Nak'ed for other reasons (namely "-EEXPRESSIONTOOCOMPLICATEDTOLIVE"). Linus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c index e2d1024..004ed47 100644 --- a/arch/sparc/kernel/sys_sparc_64.c +++ b/arch/sparc/kernel/sys_sparc_64.c @@ -361,6 +361,7 @@ EXPORT_SYMBOL(get_fb_unmapped_area); void arch_pick_mmap_layout(struct mm_struct *mm) { unsigned long random_factor = 0UL; + unsigned long gap; if (current->flags & PF_RANDOMIZE) { random_factor = get_random_int(); @@ -375,9 +376,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm) * Fall back to the standard layout if the personality * bit is set, or if the expected stack growth is unlimited: */ + gap = ACCESS_ONCE(current->signal->rlim[RLIMIT_STACK].rlim_cur); if (!test_thread_flag(TIF_32BIT) || (current->personality & ADDR_COMPAT_LAYOUT) || - current->signal->rlim[RLIMIT_STACK].rlim_cur == RLIM_INFINITY || + gap == RLIM_INFINITY || sysctl_legacy_va_layout) { mm->mmap_base = TASK_UNMAPPED_BASE + random_factor; mm->get_unmapped_area = arch_get_unmapped_area; @@ -385,9 +387,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm) } else { /* We know it's 32-bit */ unsigned long task_size = STACK_TOP32; - unsigned long gap; - gap = current->signal->rlim[RLIMIT_STACK].rlim_cur; if (gap < 128 * 1024 * 1024) gap = 128 * 1024 * 1024; if (gap > (task_size / 6 * 5))
Make sure compiler won't do weird things with limits. E.g. fetching them twice may return 2 different values after writable limits are implemented. Signed-off-by: Jiri Slaby <jslaby@novell.com> Cc: James Morris <jmorris@namei.org> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: "David S. Miller" <davem@davemloft.net> Cc: sparclinux@vger.kernel.org --- arch/sparc/kernel/sys_sparc_64.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)