Re: [PATCH v13 for-xen-4.5 11/21] x86/VPMU: Interface for setting PMU mode and flags

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

On 10/13/2014 09:51 AM, Jan Beulich wrote:
>>>> On 03.10.14 at 23:40, <boris.ostrovsky@oracle.com> wrote:
>> @@ -274,3 +292,176 @@ void vpmu_dump(struct vcpu *v)
>>           vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
>>   }
>>   
>> +static atomic_t vpmu_sched_counter;
>> +
>> +static void vpmu_sched_checkin(unsigned long unused)
>> +{
>> +    atomic_inc(&vpmu_sched_counter);
>> +}
>> +
>> +static int vpmu_force_context_switch(void)
>> +{
>> +    unsigned i, numcpus, mycpu;
>> +    s_time_t start;
>> +    static DEFINE_PER_CPU(struct tasklet *, sync_task);
>> +    int ret = 0;
>> +
>> +    numcpus = num_online_cpus();
>> +    mycpu = smp_processor_id();
>> +
>> +    for ( i = 0; i < numcpus; i++ )
> for_each_online_cpu() would have saved you from running into ...
>
>> +    {
>> +        if ( i == mycpu )
>> +            continue;
>> +
>> +        per_cpu(sync_task, i) = xmalloc(struct tasklet);
> ... a crash here when some CPU other than the highest numbered
> one got offlined.
>
>> +        if ( per_cpu(sync_task, i) == NULL )
>> +        {
>> +            printk(XENLOG_WARNING "vpmu_force_context_switch: out of memory\n");
>> +            ret = -ENOMEM;
>> +            goto out;
>> +        }
>> +        tasklet_init(per_cpu(sync_task, i), vpmu_sched_checkin, 0);
>> +    }
>> +
>> +    /* First count is for self */
>> +    atomic_set(&vpmu_sched_counter, 1);
>> +
>> +    for_each_online_cpu( i )
> And oddly enough you use it here (albeit with malformed style -
> either you drop the two blanks inside the parentheses or you add
> another before the opening one)...
>
>> +    {
>> +        if ( i != mycpu )
>> +            tasklet_schedule_on_cpu(per_cpu(sync_task, i), i);
>> +    }
>> +
>> +    vpmu_save(current);
>> +
>> +    start = NOW();
>> +
>> +    /*
>> +     * Note that we may fail here if a CPU is hot-plugged while we are
>> +     * waiting. We will then time out.
>> +     */
>> +    while ( atomic_read(&vpmu_sched_counter) != numcpus )
>> +    {
>> +        s_time_t now;
>> +
>> +        cpu_relax();
>> +
>> +        now = NOW();
>> +
>> +        /* Give up after 5 seconds */
>> +        if ( now > start + SECONDS(5) )
>> +        {
>> +            printk(XENLOG_WARNING
>> +                   "vpmu_force_context_switch: failed to sync\n");
>> +            ret = -EBUSY;
>> +            break;
>> +        }
>> +
>> +        /* Or after (arbitrarily chosen) 2ms if need to be preempted */
>> +        if ( (now > start + MILLISECS(2)) && hypercall_preempt_check() )
>> +        {
>> +            ret = -EAGAIN;
>> +            break;
>> +        }
>> +    }
> So this time round you don't retain any state between retries at all.
> How is this process expected to ever complete on a large and loaded
> enough system?

Since we now leave completion of the mode change request to user we 
can't guarantee that it will ever happen. So if we were to keep the 
state of who has passed through the context switch we'd have to prevent 
VPMUs on those CPUs from being loaded again. We don't have such 
mechanism now (unless we introduce a per-CPU VPMU mode which is not a 
good idea imo).

With continuations we could leave VPMU mode in new state and revert it 
to old if continuations failed but with things left to user we can't do 
this.

This has been the case for the last few versions btw.

-boris