diff mbox

xen: enable per-VCPU parameter for RTDS

Message ID 1459804880-1689-1-git-send-email-lichong659@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chong Li April 4, 2016, 9:21 p.m. UTC 
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.

Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>

---
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <lichong659@gmail.com>
---
 xen/common/sched_credit.c  | 1 +
 xen/common/sched_credit2.c | 1 +
 2 files changed, 2 insertions(+)

Comments

Dario Faggioli April 4, 2016, 10:05 p.m. UTC | #1 
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
diff mbox

Patch

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index e5d15d8..fa6b7f0 100644
--- 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;
     }
 
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index d48ed5a..cf444c9 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1457,6 +1457,7 @@  csched2_dom_cntl(
         }
         break;
     default:
+        spin_unlock_irqrestore(&prv->lock, flags);
         return -EINVAL;
     }