From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 2/2] xen: credit1: avoid boosting vCPUs being "just" migrated Date: Wed, 24 Feb 2016 11:14:03 +0000 Message-ID: <56CD907B.2060805@citrix.com> References: <20160212162338.25796.17618.stgit@Solace.station> <20160212162926.25796.58870.stgit@Solace.station> <56CD8947.4050307@citrix.com> <1456312334.17312.14.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 1aYXOS-0001MC-6u for xen-devel@lists.xenproject.org; Wed, 24 Feb 2016 11:14:08 +0000 In-Reply-To: <1456312334.17312.14.camel@citrix.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: Dario Faggioli , xen-devel@lists.xenproject.org Cc: George Dunlap , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 24/02/16 11:12, Dario Faggioli wrote: > On Wed, 2016-02-24 at 10:43 +0000, George Dunlap wrote: >> On 12/02/16 16:29, Dario Faggioli wrote: >>> >>> @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler >>> *ops, struct vcpu *vc) >>> * more CPU resource intensive VCPUs without impacting >>> overall >>> * system fairness. >>> * >>> - * The one exception is for VCPUs of capped domains unpausing >>> - * after earning credits they had overspent. We don't boost >>> - * those. >>> + * There are two cases, when we don't want to boost: >>> + * - VCPUs that are waking up after a migration, rather than >>> + * after having block; >>> + * - VCPUs of capped domains unpausing after earning credits >>> + * they had overspent. >>> + * >>> + * Note that checking whether we are "only" migrating must be >>> + * done up front, as we do not want the clearing of the bit we >>> + * set in csched_cpu_pick() to be short-circuited away. >>> */ >>> - if ( svc->pri == CSCHED_PRI_TS_UNDER && >>> + if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc- >>>> flags) && >>> + svc->pri == CSCHED_PRI_TS_UNDER && >>> !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) >> >> Sorry to be late reviewing this. >> > No problem. Thanks for getting to it, actually, as I've got a few more > patches stacked on top of these outstanding series. > >> So we always want to clear the 'migrating' flag, regardless of >> whether >> we do anything with boosting. Would that logic be clearer if we >> cleared >> it as a separate step, storing the result in a local variable? E.g.: >> >> bool migrating; >> >> ... >> >> /* Always clear migrating flag if it's set */ >> migrating = test_and_clear_bit(...) >> >> if ( !migrating && ...) { >> } >> >> Then we wouldn't need the last paragraph in the comment. >> > Yes, I think I like this better. > >> That said, this is v3, so if you'd rather just get this in as it is, >> then you can have my Acked-by as well. >> > No, I'll resend... If I make (only) this change, can I resend directly > with your Acked-by? Well I won't know what it looks like until I see it, will I? :-) I prioritize recently-reviewed series, so if you resend I should be able to give an Acked-by the same day. -George