Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

On Sat, Aug 05, 2006 at 07:47:41PM +1000, Rusty Russell wrote:
> [Andi, sorry, x86_64 part untested, so sending straight to you]
> 
> rdmsr and rdtsc are macros, altering their arguments directly.  An
> inline function would offer decent typechecking, but needs to take
> pointer args.  The comment notes that gcc produces better code with

I think I prefer the macro variant actually. Sorry. It just looks
better without the &s.

We don't care very much about the code quality here because
rdmsr/wrmsr are always very slow in microcode anyways and tend
to synchronize the CPUs.

If you feel a need to clean up I would suggest you convert more
users over to the ll variants which take a single 64bit value
instead of two 32bit ones.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Rusty Russell]

On Sun, 2006-08-06 at 04:38 +0200, Andi Kleen wrote:
> On Sat, Aug 05, 2006 at 07:47:41PM +1000, Rusty Russell wrote:
> > [Andi, sorry, x86_64 part untested, so sending straight to you]
> > 
> > rdmsr and rdtsc are macros, altering their arguments directly.  An
> > inline function would offer decent typechecking, but needs to take
> > pointer args.  The comment notes that gcc produces better code with
> 
> I think I prefer the macro variant actually. Sorry. It just looks
> better without the &s.

Hi Andi,

	Please reconsider.  This isn't about being pretty, it's about not
having hidden side-effects, and having typechecking.

> We don't care very much about the code quality here because
> rdmsr/wrmsr are always very slow in microcode anyways and tend
> to synchronize the CPUs.

Agreed, but comment about it above the macros made me wary, so I checked
it.  No significant code difference with gcc >= 4.0, at least.

> If you feel a need to clean up I would suggest you convert more
> users over to the ll variants which take a single 64bit value
> instead of two 32bit ones.

You mean the l and ll variants?  The 64 bit variants are rdmsrl and
rdtscll, not to be confused with rdtscl, which returns the lower 32
bits.  This confusion caused the x86_64 bug in gameport.c which the
patch comment mentioned (at least, seems to be a bug to me).

See why I want to fix these names?

So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
it's a more invasive change...

Thanks,
Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[H. Peter Anvin]

Rusty Russell wrote:
> 
> You mean the l and ll variants?  The 64 bit variants are rdmsrl and
> rdtscll, not to be confused with rdtscl, which returns the lower 32
> bits.  This confusion caused the x86_64 bug in gameport.c which the
> patch comment mentioned (at least, seems to be a bug to me).
> 
> See why I want to fix these names?
> 
> So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
> msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
> it's a more invasive change...

rdmsrl is really misnamed.  It should have been rdmsrq to be consistent, 
and have rdmsrl return the low 32 bits.

	-hpa

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Rusty Russell]

On Sat, 2006-08-05 at 19:58 -0700, H. Peter Anvin wrote:
> Rusty Russell wrote:
> > 
> > You mean the l and ll variants?  The 64 bit variants are rdmsrl and
> > rdtscll, not to be confused with rdtscl, which returns the lower 32
> > bits.  This confusion caused the x86_64 bug in gameport.c which the
> > patch comment mentioned (at least, seems to be a bug to me).
> > 
> > See why I want to fix these names?
> > 
> > So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
> > msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
> > it's a more invasive change...
> 
> rdmsrl is really misnamed.  It should have been rdmsrq to be consistent, 
> and have rdmsrl return the low 32 bits.

I prefer the more explicit linux-style naming of rdmsr_low32/rdmsr64,
myself, even though this is x86-specific code.  Noone has an excuse for
misunderstanding then...

Thanks!
Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[H. Peter Anvin]

Rusty Russell wrote:
>>>
>>> So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
>>> msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
>>> it's a more invasive change...
>> rdmsrl is really misnamed.  It should have been rdmsrq to be consistent, 
>> and have rdmsrl return the low 32 bits.
> 
> I prefer the more explicit linux-style naming of rdmsr_low32/rdmsr64,
> myself, even though this is x86-specific code.  Noone has an excuse for
> misunderstanding then...
> 

Well, we *do* have readb/readw/readl/readq etc.

	-hpa

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Rusty Russell]

On Sat, 2006-08-05 at 20:11 -0700, H. Peter Anvin wrote:
> Rusty Russell wrote:
> >>>
> >>> So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
> >>> msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
> >>> it's a more invasive change...
> >> rdmsrl is really misnamed.  It should have been rdmsrq to be consistent, 
> >> and have rdmsrl return the low 32 bits.
> > 
> > I prefer the more explicit linux-style naming of rdmsr_low32/rdmsr64,
> > myself, even though this is x86-specific code.  Noone has an excuse for
> > misunderstanding then...
> 
> Well, we *do* have readb/readw/readl/readq etc.

Exactly.  And I modelled the name rdtsc64 on the "new-style" ioread8,
ioread16 and ioread32 from asm-generic/iomap.h 8)

Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

> 	Please reconsider.  This isn't about being pretty, it's about not
> having hidden side-effects,

I wouldn't call it hidden, it's well defined in the architecture.

> and having typechecking.

The existing code will already reject any non integer and I don't
see a particular need to be more strict than that.

> > If you feel a need to clean up I would suggest you convert more
> > users over to the ll variants which take a single 64bit value
> > instead of two 32bit ones.
> 
> You mean the l and ll variants?  The 64 bit variants are rdmsrl and
> rdtscll, not to be confused with rdtscl, which returns the lower 32
> bits.  This confusion caused the x86_64 bug in gameport.c 

Why does gameport access MSRs or TSCs? That sounds like a bug in itself.

<looking at the code> 

This whole thing is broken, e.g. on a preemptive kernel when the
code can switch CPUs 

Dmitry, I would suggest to convert it over to do_gettimeofday and remove
all the architecture ifdefs.

Or maybe just remove it completely.  Who cares about the speed of a gameport 
anyways? And why can't they measure it in user space? 

> See why I want to fix these names?

No.

> So if you would prefer u64 rdtsc64(), u32 rdtsc_low(), u64 rdmsr64(int
> msr), u32 rdmsr_low(int msr), I can convert everyone to that, although
> it's a more invasive change...

I think things are most fine right now, except that I think most users
of high low should be converted to work directly on 64bit quantities.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Rusty Russell]

On Sun, 2006-08-06 at 05:16 +0200, Andi Kleen wrote:
> > 	Please reconsider.  This isn't about being pretty, it's about not
> > having hidden side-effects,
> 
> I wouldn't call it hidden, it's well defined in the architecture.

Sorry Andi, I'm not talking about the asm, which is fine.  I'm talking
about the function-style macro which alters its arguments directly.
It's very bad form.

	int a, b;
	rdtsc(a, b);

> > and having typechecking.
> 
> The existing code will already reject any non integer and I don't
> see a particular need to be more strict than that.

Um?

	u64 c;
	int a,b;

	rdtsc(&a, &b);
	rdtscl(c);

These macros are badly named and don't check for bad usage.  Sure, less
than 1% of uses is wrong at the moment, but I'm volunteering to fix them
because I think it sets a bad example and sets us up for more bugs.

I feel fairly strongly about this, but I'll drop the x86_64 changes.

Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

> I feel fairly strongly about this, but I'll drop the x86_64 changes.

I would prefer if you leave them alone in i386 too.

-andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Dmitry Torokhov]

On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> This whole thing is broken, e.g. on a preemptive kernel when the
> code can switch CPUs 
> 

Would not preempt_disable fix that?

> Dmitry, I would suggest to convert it over to do_gettimeofday and remove
> all the architecture ifdefs.
> 
> Or maybe just remove it completely.  Who cares about the speed of a gameport 
> anyways? And why can't they measure it in user space? 
> 

Analog driver uses it to adjust timing. Vojtech should have more background
on that..

-- 
Dmitry

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > This whole thing is broken, e.g. on a preemptive kernel when the
> > code can switch CPUs 
> > 
> 
> Would not preempt_disable fix that?

Partially, but you still have other problems. Please just get rid
of it. Why do we have timer code in the kernel if you then chose
not to use it?

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > code can switch CPUs 
> > > 
> > 
> > Would not preempt_disable fix that?
> 
> Partially, but you still have other problems. Please just get rid
> of it. Why do we have timer code in the kernel if you then chose
> not to use it?
 
The problem is that gettimeofday() is not always fast. The joystick
drivers will not work if the timing calls take significant time compared
to an inb() from ISA/LPC space.

That's why they don't fallback to PIT timing when TSC is not available -
io counting is a better option.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > > code can switch CPUs 
> > > > 
> > > 
> > > Would not preempt_disable fix that?
> > 
> > Partially, but you still have other problems. Please just get rid
> > of it. Why do we have timer code in the kernel if you then chose
> > not to use it?
>  
> The problem is that gettimeofday() is not always fast. 

When it is not fast that means it is not reliable and then you're
also not well off using it anyways.

Please change that code.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Mon, Aug 07, 2006 at 02:28:45PM +0200, Andi Kleen wrote:
> On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> > On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> > > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > > > code can switch CPUs 
> > > > > 
> > > > 
> > > > Would not preempt_disable fix that?
> > > 
> > > Partially, but you still have other problems. Please just get rid
> > > of it. Why do we have timer code in the kernel if you then chose
> > > not to use it?
> >  
> > The problem is that gettimeofday() is not always fast. 
> 
> When it is not fast that means it is not reliable and then you're
> also not well off using it anyways.

I assume you wanted to say "When gettimeofday() is slow, it means TSC is
not reliable", which I agree with. 

But I need, in the driver, in the no-TSC case use i/o counting, not a
slow but reliable method. And I can't say, from outside the timing
subsystem, whether gettimeofday() is fast or slow.

I assume we could make it work with the monotonic timer instead. 

> Please change that code.
 
I'm not arguing that the code is correct. ;)

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

On Mon, Aug 07, 2006 at 02:48:55PM +0200, Vojtech Pavlik wrote:
> On Mon, Aug 07, 2006 at 02:28:45PM +0200, Andi Kleen wrote:
> > On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> > > On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> > > > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > > > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > > > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > > > > code can switch CPUs 
> > > > > > 
> > > > > 
> > > > > Would not preempt_disable fix that?
> > > > 
> > > > Partially, but you still have other problems. Please just get rid
> > > > of it. Why do we have timer code in the kernel if you then chose
> > > > not to use it?
> > >  
> > > The problem is that gettimeofday() is not always fast. 
> > 
> > When it is not fast that means it is not reliable and then you're
> > also not well off using it anyways.
> 
> I assume you wanted to say "When gettimeofday() is slow, it means TSC is
> not reliable", which I agree with. 
> 
> But I need, in the driver, in the no-TSC case use i/o counting, not a
> slow but reliable method. And I can't say, from outside the timing
> subsystem, whether gettimeofday() is fast or slow.

Hmm if that is the only obstacle I can export a "slow gettimeofday" flag.

However it would be some work to implement it for all architectures.

> 
> I assume we could make it work with the monotonic timer instead. 

The monotonic timer is the right thing to use to make you
independent of ntpd, but it's normally not faster or slower than gettimeofday.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Mon, Aug 07, 2006 at 02:56:39PM +0200, Andi Kleen wrote:
> On Mon, Aug 07, 2006 at 02:48:55PM +0200, Vojtech Pavlik wrote:
> > On Mon, Aug 07, 2006 at 02:28:45PM +0200, Andi Kleen wrote:
> > > On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> > > > On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> > > > > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > > > > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > > > > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > > > > > code can switch CPUs 
> > > > > > > 
> > > > > > 
> > > > > > Would not preempt_disable fix that?
> > > > > 
> > > > > Partially, but you still have other problems. Please just get rid
> > > > > of it. Why do we have timer code in the kernel if you then chose
> > > > > not to use it?
> > > >  
> > > > The problem is that gettimeofday() is not always fast. 
> > > 
> > > When it is not fast that means it is not reliable and then you're
> > > also not well off using it anyways.
> > 
> > I assume you wanted to say "When gettimeofday() is slow, it means TSC is
> > not reliable", which I agree with. 
> > 
> > But I need, in the driver, in the no-TSC case use i/o counting, not a
> > slow but reliable method. And I can't say, from outside the timing
> > subsystem, whether gettimeofday() is fast or slow.
> 
> Hmm if that is the only obstacle I can export a "slow gettimeofday" flag.

That would help.

> However it would be some work to implement it for all architectures.
> 
> > I assume we could make it work with the monotonic timer instead. 
> 
> The monotonic timer is the right thing to use to make you independent
> of ntpd, but it's normally not faster or slower than gettimeofday.
 
Yup.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Dmitry Torokhov]

On 8/7/06, Andi Kleen <ak@muc.de> wrote:
> On Mon, Aug 07, 2006 at 02:48:55PM +0200, Vojtech Pavlik wrote:
> > On Mon, Aug 07, 2006 at 02:28:45PM +0200, Andi Kleen wrote:
> > > On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> > > > On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> > > > > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> > > > > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > > > > > > This whole thing is broken, e.g. on a preemptive kernel when the
> > > > > > > code can switch CPUs
> > > > > > >
> > > > > >
> > > > > > Would not preempt_disable fix that?
> > > > >
> > > > > Partially, but you still have other problems. Please just get rid
> > > > > of it. Why do we have timer code in the kernel if you then chose
> > > > > not to use it?
> > > >
> > > > The problem is that gettimeofday() is not always fast.
> > >
> > > When it is not fast that means it is not reliable and then you're
> > > also not well off using it anyways.
> >
> > I assume you wanted to say "When gettimeofday() is slow, it means TSC is
> > not reliable", which I agree with.
> >
> > But I need, in the driver, in the no-TSC case use i/o counting, not a
> > slow but reliable method. And I can't say, from outside the timing
> > subsystem, whether gettimeofday() is fast or slow.
>
> Hmm if that is the only obstacle I can export a "slow gettimeofday" flag.
>
> However it would be some work to implement it for all architectures.
>

Hmm, would it be easier to export "fast gettimeofday" and assume that
we have slow gettimeofday by default (so gameport will fall back on io
counting)?

Btw, could anyone point me to the origin of the thread - I can't find
it in any of  the archives of LKML list (including my personal).
Thanks!

-- 
Dmitry

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

On Mon, Aug 07, 2006 at 09:32:29AM -0400, Dmitry Torokhov wrote:
> On 8/7/06, Andi Kleen <ak@muc.de> wrote:
> >On Mon, Aug 07, 2006 at 02:48:55PM +0200, Vojtech Pavlik wrote:
> >> On Mon, Aug 07, 2006 at 02:28:45PM +0200, Andi Kleen wrote:
> >> > On Mon, Aug 07, 2006 at 01:09:31PM +0200, Vojtech Pavlik wrote:
> >> > > On Mon, Aug 07, 2006 at 10:48:50AM +0200, Andi Kleen wrote:
> >> > > > On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> >> > > > > On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> >> > > > > > This whole thing is broken, e.g. on a preemptive kernel when 
> >the
> >> > > > > > code can switch CPUs
> >> > > > > >
> >> > > > >
> >> > > > > Would not preempt_disable fix that?
> >> > > >
> >> > > > Partially, but you still have other problems. Please just get rid
> >> > > > of it. Why do we have timer code in the kernel if you then chose
> >> > > > not to use it?
> >> > >
> >> > > The problem is that gettimeofday() is not always fast.
> >> >
> >> > When it is not fast that means it is not reliable and then you're
> >> > also not well off using it anyways.
> >>
> >> I assume you wanted to say "When gettimeofday() is slow, it means TSC is
> >> not reliable", which I agree with.
> >>
> >> But I need, in the driver, in the no-TSC case use i/o counting, not a
> >> slow but reliable method. And I can't say, from outside the timing
> >> subsystem, whether gettimeofday() is fast or slow.
> >
> >Hmm if that is the only obstacle I can export a "slow gettimeofday" flag.
> >
> >However it would be some work to implement it for all architectures.
> >
> 
> Hmm, would it be easier to export "fast gettimeofday" and assume that
> we have slow gettimeofday by default (so gameport will fall back on io
> counting)?

I would expect fast gettimeofday to be more common than slow.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andreas Mohr]

On Mon, Aug 07, 2006 at 02:56:39PM +0200, Andi Kleen wrote:
> On Mon, Aug 07, 2006 at 02:48:55PM +0200, Vojtech Pavlik wrote:
> > But I need, in the driver, in the no-TSC case use i/o counting, not a
> > slow but reliable method. And I can't say, from outside the timing
> > subsystem, whether gettimeofday() is fast or slow.
> 
> Hmm if that is the only obstacle I can export a "slow gettimeofday" flag.

Wouldn't it be much more useful to normalize this versus (e.g.) CPU cycles
for much more information than a plain "this is fast/this is slow" flag,
to be measured on bootup?
That way a driver could use

	if (gtod_cpu_cycles_needed <= 500)
		gettimeofday();
	else
		funky_fast_workaround();

OK, in total we have at least four ways of doing this:

a) gtod_is_slow flag
b) number of CPU cycles needed
c) number of nanoseconds needed (but this is less useful since it doesn't
   properly take into account the fast vs. slow CPUs behaviour, I think)
d) providing all three items above together, for optimal flexibility??

This is somewhat related to an idea of mine which would be to benchmark all
clock sources on bootup and print a timing summary, optionally warning
users if grave performance issues have been found with a specific source
(and especially if that one is active!).
Additionally, print timing summary of gettimeofday() itself on bootup?

Andreas Mohr

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

> That way a driver could use
> 
> 	if (gtod_cpu_cycles_needed <= 500)
> 		gettimeofday();
> 	else
> 		funky_fast_workaround();

Sounds like overengineering to me. I prefer something simple.

> OK, in total we have at least four ways of doing this:

Please don't get carried away with this. I'm really not interested
in any complex solutions here.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Mon, Aug 07, 2006 at 05:57:14PM +0200, Andi Kleen wrote:
> > That way a driver could use
> > 
> > 	if (gtod_cpu_cycles_needed <= 500)
> > 		gettimeofday();
> > 	else
> > 		funky_fast_workaround();
> 
> Sounds like overengineering to me. I prefer something simple.

I could as well benchmark gettimeofday() in the gameport init.

> > OK, in total we have at least four ways of doing this:
> 
> Please don't get carried away with this. I'm really not interested
> in any complex solutions here.


-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Andi Kleen]

Vojtech Pavlik <vojtech@suse.cz> writes:

> On Mon, Aug 07, 2006 at 05:57:14PM +0200, Andi Kleen wrote:
> > > That way a driver could use
> > > 
> > > 	if (gtod_cpu_cycles_needed <= 500)
> > > 		gettimeofday();
> > > 	else
> > > 		funky_fast_workaround();
> > 
> > Sounds like overengineering to me. I prefer something simple.
> 
> I could as well benchmark gettimeofday() in the gameport init.

Except you can't because the only way to benchmark it would
be to use gettimeofday already and then you don't know
how much is overhead and what is the real time.

-Andi

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Mon, Aug 07, 2006 at 06:12:03PM +0200, Andi Kleen wrote:
> Vojtech Pavlik <vojtech@suse.cz> writes:
> 
> > On Mon, Aug 07, 2006 at 05:57:14PM +0200, Andi Kleen wrote:
> > > > That way a driver could use
> > > > 
> > > > 	if (gtod_cpu_cycles_needed <= 500)
> > > > 		gettimeofday();
> > > > 	else
> > > > 		funky_fast_workaround();
> > > 
> > > Sounds like overengineering to me. I prefer something simple.
> > 
> > I could as well benchmark gettimeofday() in the gameport init.
> 
> Except you can't because the only way to benchmark it would
> be to use gettimeofday already and then you don't know
> how much is overhead and what is the real time.
 
I run it for 1/10th of a second in a loop and see how many iterations
I've done.  That's enough to check if its fast enough to use in the
gameport routine. Less than 50k iterations and I can't use it.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Pavel Machek]

Hi!

> > > > > Would not preempt_disable fix that?
> > > > 
> > > > Partially, but you still have other problems. Please just get rid
> > > > of it. Why do we have timer code in the kernel if you then chose
> > > > not to use it?
> > >  
> > > The problem is that gettimeofday() is not always fast. 
> > 
> > When it is not fast that means it is not reliable and then you're
> > also not well off using it anyways.
> 
> I assume you wanted to say "When gettimeofday() is slow, it means TSC is
> not reliable", which I agree with. 
> 
> But I need, in the driver, in the no-TSC case use i/o counting, not a
> slow but reliable method. And I can't say, from outside the timing
> subsystem, whether gettimeofday() is fast or slow.

do gettimeofday(); gettimeofday(); gettimeofday();, then compare the
result with gettimeofday(); inb(); gettimeofday(); ?

						Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] Turn rdmsr, rdtsc into inline functions, clarify names

[Vojtech Pavlik]

On Sun, Aug 06, 2006 at 10:43:44PM -0400, Dmitry Torokhov wrote:
> On Saturday 05 August 2006 23:16, Andi Kleen wrote:
> > This whole thing is broken, e.g. on a preemptive kernel when the
> > code can switch CPUs 
> 
> Would not preempt_disable fix that?
> 
> > Dmitry, I would suggest to convert it over to do_gettimeofday and remove
> > all the architecture ifdefs.
> > 
> > Or maybe just remove it completely.  Who cares about the speed of a gameport 
> > anyways? And why can't they measure it in user space? 
> 
> Analog driver uses it to adjust timing. Vojtech should have more background
> on that..
 
The gameport speed is used by digital-over-gameport joystick drivers to
keep track of time based on the number of inb()s done to the gameport. 

We can't use anything slower than rdtsc there, and since rdtsc isn't
always available, the count of inb()s is the best approximation.

The speed can't easily be measured in userspace - you need to make sure
IRQs, DMA and other stuff doesn't interfere with the measurement, which
needs to be rather exact. AND, you need it for the kernel to work, so it
doesn't make sense to move it to an userspace tool that the kernel would
have to call.

The analog joystick driver does even more timing magic to get precise
readings from the port, where the value read depends on the time for a
bit on the port to change from 0 to 1.

I agree that gameports are mostly dead today, but people are still using
them.

-- 
Vojtech Pavlik
Director SuSE Labs