From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/4] xen: sched: reorganize cpu_disable_scheduler() Date: Wed, 8 Jul 2015 18:37:24 +0200 Message-ID: <1436373444.22672.256.camel@citrix.com> References: <20150703152743.23194.15530.stgit@Solace.station> <20150703154930.23194.20319.stgit@Solace.station> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6432939706872182755==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZCsMO-0007df-QG for xen-devel@lists.xenproject.org; Wed, 08 Jul 2015 16:38:13 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Juergen Gross , xen-devel List-Id: xen-devel@lists.xenproject.org --===============6432939706872182755== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-UwVDGMCp5o+GsWU6+RdI" --=-UwVDGMCp5o+GsWU6+RdI Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2015-07-08 at 17:01 +0100, George Dunlap wrote: > On Fri, Jul 3, 2015 at 4:49 PM, Dario Faggioli > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -455,8 +455,8 @@ void vcpu_unblock(struct vcpu *v) > > * Do the actual movemet of a vcpu from old to new CPU. Locks for *bot= h* > > * CPUs needs to have been taken already when calling this! > > */ > > -static void vcpu_move(struct vcpu *v, unsigned int old_cpu, > > - unsigned int new_cpu) > > +static void _vcpu_move(struct vcpu *v, unsigned int old_cpu, > > + unsigned int new_cpu) > > { > > /* > > * Transfer urgency status to new CPU before switching CPUs, as > > @@ -479,6 +479,35 @@ static void vcpu_move(struct vcpu *v, unsigned int= old_cpu, > > v->processor =3D new_cpu; > > } > > > > +static void vcpu_move(struct vcpu *v, unsigned int new_cpu) >=20 > Not quite a fan of the naming scheme here. Perhaps vcpu_move and > vcpu_move_locked? >=20 I'm fine with that. > Actually -- looking at this again, is there a reason to pass old_cpu > into _vcpu_move? It looks like old_vcpu should always be equal to > v->processor. That would make the function prototypes the same. >=20 It should yes, I think I can get rid of it. > > +{ > > + unsigned long flags; > > + unsigned int cpu =3D v->processor; > > + spinlock_t *lock, *new_lock; > > + > > + /* > > + * When here, the vcpu should be ready for being moved. This means= : > > + * - both its original and target processor must be quiet; > > + * - it must not be marked as currently running; > > + * - the proper flag must be set (i.e., no one must have had any > > + * chance to reset it). > > + */ > > + ASSERT(is_idle_vcpu(curr_on_cpu(cpu)) && > > + is_idle_vcpu(curr_on_cpu(new_cpu))); > > + ASSERT(!v->is_running && test_bit(_VPF_migrating, &v->pause_flags)= ); > > + > > + lock =3D per_cpu(schedule_data, v->processor).schedule_lock; > > + new_lock =3D per_cpu(schedule_data, new_cpu).schedule_lock; > > + > > + sched_spin_lock_double(lock, new_lock, &flags); > > + ASSERT(new_cpu !=3D v->processor); >=20 > Don't we need to do the "schedule lock dance" here as well -- > double-check to make sure that v->processor hasn't changed since the > time we grabbed the lock? >=20 This is intended to be called pretty much only from the place where it's called, i.e., during system teardown, when already is already quite quiet. So, no, I don't think we need that, but I probably could have made this _a_lot_ more clear both with comments and ASSERT()-s. Would that be ok? > > @@ -645,25 +675,72 @@ int cpu_disable_scheduler(unsigned int cpu) > > cpumask_setall(v->cpu_hard_affinity); > > } > > > > - if ( v->processor =3D=3D cpu ) > > + if ( v->processor !=3D cpu ) > > { > > - set_bit(_VPF_migrating, &v->pause_flags); > > + /* This vcpu is not on cpu, so we can move on. */ > > vcpu_schedule_unlock_irqrestore(lock, flags, v); > > - vcpu_sleep_nosync(v); > > - vcpu_migrate(v); > > + continue; > > } > > - else > > - vcpu_schedule_unlock_irqrestore(lock, flags, v); > > > > /* > > - * A vcpu active in the hypervisor will not be migratable. > > - * The caller should try again after releasing and reaquir= ing > > - * all locks. > > + * If we're here, it means that the vcpu is on cpu. Let's = see how > > + * it's best to send it away, depending on whether we are = shutting > > + * down/suspending, or doing cpupool manipulations. > > */ > > - if ( v->processor =3D=3D cpu ) > > - ret =3D -EAGAIN; > > - } > > + set_bit(_VPF_migrating, &v->pause_flags); > > + vcpu_schedule_unlock_irqrestore(lock, flags, v); > > + vcpu_sleep_nosync(v); >=20 > I don't quite understand this. By calling _nosync(), you're not > guaranteed that the vcpu has actually stopped running when you call > vcpu_move() down below; and yet inside vcpu_move(), you assert > !v->is_running. > > So either at this point, when doing suspend, the vcpu has already been > paused; in which case this is unnecessary; or it hasn't been paused, > in which case we're potentially racing the IPI / deschedule, and will > trip over the ASSERT if we "win". >=20 The former: if we're are suspending, at this stage, everything is paused already. My aim was to minimize the code being special cased basing on system_state, and I left this out because it is required for the !SYS_STATE_suspend case, and does not harm in the SYS_STATE_suspended case. However, I see how you find it confusing, and agree it could trip people over. I'll restructure the code in such a way that we go through this only in the non-suspending case (and change vcpu_move() accordingly). Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-UwVDGMCp5o+GsWU6+RdI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlWdUcsACgkQk4XaBE3IOsSEPgCdGdQHO20ASOOIbY4a71t/pd9N w8EAn2/si9c/G2B7Wcnx5FoQ9roYMd0Y =alq+ -----END PGP SIGNATURE----- --=-UwVDGMCp5o+GsWU6+RdI-- --===============6432939706872182755== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6432939706872182755==--