Re: [PATCH v9 11/20] x86/VPMU: Interface for setting PMU mode and flags

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/12/2014 06:37 AM, Jan Beulich wrote:
>>>> On 08.08.14 at 18:55, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1482,7 +1482,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>   
>>       if ( is_hvm_vcpu(prev) )
>>       {
>> -        if (prev != next)
>> +        if ( (prev != next) && (vpmu_mode & XENPMU_MODE_SELF) )
>>               vpmu_save(prev);
>>   
>>           if ( !list_empty(&prev->arch.hvm_vcpu.tm_list) )
>> @@ -1526,7 +1526,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>                              !is_hardware_domain(next->domain));
>>       }
>>   
>> -    if (is_hvm_vcpu(next) && (prev != next) )
>> +    if ( is_hvm_vcpu(next) && (prev != next) && (vpmu_mode & XENPMU_MODE_SELF) )
>>           /* Must be done with interrupts enabled */
>>           vpmu_load(next);
> Wouldn't such vPMU internals be better hidden in the functions
> themselves? I realize you can save the calls this way, but if the
> condition changes again later, we'll again have to adjust this
> core function rather than just the vPMU code. It's bad enough that
> the vpmu_mode variable is visible to non-vPMU code.

How about an inline function?

>
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -21,6 +21,8 @@
>>   #include <xen/config.h>
>>   #include <xen/sched.h>
>>   #include <xen/xenoprof.h>
>> +#include <xen/event.h>
>> +#include <xen/guest_access.h>
>>   #include <asm/regs.h>
>>   #include <asm/types.h>
>>   #include <asm/msr.h>
>> @@ -32,13 +34,21 @@
>>   #include <asm/hvm/svm/vmcb.h>
>>   #include <asm/apic.h>
>>   #include <public/pmu.h>
>> +#include <xen/tasklet.h>
>> +
>> +#include <compat/pmu.h>
>> +CHECK_pmu_params;
>> +CHECK_pmu_intel_ctxt;
>> +CHECK_pmu_amd_ctxt;
>> +CHECK_pmu_cntr_pair;
> Such being placed in a HVM-specific file suggests that the series is badly
> ordered: Anything relevant to PV should normally only be added here
> _after_ the file got moved out of the hvm/ subtree. Yet since I realize
> this would be a major re-work, I think you can leave this as is unless
> you expect potentially just part of the series to go in, with the splitting
> point being (immediately or later) after this patch (from my looking at
> it I would suppose that patches 3 and 4 could go in right away - they
> don't appear to depend on patches 1 and 2 - and patch 1 probably
> could go in too, but it doesn't make much sense to have it in without
> the rest of the series).

I would prefer the whole series to get in in one go (with patch 2 
possibly being the only exception). All other patches (including patch 
1) are not particularly useful on their own.

>> +        goto cont_wait;
>> +
>> +    cpumask_andnot(&allbutself, &cpu_online_map,
>> +                   cpumask_of(smp_processor_id()));
>> +
>> +    sync_task = xmalloc_array(struct tasklet, allbutself_num);
>> +    if ( !sync_task )
>> +    {
>> +        printk("vpmu_force_context_switch: out of memory\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    for ( i = 0; i < allbutself_num; i++ )
>> +        tasklet_init(&sync_task[i], vpmu_sched_checkin, 0);
>> +
>> +    atomic_set(&vpmu_sched_counter, 0);
>> +
>> +    j = 0;
>> +    for_each_cpu ( i, &allbutself )
> This looks to be the only use for the (on stack) allbutself variable,
> but you could easily avoid this by using for_each_online_cpu() and
> skipping the local one. I'd also recommend that you count
> allbutself_num here rather than up front, since that will much more
> obviously prove that you wait for exactly as many CPUs as you
> scheduled. The array allocation above is bogus anyway, as on a
> huge system this can easily be more than a page in size.

I don't understand the last sentence. What does page-sized allocation 
have to do with this?

>
>> +        tasklet_schedule_on_cpu(&sync_task[j++], i);
>> +
>> +    vpmu_save(current);
>> +
>> +    start = NOW();
>> +
>> + cont_wait:
>> +    /*
>> +     * Note that we may fail here if a CPU is hot-unplugged while we are
>> +     * waiting. We will then time out.
>> +     */
>> +    while ( atomic_read(&vpmu_sched_counter) != allbutself_num )
>> +    {
>> +        /* Give up after 5 seconds */
>> +        if ( NOW() > start + SECONDS(5) )
>> +        {
>> +            printk("vpmu_force_context_switch: failed to sync\n");
>> +            ret = -EBUSY;
>> +            break;
>> +        }
>> +        cpu_relax();
>> +        if ( hypercall_preempt_check() )
>> +            return hypercall_create_continuation(
>> +                __HYPERVISOR_xenpmu_op, "ih", XENPMU_mode_set, arg);
>> +    }
>> +
>> +    for ( i = 0; i < allbutself_num; i++ )
>> +        tasklet_kill(&sync_task[i]);
>> +    xfree(sync_task);
>> +    sync_task = NULL;
>> +
>> +    return ret;
>> +}
>> +
>> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    int ret = -EINVAL;
>> +    xen_pmu_params_t pmu_params;
>> +
>> +    switch ( op )
>> +    {
>> +    case XENPMU_mode_set:
>> +    {
>> +        static DEFINE_SPINLOCK(xenpmu_mode_lock);
>> +        uint32_t current_mode;
>> +
>> +        if ( !is_control_domain(current->domain) )
>> +            return -EPERM;
>> +
>> +        if ( copy_from_guest(&pmu_params, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        if ( pmu_params.val & ~XENPMU_MODE_SELF )
>> +            return -EINVAL;
>> +
>> +        /*
>> +         * Return error is someone else is in the middle of changing mode ---
>> +         * this is most likely indication of two system administrators
>> +         * working against each other
>> +         */
>> +        if ( !spin_trylock(&xenpmu_mode_lock) )
>> +            return -EAGAIN;
>> +
>> +        current_mode = vpmu_mode;
>> +        vpmu_mode = pmu_params.val;
>> +
>> +        if ( vpmu_mode == XENPMU_MODE_OFF )
>> +        {
>> +            /*
>> +             * Make sure all (non-dom0) VCPUs have unloaded their VPMUs. This
>> +             * can be achieved by having all physical processors go through
>> +             * context_switch().
>> +             */
>> +            ret = vpmu_force_context_switch(arg);
>> +            if ( ret )
>> +                vpmu_mode = current_mode;
>> +        }
>> +        else
>> +            ret = 0;
>> +
>> +        spin_unlock(&xenpmu_mode_lock);
>> +        break;
> This still isn't safe: There's nothing preventing another vCPU to issue
> another XENPMU_mode_set operation while the one turning the vPMU
> off is still in the process of waiting, but having exited the lock protected
> region in order to allow other processing to occur.

I think that's OK: one of these two competing requests will timeout in 
the while loop of vpmu_force_context_switch(). It will, in fact, likely 
be the first caller because the second one will get right into the while 
loop, without setting up the waiting data. This is a little 
counter-intuitive but trying to set mode simultaneously from two 
different places is asking for trouble anyway.


> I think you simply
> need another mode "being-turned-off" during which only mode
> changes to XENPMU_MODE_OFF, and only by the originally requesting
> vCPU, are permitted (or else your "if true, we are in hypercall
> continuation" comment above wouldn't always be true either, as that
> second vCPU might also issue a second turn-off request).

-boris