linux-kernel - Re: [git pull] scheduler updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 8 Nov 2008 19:52:19 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Mike Galbraith <efault@....de>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [git pull] scheduler updates

* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sat, 8 Nov 2008, Ingo Molnar wrote:
> >
> > Ingo Molnar (2):
> >       sched: improve sched_clock() performance
> >       sched: optimize sched_clock() a bit
> 
> Btw, why do we do that _idiotic_ rdtsc_barrier() AT ALL?
> 
> No sane user can possibly want it. If you do 'rdtsc', there's 
> nothing you can do about a few cycles difference due to OoO 
> _anyway_. Adding barriers is entirely meaningless - it's not going 
> to make the return value mean anything else.
> 
> Can we please just remove that idiocy? Or can somebody give a _sane_ 
> argument for it?

yeah, i had the same thinking, so i zapped it for everything but the 
vdso driven vgettimeofday GTOD method.

For that one, i chickened out, because we have this use in 
arch/x86/kernel/vsyscall_64.c:

                now = vread();
                base = __vsyscall_gtod_data.clock.cycle_last;
                mask = __vsyscall_gtod_data.clock.mask;
                mult = __vsyscall_gtod_data.clock.mult;
                shift = __vsyscall_gtod_data.clock.shift;

which can be triggered by gettimeofday() on certain systems.

And i couldnt convince myself that this sequence couldnt result in 
userspace-observable GTOD time warps there, so i went for the obvious 
fix first.

If the "now = vread()"'s RDTSC instruction is speculated to after it 
reads cycle_last, and another vdso call shortly after this does 
another RDTSC in this same sequence, the two RDTSC's could be mixed up 
in theory, resulting in negative time?

I _think_ i heard some noises in the past that this could indeed 
happen (and have vague memories that this was the justification for 
the barrier's introduction), but have to check the old emails to 
figure out what exactly the issue was and on what CPUs.

It's not completely impossible for this to happen, as the vdso calls 
are really just simple function calls, so not nearly as strongly 
serialized as say a real syscall based gettimeofday() call.

In any case, it failed my "it must be obvious within 1 minute for it 
to be eligible for sched/urgent" threshold and i didnt want to 
introduce a time warp.

But you are right, and i've queued up the full fix below as well, as a 
reminder.

	Ingo

----------------->
>From 2efe2c42e008a80ebe1992db63749386778f7df8 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@...e.hu>
Date: Sat, 8 Nov 2008 19:46:48 +0100
Subject: [PATCH] x86, time: remove rdtsc_barrier()

Linus pointed out that even for vread() rdtsc_barrier() is pointless
overhead - as due to speculative instructions there's no such thing
as reliable cycle count anyway.

Signed-off-by: Ingo Molnar <mingo@...e.hu>
---
 arch/x86/include/asm/system.h |   13 -------------
 arch/x86/include/asm/tsc.h    |    6 +-----
 2 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index 2ed3f0f..1a1d45e 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -409,17 +409,4 @@ void default_idle(void);
 #define set_mb(var, value) do { var = value; barrier(); } while (0)
 #endif

-/*
- * Stop RDTSC speculation. This is needed when you need to use RDTSC
- * (or get_cycles or vread that possibly accesses the TSC) in a defined
- * code region.
- *
- * (Could use an alternative three way for this if there was one.)
- */
-static inline void rdtsc_barrier(void)
-{
-	alternative(ASM_NOP3, "mfence", X86_FEATURE_MFENCE_RDTSC);
-	alternative(ASM_NOP3, "lfence", X86_FEATURE_LFENCE_RDTSC);
-}
-
 #endif /* _ASM_X86_SYSTEM_H */
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 9cd83a8..700aeb8 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -44,11 +44,7 @@ static __always_inline cycles_t vget_cycles(void)
 	if (!cpu_has_tsc)
 		return 0;
 #endif
-	rdtsc_barrier();
-	cycles = (cycles_t)__native_read_tsc();
-	rdtsc_barrier();
-
-	return cycles;
+	return (cycles_t)__native_read_tsc();
 }

 extern void tsc_init(void);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/