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

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

On 09/10/2014 11:05 AM, Jan Beulich wrote:
>>>> On 04.09.14 at 05:41, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1499,8 +1499,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>   
>>       if ( is_hvm_vcpu(prev) )
>>       {
>> -        if (prev != next)
>> -            vpmu_save(prev);
>> +        vpmu_switch_from(prev, next);
> It escapes me why you move the "prev != next" check here ...

No need to, indeed.

>
>> @@ -1543,9 +1542,9 @@ 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(prev) )
>>           /* Must be done with interrupts enabled */
>> -        vpmu_load(next);
>> +        vpmu_switch_to(prev, next);
> ... and here into the wrapper functions.
>
>> +static int
>> +vpmu_force_context_switch(XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    unsigned i, j, allbutself_num, tasknum, mycpu;
>> +    static s_time_t start;
>> +    static struct tasklet **sync_task;
>> +    struct vcpu *curr_vcpu = current;
>> +    static struct vcpu *sync_vcpu;
>> +    int ret = 0;
>> +
>> +    tasknum = allbutself_num = num_online_cpus() - 1;
>> +
>> +    if ( sync_task ) /* if set, we are in hypercall continuation */
>> +    {
>> +        if ( (sync_vcpu != NULL) && (sync_vcpu != curr_vcpu) )
>> +            /* We are not the original caller */
>> +            return -EAGAIN;
>> +        goto cont_wait;
>> +    }
>> +
>> +    sync_task = xmalloc_array(struct tasklet *, allbutself_num);
>> +    if ( !sync_task )
>> +    {
>> +        printk(XENLOG_WARNING "vpmu_force_context_switch: out of memory\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    for ( tasknum = 0; tasknum < allbutself_num; tasknum++ )
>> +    {
>> +        sync_task[tasknum] = xmalloc(struct tasklet);
>> +        if ( sync_task[tasknum] == NULL )
>> +        {
>> +            printk(XENLOG_WARNING "vpmu_force_context_switch: out of memory\n");
>> +            ret = -ENOMEM;
>> +            goto out;
>> +        }
>> +        tasklet_init(sync_task[tasknum], vpmu_sched_checkin, 0);
>> +    }
>> +
>> +    atomic_set(&vpmu_sched_counter, 0);
>> +    sync_vcpu = curr_vcpu;
>> +
>> +    j = 0;
>> +    mycpu = smp_processor_id();
>> +    for_each_online_cpu( i )
>> +    {
>> +        if ( i != mycpu )
>> +            tasklet_schedule_on_cpu(sync_task[j++], i);
>> +    }
>> +
>> +    vpmu_save(curr_vcpu);
>> +
>> +    start = NOW();
>> +
>> + cont_wait:
>> +    /*
>> +     * Note that we may fail here if a CPU is hot-(un)plugged 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(XENLOG_WARNING
>> +                   "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);
>> +    }
> I wouldn't complain about this not being synchronized with CPU
> hotplug if there wasn't this hypercall continuation and relatively
> long timeout. Much of the state you latch in static variables will
> cause this operation to time out if in between a CPU got brought
> down.

It seemed to me that if we were to correctly deal with CPU hotplug it 
would add a bit too much complexity to the code. So I felt that letting 
the operation timeout would be a better way out.

>
> And as already alluded to, all this looks rather fragile anyway,
> even if I can't immediately spot any problems with it anymore.

The continuation is really a carry-over from earlier patch version when 
I had double loops over domain and VCPUs to explicitly unload VPMUs. At 
that time Andrew pointed out that these loops may take really long time 
and so I added continuations.

Now that I changed that after realizing that having each PCPU go through 
a context switch is sufficient perhaps I don't need it any longer. Is 
the worst case scenario of being stuck here for 5 seconds (chosen 
somewhat arbitrary) acceptable without continuation?


-boris

>
>> +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;
> So what happens if you can't take the lock in a continuation? If
> returning -EAGAIN in that case is not a problem, what do you
> need the continuation for in the first place?
>
> Jan