Re: [PATCH] xen: enable per-VCPU parameter for RTDS

From: Dario Faggioli <dario.faggioli@citrix.com>
To: Chong Li <lichong659@gmail.com>, xen-devel@lists.xen.org
Cc: Chong Li <chong.li@wustl.edu>, Sisu Xi <xisisu@gmail.com>,
	george.dunlap@eu.citrix.com, Meng Xu <mengxu@cis.upenn.edu>,
	jbeulich@suse.com, dgolomb@seas.upenn.edu
Subject: Re: [PATCH] xen: enable per-VCPU parameter for RTDS
Date: Tue, 5 Apr 2016 00:05:45 +0200	[thread overview]
Message-ID: <1459807545.3166.21.camel@citrix.com> (raw)
In-Reply-To: <1459804880-1689-1-git-send-email-lichong659@gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2095 bytes --]

On Mon, 2016-04-04 at 16:21 -0500, Chong Li wrote:
> From: Chong-Li <lichong659@gmail.com>
> 
> Fix a bug in sched_credit.c and sched_credit2.c: in the default case
> of csched_dom_cntl and csched2_dom_cntl, function returns without
> unlocking prv->lock.
> 
This should mention what commit introduced the bug. Of course, this is
not a requirement for any bugfix, but in cases like this, where we know
it, it's IMO an important piece of information.

Also, I find it that the changelog itself contains a lot of information
that are already available by looking at the patch itself (e.g., files
and function names), while it should be a more high level description
of what happened/what is being fixed.

So, with a changelog like this:

  Commit f7b87b0745b4 ("enable per-VCPU parameter for RTDS") introduced
  a bug: it made it possible, in Credit and Credit2, when doing domain 
  or vcpu parameters' manipulation, to leave the hypervisor with a 
  spinlock held.

  Fix it.

This patch is:

Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

Let me just say that, I think the code would look much better if a 'int
rc = 0' variable was declared at the beginning, used like this:

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1101,6 +1101,7 @@ csched_dom_cntl(
>              sdom->cap = op->u.credit.cap;
>          break;
>      default:
> +        spin_unlock_irqrestore(&prv->lock, flags);
>          return -EINVAL;
          rc = -EINVAL;
          break;

and then returned.

But since this is functionally equivalent, I'm ok with the current
form, if that can help speeding up things, and I'll be satisfied by an
updated changelog.

Thanks for looking into this so quickly,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel