Re: [patch] x86: convert rdtscll() to use __native_read_tsc

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@xxxxxxxx> wrote:

> Ingo Molnar wrote:
>> The reason for the __native_read_tsc() / native_read_tsc() distinction 
>> is and obscure problem with paravirt function pointers. Such 
>> constructs:
>>
>>   ./xen/enlighten.c:	.read_tsc = native_read_tsc,
>>
>> do not always work fine with all versions of gcc, if native_read_tsc() 
>> is a simple static inline (as it should be) - the build would fail with 
>> certain gcc flags.
>
> I don't think that's true.  We rely on taking function pointers of 
> static inlines pretty extensively; native_read_tsc is hardly unique in 
> this respect.  I don't remember seeing any problems of the sort you 
> describe.  (I can well believe this may have been a problem at some 
> point, but not during the pv-ops development timeframe.)

i do remember build and boot failures there - with weird combos of gcc 
options. It's a clear GCC bug. Anyway, we can clean this up and we'll see 
how relevant the failure modes are.

>> Perhaps the real fix is to do this rename as well:
>>
>>     native_read_tsc => native_read_tsc_paravirt
>>   __native_read_tsc => native_read_tsc
>>
>> as this makes the native_read_tsc_paravirt() a pure technical variant, 
>> to be used in paravirt_ops function pointer assignments. People would 
>> thus just use the obvious native_read_tsc() inline function most of the 
>> time and could forget about native_read_tsc_paravirt().
>>
>> Jeremy?
>
> I'm trying to remember the real reason for 
> __native_read_tsc/native_read_tsc.  At least part of it is that 
> __native_read_tsc is used in a vdso, and so *must* be inlined to avoid a 
> bogus call from user to kernel space.  But I don't know why you wouldn't 
> want to inline native_read_tsc everywhere.  I have a feeling it may be a 
> relic from unification - possibly because x86-64 was late to the 
> clocksource party - but I don't remember anything specific.
>
> I think we can probably make do with a single native_read_tsc, so long 
> as its always inlined.

agreed mostly, with this twist: vdso inlining dependencies should be 
expressed explicitly, via:

  native_vread_tsc()

but we can also make native_read_tsc() __always_inline [it's a single 
instruction with basically no preparatory halo around that instruction] 
and document the vdso detail there.

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