Re: [PATCH v4 1/5] x86: allow reading MSR_IA32_TSC with XENPF_resource_op

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/01/15 11:44, Jan Beulich wrote:
>>>> On 20.01.15 at 12:20, <chao.p.peng@linux.intel.com> wrote:
>> On Tue, Jan 20, 2015 at 10:02:32AM +0000, Jan Beulich wrote:
>>>>>> On 20.01.15 at 10:45, <chao.p.peng@linux.intel.com> wrote:
>>>> Memory bandwidth monitoring requires timestamp returned along with the
>>>> monitoring counter to verify the correctness of the counter value.
>>>> Allow MSR_IA32_TSC to be read and increase RESOURCE_ACCESS_MAX_ENTRIES
>>>> to 3.
>>> Do you really need an MSR access here, i.e. is RDTSC not
>>> sufficient?
>> RDTSC is enough. The purpose of using MSR is to reuse the existed
>> XEN_RESOURCE_OP_MSR_READ command. A new command XEN_RESOURCE_OP_TSC_READ
>> (looks like not so generic) is needed If using RDTSC. If it’s not the
>> issue, then RDTSC can be used instead.
> I don't see the need for a sub-op - just filter out MSR_IA32_TSC
> in the existing MSR read/write code paths.
>
>>>> --- a/xen/arch/x86/platform_hypercall.c
>>>> +++ b/xen/arch/x86/platform_hypercall.c
>>>> @@ -61,14 +61,14 @@ long cpu_down_helper(void *data);
>>>>  long core_parking_helper(void *data);
>>>>  uint32_t get_cur_idle_nums(void);
>>>>  
>>>> -#define RESOURCE_ACCESS_MAX_ENTRIES 2
>>>> +#define RESOURCE_ACCESS_MAX_ENTRIES 3
>>> No - the limit on the number of consecutive entries doesn't change
>>> because of your addition of another MSR. Actual batching is to be
>>> done via multicalls, the multi-entry requests here are meant to be
>>> used for successive operations that _must_ be issued without
>>> preemption (e.g. an MSR write needed for a successive read).
>>>
>> You remind me that the requirement here is more than "without
>> preemption", ideally the exact timestamp the moment that the counter
>> being read should be returned, which is used by used space to calculate
>> the time elapsed between two samplings.  So the MSR read and TSC
>> read should be atomic, which means, 1) preemption should not happen,
>> 2) interrupt should be disabled. Sounds more complex as we have no existed
>> way to disable interrupt, using back-to-back flag again?
> Yeah, in that case pairing the access would be necessary. And
> we have a rsvd field in struct xenpf_resource_entry, which you
> could use to request IRQ disabling across the pair of accesses.

In terms of the actual time measurement, would it not be better to use
NOW() instead?  That will allow you to get already-scaled ns instead of
having to interpret raw tsc values.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel