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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
