From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch() Date: Thu, 15 Oct 2015 10:25:47 +0200 Message-ID: <561F630B.5080401@suse.com> References: <20151014151805.28642.63981.stgit@Solace.station> <20151014155416.28642.76465.stgit@Solace.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZmdrD-00078C-U0 for xen-devel@lists.xenproject.org; Thu, 15 Oct 2015 08:25:52 +0000 In-Reply-To: <20151014155416.28642.76465.stgit@Solace.station> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli , xen-devel@lists.xenproject.org Cc: George Dunlap , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 10/14/2015 05:54 PM, Dario Faggioli wrote: > schedule_cpu_switch() is meant to be only used for moving > pCPUs from a cpupool to no cpupool, and from there back > to a cpupool, *not* to move them directly from one cpupool > to another. > > This is something that is reflected in the way it is > implemented, and should be kept in mind when looking at > it. However, that is not that clear, by just the look of > it. > > Make it more evident by adding commentary and ASSERT()s. > > Signed-off-by: Dario Faggioli > --- > Cc: George Dunlap > Cc: Juergen Gross > Cc: Jan Beulich > --- > Changes from v1: > * new patch, was not there in v1. > --- > xen/common/schedule.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 9aa209d..5ebfa33 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -1486,12 +1486,40 @@ void __init scheduler_init(void) > BUG(); > } > > +/* > + * Move a pCPU outside of the influence of the scheduler of its current > + * cpupool, or subject it to the scheduler of a new cpupool. > + * > + * For the pCPUs that are removed from their cpupool, their scheduler becomes > + * &ops (the default scheduler, selected at boot, which also services the > + * default cpupool). However, as these pCPUs are not really part of any pool, > + * there won't be any scheduling event on them, not even from the default > + * scheduler. Basically, they will just sit idle until they are explicitly > + * added back to a cpupool. > + */ > int schedule_cpu_switch(unsigned int cpu, struct cpupool *c) > { > struct vcpu *idle; > void *ppriv, *ppriv_old, *vpriv, *vpriv_old; > struct scheduler *old_ops = per_cpu(scheduler, cpu); > struct scheduler *new_ops = (c == NULL) ? &ops : c->sched; > + struct cpupool *pool = per_cpu(cpupool, cpu); > + > + /* > + * pCPUs only move from a valid cpupool to free (i.e., out of any pool), > + * or from free to a valid cpupool. In the former case (which happens when > + * c is NULL), we want the CPU to have been marked as free already, as > + * well as to not be valid for the source pool any longer, when we get to > + * here. In the latter case (which happens when c is a valid cpupool), we > + * want the CPU to still be marked as free, as well as to not yet be valid > + * for the destination pool. > + * This all is because we do not want any scheduling activity on the CPU > + * while, in here, we switch things over. While this is correct I think you should mention that in the "adding to a pool" case per_cpu(cpupool, cpu) already contains the new pool pointer. So c == pool then and both are not NULL. In fact pool is never NULL, c might be. Maybe it would be a good idea to move setting of per_cpu(cpupool, cpu) into schedule_cpu_switch(). Originally I didn't do that to avoid spreading too much cpupool related actions outside of cpupool.c. But with those ASSERT()s added hiding that action will cause more confusion than having the modification of per_cpu(cpupool, cpu) here. When doing the code movement the current behaviour regarding sequence of changes must be kept, of course. So when adding the cpu to a pool the cpupool information must be set _before_ taking the scheduler lock, while when removing this must happen after release of the lock. > + */ > + ASSERT(c != NULL || pool != NULL); > + ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus)); > + ASSERT((c == NULL && !cpumask_test_cpu(cpu, pool->cpu_valid)) || > + (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid))); > > if ( old_ops == new_ops ) > return 0; Juergen