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:	Mon, 17 Nov 2008 14:43:58 -0800
From:	Venki Pallipadi <venkatesh.pallipadi@...el.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Arjan van de Ven <arjan@...radead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Mike Galbraith <efault@....de>
Subject: Re: [git pull] scheduler updates

On Sat, Nov 08, 2008 at 11:29:57AM -0800, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> > On Sat, 8 Nov 2008, Ingo Molnar wrote:
> > > 
> > > So that's why my change moves it from the __native_read_tsc() over to 
> > > _only_ the vget_cycles().
> > 
> > Ahh. I was looking at native_read_tscp(). Which has no barriers. But then 
> > we don't actually save the actual TSC, we only end up using the "p" part, 
> > so we don't care..
> > 
> > Anyway, even for the vget_cycles(), is there really any reason to 
> > have _two_ barriers? Also, I still think it would be a hell of a lot 
> > more readable and logical to put the barriers in the _caller_, so 
> > that people actually see what the barriers are there for.
> > 
> > When they are hidden, they make no sense. The helper function just 
> > has two insane barriers without explanation, and the caller doesn't 
> > know that the code is serialized wrt something random.
> 
> ok, fully agreed, i've queued up the cleanup for that, see it below.
> 
> sidenote: i still kept the get_cycles() versus vget_cycles() 
> distinction, to preserve the explicit marker that vget_cycles() is 
> used in user-space mode code. We periodically forgot about that in the 
> past. But otherwise, the two inline functions are now identical. 
> (except for the assymetry of its inlining, and the comment about the 
> boot_cpu_data use of the has_tsc check)
> 


Patch being discussed on this thread (commit 0d12cdd) has a regression on
one of the test systems here.

With the patch, I see

checking TSC synchronization [CPU#0 -> CPU#1]:
Measured 28 cycles TSC warp between CPUs, turning off TSC clock.
Marking TSC unstable due to check_tsc_sync_source failed

Whereas, without the patch syncs pass fine on all CPUs

checking TSC synchronization [CPU#0 -> CPU#1]: passed.

Due to this, TSC is marke unstable, when it is not actually unstable.
This is because syncs in check_tsc_wrap() goes away due to this commit.

As per the discussion on this thread, correct way to fix this is to add
explicit syncs as below?

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@...el.com>

---
 arch/x86/kernel/tsc_sync.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6/arch/x86/kernel/tsc_sync.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/tsc_sync.c	2008-11-10 15:27:12.000000000 -0800
+++ linux-2.6/arch/x86/kernel/tsc_sync.c	2008-11-17 14:13:17.000000000 -0800
@@ -46,7 +46,9 @@ static __cpuinit void check_tsc_warp(voi
 	cycles_t start, now, prev, end;
 	int i;
 
+	rdtsc_barrier();
 	start = get_cycles();
+	rdtsc_barrier();
 	/*
 	 * The measurement runs for 20 msecs:
 	 */
@@ -61,7 +63,9 @@ static __cpuinit void check_tsc_warp(voi
 		 */
 		__raw_spin_lock(&sync_lock);
 		prev = last_tsc;
+		rdtsc_barrier();
 		now = get_cycles();
+		rdtsc_barrier();
 		last_tsc = now;
 		__raw_spin_unlock(&sync_lock);
 
--
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/