Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2023 16:07, Matias Ezequiel Vara Larsen wrote:
> On Wed, Dec 14, 2022 at 08:29:53AM +0100, Jan Beulich wrote:
>> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
>>> @@ -287,6 +289,20 @@ static inline void vcpu_runstate_change(
>>>      }
>>>  
>>>      v->runstate.state = new_state;
>>> +
>>> +    vcpustats_va = (shared_vcpustatspage_t*)d->vcpustats_page.va;
>>> +    if ( vcpustats_va )
>>> +    {
>>> +   vcpustats_va->vcpu_info[v->vcpu_id].version =
>>> +       version_update_begin(vcpustats_va->vcpu_info[v->vcpu_id].version);
>>> +        smp_wmb();
>>> +        memcpy(&vcpustats_va->vcpu_info[v->vcpu_id].runstate_running_time,
>>> +               &v->runstate.time[RUNSTATE_running],
>>> +               sizeof(v->runstate.time[RUNSTATE_running]));
>>> +        smp_wmb();
>>> +        vcpustats_va->vcpu_info[v->vcpu_id].version =
>>> +            
>>> version_update_end(vcpustats_va->vcpu_info[v->vcpu_id].version);
>>> +    }
>>
>> A further aspect to consider here is cache line ping-pong. I think the
>> per-vCPU elements of the array want to be big enough to not share a
>> cache line. The interface being generic this presents some challenge
>> in determining what the supposed size is to be. However, taking into
>> account the extensibility question, maybe the route to take is to
>> simply settle on a power-of-2 value somewhere between x86'es and Arm's
>> cache line sizes and the pretty common page size of 4k, e.g. 512 bytes
>> or 1k?
>>
> 
> I do not now how to address this. I was thinking to align each vcpu_stats
> instance to a multiple of the cache-line. I would pick up the first multiple
> that is bigger to the size of the vcpu_stats structure. For example, currently
> the structure is 16 bytes so I would align each instance in a frame to 64
> bytes. Would it make sense? 

Well, 64 may be an option, but I gave higher numbers for a reason. One thing
I don't know is what common cache line sizes are on Arm or e.g. RISC-V.

>>> --- a/xen/include/public/vcpu.h
>>> +++ b/xen/include/public/vcpu.h
>>> @@ -235,6 +235,22 @@ struct vcpu_register_time_memory_area {
>>>  typedef struct vcpu_register_time_memory_area 
>>> vcpu_register_time_memory_area_t;
>>>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>>  
>>> +struct vcpu_stats{
>>> +    /* If the least-significant bit of the version number is set then an 
>>> update
>>> +     * is in progress and the guest must wait to read a consistent set of 
>>> values
>>> +     * This mechanism is similar to Linux's seqlock.
>>> +     */
>>> +    uint32_t version;
>>> +    uint32_t pad0;
>>> +    uint64_t runstate_running_time;
>>> +};
>>> +
>>> +struct shared_vcpustatspage {
>>> +    struct vcpu_stats vcpu_info[1];
>>> +};
>>> +
>>> +typedef struct shared_vcpustatspage shared_vcpustatspage_t;
>>
>> For new additions please avoid further name space issues: All types
>> and alike want to be prefixed by "xen_".
> 
> Should I name it "xen_shared_vcpustatspage_t" instead?

Yes, that would fulfill the name space requirements. It's getting longish,
so you may want to think about abbreviating it some. For example, I'm not
sure the "page" in the name is really necessary.

Jan