Re: [PATCH 2/5] kvmtrace: make cycle calculation architecture aware

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Hollis Blanchard wrote:
> On Mon, 2008-07-07 at 15:56 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>   
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>
>> The current implementation of kvmtrace uses always a 64 bit cycle variable,
>> but get_cycles() which is used to fill it is "unsigned long" which might be 32
>> bit.
>> This reduces the accuracy e.g. on embedded powerpc since we would have a 64bit
>> value but get_cycle() only returns the low 32 bit.
>> To solve that this patch introduces kvm_arch_trace_cycles() which allows us
>> to make this calculation architecture aware. That way every architecture can
>> insert whatever fits best for their "kvmtrace cycle counter".
>>
>> Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>     
>
> "cycles" is a very poor name, because that's not really what we're
> talking about at all. (Also, that function name made me wonder what a
> "trace cycle" is. :)
>
> I would strongly prefer using "timestamp" instead. It would be nice if
> we could rename the data structure too, but I'd settle for only properly
> naming the new architecture function hook.
>   
> In fact, if we want to be rigorous about it, it should really be
> something like "nanoseconds" instead, so that userspace wouldn't need to
> perform awkward conversions of "cycles" or "timebase ticks" to real
> time. It looks like getnstimeofday() would do the trick, and that way we
> wouldn't need an arch-specific hook at all
I agree that the naming is poor. I also wondered about it and just 
continued it to the kvm_arch function.
I'll change the naming - timestamp would be fine, but only if it is 
really time and not a cycle counter.
I experimented with the tv_nsec portion and the classic current_time and 
accuracy there was just far to low compared to the time base register 
data (5 to 20 instructions until tv.nssec portion changed).

I checked the getnstimeofday function you mentioned and compared 
accuracy again and yes that is exactly what I would like to use:
here a ittle example
263527418457 (+   36696)    ns=0x1c43286a
263527740677 (+  322220)   ns=0x1c4a880c (+ 75FA2 = 483234)
263527779757 (+   39080)    ns=0x1c4b6cae (+ E4A2   =  58530)
While the unit is not the same (explains the difference in the same line 
e.g. 39080 vs. 58530) the accuracy is the same.
this can be seen due to the fact that it changes by the same ratio all 
over the file.
For the example I had above
cycle = 322220/39080 = 8.25
nsec = 483234/58530 = 8.25

That leads me to the point where I completely agree with Hollis that the 
naming should be timestamp as well as that the function should rely on 
getnstimeofday() instead of get_cycles() and that way would remove the 
need kvm_arch_ function.

So the question that is left before changing that is, if the original 
author had something special in mind chosing cycles here.
I added Eric on CC for that.

I wait with my resubmission of the patch series until all architectures 
agree *hope* on using getnstimeofday() - after an ack from all sides I 
would revise my patch series and submit that changes alltogether.

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization