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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

On 12/07/16 05:45, David Matlack wrote:
> On Mon, Jul 11, 2016 at 12:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 11/07/2016 19:30, David Matlack wrote:
>>> On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>> On 11/07/2016 18:51, David Matlack wrote:
>>>>>>> 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.

>
>> Paolo