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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.14 at 05:41, <boris.ostrovsky@xxxxxxxxxx> 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 ...

> @@ -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.

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

> +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

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