Re: [PATCH V2 4/5] kvm/stats: Add provisioning for 64-bit vcpu statistics

On 12/07/16 05:45, David Matlack wrote:

On Mon, Jul 11, 2016 at 12:31 PM, Paolo Bonzini [off-list ref] wrote:

quoted

On 11/07/2016 19:30, David Matlack wrote:

quoted

On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:

quoted

On 11/07/2016 18:51, David Matlack wrote:

quoted

quoted

quoted

vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.

Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".

I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.

I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.

Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.

I think that's ok, none of the stats currently use atomic operations.

Yeah so this patch pretty much duplicates the 32-bit code.

So what you're saying is just replace all of the 32-bit statistics with
longs, that way we get 32-bit on 32-bit machines and 64-bit on 64-bit
machines? Then we just accept that on 32-bit machines we will get overflow
on some stats.

Or do you think u64s would be better and we accept that on 32-bit machines
we might get update conflicts from non-atomic concurrent accesses? Which
honestly I don't see being a huge issue in this use case.

quoted

Paolo