Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
On 12/04/2014 03:51 AM, Jan Beulich wrote:
On 03.12.14 at 21:13, wrote:
On 11/27/2014 03:57 AM, Jan Beulich wrote:
On 26.11.14 at 15:32, wrote:
On 11/25/2014 08:49 AM, Jan Beulich wrote:
On 17.11.14 at 00:07, wrote:
@@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
switch ( vendor )
{
case X86_VENDOR_AMD:
-if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( svm_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
case X86_VENDOR_INTEL:
-if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( vmx_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
So this turns off the vPMU globally upon failure of initializing
some random vCPU. Why is that? I see this was the case even
before your entire series, but shouldn't this be fixed _before_
enhancing the whole thing to support PV/PVH?
Yes, that's probably too strong. Do you want to fix this as an early
patch (before PV(H)) support gets in? I'd rather do it in the patch that
moves things into initcalls.
Yes, I think this should be fixed in a prereq patch, thus allowing it
to be easily backported if so desired.
I started to make this change and then I realized that the next patch
(12/21) is actually already taking care of this problem: most of the
*_vpmu_initialise() will be executed as initcalls during host boot and
if any of those fail then we do want to disable VPMU globally (those
failures would not be VCPU-specific).
Funny that you say that - it was actually while reviewing that next
patch when I made that observation: svm_vpmu_initialise() get a
-ENOMEM return _added_ there for example, which is contrary to
what I asked for above.
Oh, *that* initialization code (I was looking at vpmu_init()). Yes, VPMU
should not be turned off there.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
>>> On 03.12.14 at 21:13, wrote:
> On 11/27/2014 03:57 AM, Jan Beulich wrote:
> On 26.11.14 at 15:32, wrote:
>>> On 11/25/2014 08:49 AM, Jan Beulich wrote:
>>> On 17.11.14 at 00:07, wrote:
> @@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
>switch ( vendor )
>{
>case X86_VENDOR_AMD:
> -if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> -opt_vpmu_enabled = 0;
> +if ( svm_vpmu_initialise(v) != 0 )
> +vpmu_mode = XENPMU_MODE_OFF;
>break;
>
>case X86_VENDOR_INTEL:
> -if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> -opt_vpmu_enabled = 0;
> +if ( vmx_vpmu_initialise(v) != 0 )
> +vpmu_mode = XENPMU_MODE_OFF;
>break;
So this turns off the vPMU globally upon failure of initializing
some random vCPU. Why is that? I see this was the case even
before your entire series, but shouldn't this be fixed _before_
enhancing the whole thing to support PV/PVH?
>>> Yes, that's probably too strong. Do you want to fix this as an early
>>> patch (before PV(H)) support gets in? I'd rather do it in the patch that
>>> moves things into initcalls.
>> Yes, I think this should be fixed in a prereq patch, thus allowing it
>> to be easily backported if so desired.
>
> I started to make this change and then I realized that the next patch
> (12/21) is actually already taking care of this problem: most of the
> *_vpmu_initialise() will be executed as initcalls during host boot and
> if any of those fail then we do want to disable VPMU globally (those
> failures would not be VCPU-specific).
Funny that you say that - it was actually while reviewing that next
patch when I made that observation: svm_vpmu_initialise() get a
-ENOMEM return _added_ there for example, which is contrary to
what I asked for above.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
On 11/27/2014 03:57 AM, Jan Beulich wrote:
On 26.11.14 at 15:32, wrote:
On 11/25/2014 08:49 AM, Jan Beulich wrote:
On 17.11.14 at 00:07, wrote:
@@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
switch ( vendor )
{
case X86_VENDOR_AMD:
-if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( svm_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
case X86_VENDOR_INTEL:
-if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( vmx_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
So this turns off the vPMU globally upon failure of initializing
some random vCPU. Why is that? I see this was the case even
before your entire series, but shouldn't this be fixed _before_
enhancing the whole thing to support PV/PVH?
Yes, that's probably too strong. Do you want to fix this as an early
patch (before PV(H)) support gets in? I'd rather do it in the patch that
moves things into initcalls.
Yes, I think this should be fixed in a prereq patch, thus allowing it
to be easily backported if so desired.
I started to make this change and then I realized that the next patch
(12/21) is actually already taking care of this problem: most of the
*_vpmu_initialise() will be executed as initcalls during host boot and
if any of those fail then we do want to disable VPMU globally (those
failures would not be VCPU-specific).
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
>>> On 26.11.14 at 15:32, wrote:
> On 11/25/2014 08:49 AM, Jan Beulich wrote:
> On 17.11.14 at 00:07, wrote:
>>> @@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
>>> switch ( vendor )
>>> {
>>> case X86_VENDOR_AMD:
>>> -if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
>>> -opt_vpmu_enabled = 0;
>>> +if ( svm_vpmu_initialise(v) != 0 )
>>> +vpmu_mode = XENPMU_MODE_OFF;
>>> break;
>>>
>>> case X86_VENDOR_INTEL:
>>> -if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
>>> -opt_vpmu_enabled = 0;
>>> +if ( vmx_vpmu_initialise(v) != 0 )
>>> +vpmu_mode = XENPMU_MODE_OFF;
>>> break;
>> So this turns off the vPMU globally upon failure of initializing
>> some random vCPU. Why is that? I see this was the case even
>> before your entire series, but shouldn't this be fixed _before_
>> enhancing the whole thing to support PV/PVH?
>
> Yes, that's probably too strong. Do you want to fix this as an early
> patch (before PV(H)) support gets in? I'd rather do it in the patch that
> moves things into initcalls.
Yes, I think this should be fixed in a prereq patch, thus allowing it
to be easily backported if so desired.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
>>> On 26.11.14 at 15:26, wrote:
> On 11/25/2014 08:36 AM, Jan Beulich wrote:
>>
>>> +static long vpmu_sched_checkin(void *arg)
>>> +{
>>> +int cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
>> unsigned int.
>>
>>> +int ret;
>>> +
>>> +/* Mode change shouldn't take more than a few (say, 5) seconds. */
>>> +if ( NOW() > vpmu_start_ctxt_switch + SECONDS(5) )
>>> +{
>>> +ret = -EBUSY;
>>> +goto fail;
>>> +}
>> So what does this guard against? Holding xenpmu_mode_lock for
>> 5 seconds is not a problem, plus I don't think you actually need a
>> lock at all. Just using a global, atomically updated flag ought to be
>> sufficient (the way you use the lock is really nothing else afaict).
>
> I didn't put it here because of the lock. I just wanted to terminate
> this operation (mode change) if it takes unreasonable amount of time.
> And I thought 5 seconds would be unreasonable.
But the question you need to ask yourself is _why_ it could be
taking that long. Since all you do is repeatedly scheduling a
tasklet, it taking arbitrarily long can only be the effect of (a) a
huge system (extremely many CPUs) or (b) a hypervisor bug.
Neither of which is a reason for an arbitrary timeout like you put
in.
>>> +{
>>> +int ret;
>>> +
>>> +vpmu_start_ctxt_switch = NOW();
>>> +
>>> +ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
>>> +vpmu_sched_checkin, (void *)old_mode);
>>> +if ( ret )
>>> +vpmu_start_ctxt_switch = 0;
>>> +
>>> +return ret;
>>> +}
>> I don't think it is guaranteed (independent of implementation details
>> of continue_hypercall_on_cpu()) that this way you went through the
>> context switch path once on each CPU if
>> cpumask_first(&cpu_online_map) == smp_processor_id() here. In
>> particular it looks like there being a problem calling vcpu_sleep_sync()
>> in the tasklet handler when v == current (which would be the case
>> if you started on the "correct" CPU and the tasklet softirq got
>> handled before the scheduler one). I think you instead want to use
>> cpumask_cycle() here and above, and finish the whole operation
>> once you're back on the CPU you started on (the single-pCPU case
>> may need some extra consideration).
>
> So your concern is only because of initiating CPU? I can do a
> force_save() on it so I don't really need a context switch here. This
> will take care of single PCPU too.
Good.
>> I realize that you simply follow what microcode_update() does, but
>> I think we should rather correct that one than copy the latent issue
>> it has elsewhere.
>>
>>> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>>> +{
>>> +int ret;
>>> +struct xen_pmu_params pmu_params;
>>> +
>>> +ret = xsm_pmu_op(XSM_OTHER, current->domain, op);
>>> +if ( ret )
>>> +return ret;
>>> +
>>> +switch ( op )
>>> +{
>>> +case XENPMU_mode_set:
>>> +{
>>> +uint64_t old_mode;
>> Irrespective of the earlier comments I don't see why this isn't just
>> unsigned int (or typeof(vpmu_mode)).
>
> This was because vpmu_force_context switch() takes uint64_t as an
> argument. Now that it will become unsigned long this should too, no?
> Yes, the compiler will promote it if it is an int but I thought this
> would look cleaner.
Imo "cleaner" is when the variable is typed according to the value(s)
it is to hold, not according to the parameter types of functions it's
going to be passed to. And with operations on 32-bit types
(statistically) producing slightly smaller code, one could also argue
for using the smaller of the maximum width data source and the
maximum width consumer.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
On 11/25/2014 08:49 AM, Jan Beulich wrote:
On 17.11.14 at 00:07, wrote:
@@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
switch ( vendor )
{
case X86_VENDOR_AMD:
-if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( svm_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
case X86_VENDOR_INTEL:
-if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
-opt_vpmu_enabled = 0;
+if ( vmx_vpmu_initialise(v) != 0 )
+vpmu_mode = XENPMU_MODE_OFF;
break;
So this turns off the vPMU globally upon failure of initializing
some random vCPU. Why is that? I see this was the case even
before your entire series, but shouldn't this be fixed _before_
enhancing the whole thing to support PV/PVH?
Yes, that's probably too strong. Do you want to fix this as an early
patch (before PV(H)) support gets in? I'd rather do it in the patch that
moves things into initcalls.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
On 11/25/2014 08:36 AM, Jan Beulich wrote:
+static long vpmu_sched_checkin(void *arg)
+{
+int cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
unsigned int.
+int ret;
+
+/* Mode change shouldn't take more than a few (say, 5) seconds. */
+if ( NOW() > vpmu_start_ctxt_switch + SECONDS(5) )
+{
+ret = -EBUSY;
+goto fail;
+}
So what does this guard against? Holding xenpmu_mode_lock for
5 seconds is not a problem, plus I don't think you actually need a
lock at all. Just using a global, atomically updated flag ought to be
sufficient (the way you use the lock is really nothing else afaict).
I didn't put it here because of the lock. I just wanted to terminate
this operation (mode change) if it takes unreasonable amount of time.
And I thought 5 seconds would be unreasonable.
+{
+int ret;
+
+vpmu_start_ctxt_switch = NOW();
+
+ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
+vpmu_sched_checkin, (void *)old_mode);
+if ( ret )
+vpmu_start_ctxt_switch = 0;
+
+return ret;
+}
I don't think it is guaranteed (independent of implementation details
of continue_hypercall_on_cpu()) that this way you went through the
context switch path once on each CPU if
cpumask_first(&cpu_online_map) == smp_processor_id() here. In
particular it looks like there being a problem calling vcpu_sleep_sync()
in the tasklet handler when v == current (which would be the case
if you started on the "correct" CPU and the tasklet softirq got
handled before the scheduler one). I think you instead want to use
cpumask_cycle() here and above, and finish the whole operation
once you're back on the CPU you started on (the single-pCPU case
may need some extra consideration).
So your concern is only because of initiating CPU? I can do a
force_save() on it so I don't really need a context switch here. This
will take care of single PCPU too.
I realize that you simply follow what microcode_update() does, but
I think we should rather correct that one than copy the latent issue
it has elsewhere.
+long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
+{
+int ret;
+struct xen_pmu_params pmu_params;
+
+ret = xsm_pmu_op(XSM_OTHER, current->domain, op);
+if ( ret )
+return ret;
+
+switch ( op )
+{
+case XENPMU_mode_set:
+{
+uint64_t old_mode;
Irrespective of the earlier comments I don't see why this isn't just
unsigned int (or typeof(vpmu_mode)).
This was because vpmu_force_context switch() takes uint64_t as an
argument. Now that it will become unsigned long this should too, no?
Yes, the compiler will promote it if it is an int but I thought this
would look cleaner.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
>>> On 17.11.14 at 00:07, wrote:
> @@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
> switch ( vendor )
> {
> case X86_VENDOR_AMD:
> -if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> -opt_vpmu_enabled = 0;
> +if ( svm_vpmu_initialise(v) != 0 )
> +vpmu_mode = XENPMU_MODE_OFF;
> break;
>
> case X86_VENDOR_INTEL:
> -if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> -opt_vpmu_enabled = 0;
> +if ( vmx_vpmu_initialise(v) != 0 )
> +vpmu_mode = XENPMU_MODE_OFF;
> break;
So this turns off the vPMU globally upon failure of initializing
some random vCPU. Why is that? I see this was the case even
before your entire series, but shouldn't this be fixed _before_
enhancing the whole thing to support PV/PVH?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags
>>> On 17.11.14 at 00:07, wrote:
> @@ -278,3 +290,139 @@ void vpmu_dump(struct vcpu *v)
> vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
> }
>
> +static s_time_t vpmu_start_ctxt_switch;
Blank line here please.
> +static long vpmu_sched_checkin(void *arg)
> +{
> +int cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
unsigned int.
> +int ret;
> +
> +/* Mode change shouldn't take more than a few (say, 5) seconds. */
> +if ( NOW() > vpmu_start_ctxt_switch + SECONDS(5) )
> +{
> +ret = -EBUSY;
> +goto fail;
> +}
So what does this guard against? Holding xenpmu_mode_lock for
5 seconds is not a problem, plus I don't think you actually need a
lock at all. Just using a global, atomically updated flag ought to be
sufficient (the way you use the lock is really nothing else afaict).
> +
> +if ( cpu < nr_cpu_ids )
> +{
> +ret = continue_hypercall_on_cpu(cpu, vpmu_sched_checkin, arg);
> +if ( ret )
> +goto fail;
> +}
> +else
> +vpmu_start_ctxt_switch = 0;
> +
> +return 0;
> +
> + fail:
> +vpmu_mode = (uint64_t)arg;
Even if we don't support x86-32 anymore, please avoid giving bad
examples: Casts between pointers and integers should always use
(unsigned) long on the integer side.
> +vpmu_start_ctxt_switch = 0;
> +return ret;
> +}
> +
> +static int vpmu_force_context_switch(uint64_t old_mode)
Same comment as above regarding the type.
> +{
> +int ret;
> +
> +vpmu_start_ctxt_switch = NOW();
> +
> +ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
> +vpmu_sched_checkin, (void *)old_mode);
> +if ( ret )
> +vpmu_start_ctxt_switch = 0;
> +
> +return ret;
> +}
I don't think it is guaranteed (independent of implementation details
of continue_hypercall_on_cpu()) that this way you went through the
context switch path once on each CPU if
cpumask_first(&cpu_online_map) == smp_processor_id() here. In
particular it looks like there being a problem calling vcpu_sleep_sync()
in the tasklet handler when v == current (which would be the case
if you started on the "correct" CPU and the tasklet softirq got
handled before the scheduler one). I think you instead want to use
cpumask_cycle() here and above, and finish the whole operation
once you're back on the CPU you started on (the single-pCPU case
may need some extra consideration).
I realize that you simply follow what microcode_update() does, but
I think we should rather correct that one than copy the latent issue
it has elsewhere.
> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
> +{
> +int ret;
> +struct xen_pmu_params pmu_params;
> +
> +ret = xsm_pmu_op(XSM_OTHER, current->domain, op);
> +if ( ret )
> +return ret;
> +
> +switch ( op )
> +{
> +case XENPMU_mode_set:
> +{
> +uint64_t old_mode;
Irrespective of the earlier comments I don't see why this isn't just
unsigned int (or typeof(vpmu_mode)).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
