[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 10/19] x86/VPMU: Interface for setting PMU mode and flags
>>> On 28.07.14 at 18:29, <boris.ostrovsky@xxxxxxxxxx> wrote: > On 07/28/2014 11:22 AM, Jan Beulich wrote: >>>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote: >>> + start = NOW(); >>> + /* >>> + * Note that we may fail here if a CPU is hot-unplugged 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("vpmu_unload_all: 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 wonder whether this is race free (wrt another CPU doing something >> similar) and how you expect the 5s timeout above to ever be reached >> (you're virtually guaranteed to get asked to preempt earlier). > > Race-wise there is xenpmu_mode_lock in the caller (quoted below). That wasn't my point: I said "something similar" - imagine another hypercall behaving this same way, and both hypercalls getting run concurrently. >>> +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_ON ) >>> + return -EINVAL; >>> + >>> + if ( !spin_trylock(&xenpmu_mode_lock) ) >>> + return -EAGAIN; >> Wouldn't it be better for this to also set a continuation, rather than >> having the caller do the retry? > > I actually want the caller (who is most likely the administrator doing > 'echo off > /sys/hypervisor/pmu/pmu_mode') see the error since this > indicates two people trying to change system-wise settings at the same time. Then please state this in a code comment, or someone (like me) might end up "cleaning" this up. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |