From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags Date: Wed, 26 Nov 2014 09:26:29 -0500 Message-ID: <5475E315.8050507@oracle.com> References: <1416179271-1221-1-git-send-email-boris.ostrovsky@oracle.com> <1416179271-1221-12-git-send-email-boris.ostrovsky@oracle.com> <547493D4020000780004AAF3@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <547493D4020000780004AAF3@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org 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