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

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

On 11/25/2014 08:36 AM, Jan Beulich wrote:
>
>> +static long vpmu_sched_checkin(void *arg)
>> +{
>> +    int cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
> unsigned int.
>
>> +    int ret;
>> +
>> +    /* Mode change shouldn't take more than a few (say, 5) seconds. */
>> +    if ( NOW() > vpmu_start_ctxt_switch + SECONDS(5) )
>> +    {
>> +        ret = -EBUSY;
>> +        goto fail;
>> +    }
> So what does this guard against? Holding xenpmu_mode_lock for
> 5 seconds is not a problem, plus I don't think you actually need a
> lock at all. Just using a global, atomically updated flag ought to be
> sufficient (the way you use the lock is really nothing else afaict).

I didn't put it here because of the lock. I just wanted to terminate 
this operation (mode change) if it takes unreasonable amount of time. 
And I thought 5 seconds would be unreasonable.


>
>> +{
>> +    int ret;
>> +
>> +    vpmu_start_ctxt_switch = NOW();
>> +
>> +    ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
>> +                                    vpmu_sched_checkin, (void *)old_mode);
>> +    if ( ret )
>> +        vpmu_start_ctxt_switch = 0;
>> +
>> +    return ret;
>> +}
> I don't think it is guaranteed (independent of implementation details
> of continue_hypercall_on_cpu()) that this way you went through the
> context switch path once on each CPU if
> cpumask_first(&cpu_online_map) == smp_processor_id() here. In
> particular it looks like there being a problem calling vcpu_sleep_sync()
> in the tasklet handler when v == current (which would be the case
> if you started on the "correct" CPU and the tasklet softirq got
> handled before the scheduler one). I think you instead want to use
> cpumask_cycle() here and above, and finish the whole operation
> once you're back on the CPU you started on (the single-pCPU case
> may need some extra consideration).

So your concern is only because of initiating CPU? I can do a 
force_save() on it so I don't really need a context switch here. This 
will take care of single PCPU too.

>
> I realize that you simply follow what microcode_update() does, but
> I think we should rather correct that one than copy the latent issue
> it has elsewhere.
>
>> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    int ret;
>> +    struct xen_pmu_params pmu_params;
>> +
>> +    ret = xsm_pmu_op(XSM_OTHER, current->domain, op);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    switch ( op )
>> +    {
>> +    case XENPMU_mode_set:
>> +    {
>> +        uint64_t old_mode;
> Irrespective of the earlier comments I don't see why this isn't just
> unsigned int (or typeof(vpmu_mode)).

This was because vpmu_force_context switch() takes uint64_t as an 
argument. Now that it will become unsigned long this should too, no? 
Yes, the compiler will promote it if it is an int but I thought this 
would look cleaner.


-boris