svn commit: r221703 - in head/sys: amd64/include i386/include x86/isa x86/x86

[Jung-uk Kim]

On Thursday 12 May 2011 07:23 am, Andriy Gapon wrote:
> on 09/05/2011 20:34 Jung-uk Kim said the following:
> > Author: jkim
> > Date: Mon May  9 17:34:00 2011
> > New Revision: 221703
> > URL: http://svn.freebsd.org/changeset/base/221703
>
> [snip]
>
> I would to note [again] that I don't like code style of this
> change.
>
> > Modified: head/sys/x86/x86/tsc.c
> > =================================================================
> >============= --- head/sys/x86/x86/tsc.c	Mon May  9 17:30:25
> > 2011	(r221702) +++ head/sys/x86/x86/tsc.c	Mon May  9 17:34:00
> > 2011	(r221703) @@ -326,7 +326,73 @@ init_TSC(void)
> >  	    tsc_levels_changed, NULL, EVENTHANDLER_PRI_ANY);
> >  }
> >
> > -void
> > +#ifdef SMP
> > +
> > +#define	TSC_READ(x)			\
> > +static void				\
> > +tsc_read_##x(void *arg)			\
> > +{					\
> > +	uint32_t *tsc = arg;		\
> > +	u_int cpu = PCPU_GET(cpuid);	\
> > +					\
> > +	tsc[cpu * 3 + x] = rdtsc32();	\
> > +}
> > +TSC_READ(0)
> > +TSC_READ(1)
> > +TSC_READ(2)
> > +#undef TSC_READ
> > +
> > +#define	N	1000
>
> I don't like macro overuse in the above.
> Especially "N".

I think duplicating trivial and almost identical functions is more 
harmful to many eyes.

'N' here is temporary and arbitrary.  I think 1,000 is actually 
overkill but I wanted to "stress-test" smp_rendezvous(). :-P

Now that you found a bug in the function, I can remove N but I want to 
keep it for a while.  Actually, it was already proven useful, i.e., I 
gave a user the following patch:

-#define	N	1000
+#define	N	100

and it worked around his problem for now (until you commit the 
smp_rendezvous() fix).

> > +static void
> > +comp_smp_tsc(void *arg)
> > +{
> > +	uint32_t *tsc;
> > +	int32_t d1, d2;
> > +	u_int cpu = PCPU_GET(cpuid);
> > +	u_int i, j, size;
> > +
> > +	size = (mp_maxid + 1) * 3;
> > +	for (i = 0, tsc = arg; i < N; i++, tsc += size)
> > +		CPU_FOREACH(j) {
> > +			if (j == cpu)
> > +				continue;
> > +			d1 = tsc[cpu * 3 + 1] - tsc[j * 3];
> > +			d2 = tsc[cpu * 3 + 2] - tsc[j * 3 + 1];
> > +			if (d1 <= 0 || d2 <= 0) {
> > +				smp_tsc = 0;
> > +				return;
> > +			}
> > +		}
> > +}
> > +
> > +static int
> > +test_smp_tsc(void)
> > +{
> > +	uint32_t *data, *tsc;
> > +	u_int i, size;
> > +
> > +	if (!smp_tsc && !tsc_is_invariant)
> > +		return (-100);
> > +	size = (mp_maxid + 1) * 3;
> > +	data = malloc(sizeof(*data) * size * N, M_TEMP, M_WAITOK);
> > +	for (i = 0, tsc = data; i < N; i++, tsc += size)
> > +		smp_rendezvous(tsc_read_0, tsc_read_1, tsc_read_2, tsc);
>
> I don't like that what is logically a two dimensional array 3 x
> (mp_maxid + 1) is represented as a one-dimensional array with all
> ensuing multiplications by three and other array index
> manipulations.

(As I said earlier) I agree with you in general but using 
two-dimensional array or array of pointers just added more complexity 
to the code and it didn't make the code any clearer than I originally 
thought.  Again, it is just a matter of taste, anyway. ;-)

> > +	smp_tsc = 1;	/* XXX */
> > +	smp_rendezvous(smp_no_rendevous_barrier, comp_smp_tsc,
> > +	    smp_no_rendevous_barrier, data);
> > +	free(data, M_TEMP);
> > +	if (bootverbose)
> > +		printf("SMP: %sed TSC synchronization test\n",
> > +		    smp_tsc ? "pass" : "fail");
> > +	return (smp_tsc ? 800 : -100);
>
> I still think something higher should be returned here for the
> smp_tsc == true case.  It doesn't make sense to go through the
> shenanigans to underrate TSC in the end.

http://docs.freebsd.org/cgi/mid.cgi?201105091352.49971.jkim

I thought you wanted a separate commit, didn't you?

> On a more general note, smp_rendezvous() might not be the best
> mechanism here. In my opinion/understanding, smp_rendezvous()
> provides only the following guarantees:
> - if a setup action is specified, then every CPU executes the setup
> action before any CPU executes the main action
> - if a teardown action is specified, then every CPU executes the
> main action before any CPU executes the teardown action
>
> There are no timing guarantees, only the sequence guarantees.

That's all I wanted to test, i.e., we only validate whether all TSCs 
are in order (and that's all we can do, in fact).

> Any timing observations that we may have now are a product of the
> implementation and can change if the implementation change.  So the
> newly introduced check may miss systemic differences in TSC values
> between CPUs if they are small enough. But I am not really sure if
> such a small differences would really matter.
>
> Worse case if there is some difference in TSC frequency between
> CPUs (e.g. in different sockets), after a powerup or a reset
> difference between TSC values may be very small, but could grow
> more and more with uptime.  Not sure if this is a realistic enough
> scenario, though.

Actually, I am kinda reluctant to enable smp_tsc by default on recent 
CPUs.  Although they made all TSCs in sync, it is very very tricky to 
make it work in reality, e.g.,

https://patchwork.kernel.org/patch/691712/

Sigh...

Jung-uk Kim