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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.10.14 at 23:40, <boris.ostrovsky@xxxxxxxxxx> 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?

> +    case XENPMU_feature_get:
> +        memset(&pmu_params, 0, sizeof(pmu_params));
> +        pmu_params.val = vpmu_features;
> +        if ( copy_to_guest(arg, &pmu_params, 1) )

Other than for XENPMU_mode_get this could be had easier without
memset() if you used copy_field_to_guest().

> +/* Parameters structure for HYPERVISOR_xenpmu_op call */
> +struct xen_pmu_params {
> +    /* IN/OUT parameters */
> +    struct {
> +        uint32_t maj;
> +        uint32_t min;
> +    } version;
> +    uint64_t val;
> +
> +    /* IN parameters */
> +    uint64_t vcpu;

Why would we need 64 bits to represent a vCPU?

Jan


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