xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen: enable per-VCPU parameter for RTDS
@ 2016-04-05  1:07 Chong Li
  2016-04-05  1:44 ` Meng Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Chong Li @ 2016-04-05  1:07 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, george.dunlap, dario.faggioli, mengxu, jbeulich,
	lichong659, dgolomb

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 and interrupts disabled.

Fix it.

Signed-off-by: Chong Li <chong.li@wustl.edu>

Acked-by: Dario Faggioli <dario.faggioli@citrix.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  | 6 ++++--
 xen/common/sched_credit2.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index e5d15d8..4c4927f 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1075,6 +1075,7 @@ csched_dom_cntl(
     struct csched_dom * const sdom = CSCHED_DOM(d);
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
+    int rc = 0;
 
     /* Protect both get and put branches with the pluggable scheduler
      * lock. Runq lock not needed anywhere in here. */
@@ -1101,12 +1102,13 @@ csched_dom_cntl(
             sdom->cap = op->u.credit.cap;
         break;
     default:
-        return -EINVAL;
+        rc = -EINVAL;
+        break;
     }
 
     spin_unlock_irqrestore(&prv->lock, flags);
 
-    return 0;
+    return rc;
 }
 
 static inline void
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index d48ed5a..b8c8e40 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1416,6 +1416,7 @@ csched2_dom_cntl(
     struct csched2_dom * const sdom = CSCHED2_DOM(d);
     struct csched2_private *prv = CSCHED2_PRIV(ops);
     unsigned long flags;
+    int rc = 0;
 
     /* Must hold csched2_priv lock to read and update sdom,
      * runq lock to update csvcs. */
@@ -1457,12 +1458,13 @@ csched2_dom_cntl(
         }
         break;
     default:
-        return -EINVAL;
+        rc = -EINVAL;
+        break;
     }
 
     spin_unlock_irqrestore(&prv->lock, flags);
 
-    return 0;
+    return rc;
 }
 
 static void *
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  1:07 [PATCH] xen: enable per-VCPU parameter for RTDS Chong Li
@ 2016-04-05  1:44 ` Meng Xu
  2016-04-05  7:57 ` Jan Beulich
  2016-04-05 12:45 ` Wei Liu
  2 siblings, 0 replies; 12+ messages in thread
From: Meng Xu @ 2016-04-05  1:44 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, George Dunlap, Dario Faggioli, xen-devel, Jan Beulich,
	Dagaen Golomb

On Mon, Apr 4, 2016 at 9:07 PM, Chong Li <lichong659@gmail.com> wrote:
> 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 and interrupts disabled.
>
> Fix it.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
>
> Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

I'm wondering if the title "xen: enable per-VCPU parameter for RTDS"
is suitable for this patch, although I don't have a better title.

The title in my mind is: xen: fix incorrect lock for credit and credit2
I won't fight for this title, though. :-)

Probably no need to resend...

Thanks,

Meng

-- 
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  1:07 [PATCH] xen: enable per-VCPU parameter for RTDS Chong Li
  2016-04-05  1:44 ` Meng Xu
@ 2016-04-05  7:57 ` Jan Beulich
  2016-04-05  9:25   ` George Dunlap
  2016-04-05 12:45 ` Wei Liu
  2 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2016-04-05  7:57 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, george.dunlap, dario.faggioli, xen-devel, mengxu, dgolomb

>>> On 05.04.16 at 03:07, <lichong659@gmail.com> wrote:
> 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 and interrupts disabled.
> 
> Fix it.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> 
> Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

This appears to be the 3rd variant of the patch, all with the same
bogus subject, and no versioning information at all. Please
remember that mails can arrive out of order, so there's no way
to reliably tell which one got sent first. So for this to go in I'm
expecting you to re-send with a proper subject and an indication
that this is not the initial version of the patch.

Jan


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  7:57 ` Jan Beulich
@ 2016-04-05  9:25   ` George Dunlap
  2016-04-05  9:43     ` Dario Faggioli
  0 siblings, 1 reply; 12+ messages in thread
From: George Dunlap @ 2016-04-05  9:25 UTC (permalink / raw)
  To: Jan Beulich, Chong Li
  Cc: Chong Li, george.dunlap, dario.faggioli, xen-devel, mengxu, dgolomb

On 05/04/16 08:57, Jan Beulich wrote:
>>>> On 05.04.16 at 03:07, <lichong659@gmail.com> wrote:
>> 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 and interrupts disabled.
>>
>> Fix it.
>>
>> Signed-off-by: Chong Li <chong.li@wustl.edu>
>>
>> Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> This appears to be the 3rd variant of the patch, all with the same
> bogus subject, and no versioning information at all. Please
> remember that mails can arrive out of order, so there's no way
> to reliably tell which one got sent first. So for this to go in I'm
> expecting you to re-send with a proper subject and an indication
> that this is not the initial version of the patch.

And since this version of the patch is functionally different than the
one Dario acked, you should drop his Acked-by as well.

Thanks for being responsive in fixing this bug; and sorry for all the
criticism, but OSS development is very detail-oriented, and it takes a
while to internalize all the rules for how things should be done.

 -George

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  9:25   ` George Dunlap
@ 2016-04-05  9:43     ` Dario Faggioli
  0 siblings, 0 replies; 12+ messages in thread
From: Dario Faggioli @ 2016-04-05  9:43 UTC (permalink / raw)
  To: George Dunlap, Jan Beulich, Chong Li
  Cc: george.dunlap, Chong Li, mengxu, dgolomb, xen-devel


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

On Tue, 2016-04-05 at 10:25 +0100, George Dunlap wrote:
> On 05/04/16 08:57, Jan Beulich wrote:
> > 
> > > Signed-off-by: Chong Li <chong.li@wustl.edu>
> > > 
> > > Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
> > This appears to be the 3rd variant of the patch, all with the same
> > bogus subject, and no versioning information at all. Please
> > remember that mails can arrive out of order, so there's no way
> > to reliably tell which one got sent first. So for this to go in I'm
> > expecting you to re-send with a proper subject and an indication
> > that this is not the initial version of the patch.
> And since this version of the patch is functionally different than
> the
> one Dario acked, you should drop his Acked-by as well.
> 
Indeed you should have.

But, with the subject fixed (something like "xen: sched: fix deadlock
when changing scheduling parameters") and properly versioned, you can
add it again:

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

> Thanks for being responsive in fixing this bug; and sorry for all the
> criticism, but OSS development is very detail-oriented, and it takes
> a
> while to internalize all the rules for how things should be done.
> 
It does! :-)

Thanks and Regards,
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  1:07 [PATCH] xen: enable per-VCPU parameter for RTDS Chong Li
  2016-04-05  1:44 ` Meng Xu
  2016-04-05  7:57 ` Jan Beulich
@ 2016-04-05 12:45 ` Wei Liu
  2 siblings, 0 replies; 12+ messages in thread
From: Wei Liu @ 2016-04-05 12:45 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, Wei Liu, george.dunlap, dario.faggioli, xen-devel,
	mengxu, jbeulich, dgolomb

On Mon, Apr 04, 2016 at 08:07:13PM -0500, Chong Li wrote:
> 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 and interrupts disabled.
> 
> Fix it.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> 
> Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

Chong, FYI Jan has committed this patch with a proper title. There is no
need to send another version.

And for your future contributions please follow the guidelines several
maintainers have given you.

Wei.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-05  9:18   ` George Dunlap
@ 2016-04-05  9:39     ` Dario Faggioli
  0 siblings, 0 replies; 12+ messages in thread
From: Dario Faggioli @ 2016-04-05  9:39 UTC (permalink / raw)
  To: George Dunlap, Andrew Cooper, Chong Li, xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, Meng Xu, jbeulich, dgolomb


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

On Tue, 2016-04-05 at 10:18 +0100, George Dunlap wrote:
> On 05/04/16 00:14, Andrew Cooper wrote:
> > On 04/04/2016 23:45, Chong Li wrote:
> > > ---
> > >  xen/common/sched_credit.c  | 1 +
> > >  xen/common/sched_credit2.c | 1 +
> > >  2 files changed, 2 insertions(+)
> > > 
> > > 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;
> > >      }
> > >  
> > While Dario didn't care too much how you fixed the issue, I do.
> > 
> > Please use an "int rc = 0" and remove remove the return statement
> > (instead, assigning rc = -EINVAL; and a break;).  It makes far more
> > readable and understandable code, which is better in the long run.
>
> Well Dario's a scheduler maintainer and you're not.
> 
Eheh :-)

> But in any case, I also prefer the rc / break (or goto out) pattern
> enough to ask for a re-send.  (But I now see that a v3 has already
> been
> sent.)
> 
Yeah, I said I was ok in the event that we wanted to check in the fix
ASAP, editing the changelog during commit. If a resend were to be done,
I'd have hoped the structure to be changed as said above... I probably
should have said it more explicitly.

Impressive that we're all on the same page (at least, from a technical
point of view! :-P)

Thanks and Regards,
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-04 23:14 ` Andrew Cooper
@ 2016-04-05  9:18   ` George Dunlap
  2016-04-05  9:39     ` Dario Faggioli
  0 siblings, 1 reply; 12+ messages in thread
From: George Dunlap @ 2016-04-05  9:18 UTC (permalink / raw)
  To: Andrew Cooper, Chong Li, xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
	jbeulich, dgolomb

On 05/04/16 00:14, Andrew Cooper wrote:
> On 04/04/2016 23:45, Chong Li wrote:
>> From: Chong-Li <lichong659@gmail.com>
>>
>> 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.
> 
> And interrupts disabled (which is far more of a problem than just the
> spinlock).
> 
>>
>> Fix it.
>>
>> 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>
> 
> This patch is not SoB by anyone other than you.
> 
>>
>> Acked-by: Dario Faggioli <dario.faggioli@citrix.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(+)
>>
>> 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;
>>      }
>>  
> 
> While Dario didn't care too much how you fixed the issue, I do.
> 
> Please use an "int rc = 0" and remove remove the return statement
> (instead, assigning rc = -EINVAL; and a break;).  It makes far more
> readable and understandable code, which is better in the long run.

Well Dario's a scheduler maintainer and you're not.

But in any case, I also prefer the rc / break (or goto out) pattern
enough to ask for a re-send.  (But I now see that a v3 has already been
sent.)

 -George

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-04 22:45 Chong Li
@ 2016-04-04 23:14 ` Andrew Cooper
  2016-04-05  9:18   ` George Dunlap
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2016-04-04 23:14 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
	jbeulich, dgolomb

On 04/04/2016 23:45, Chong Li wrote:
> From: Chong-Li <lichong659@gmail.com>
>
> 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.

And interrupts disabled (which is far more of a problem than just the
spinlock).

>
> Fix it.
>
> 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>

This patch is not SoB by anyone other than you.

>
> Acked-by: Dario Faggioli <dario.faggioli@citrix.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(+)
>
> 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;
>      }
>  

While Dario didn't care too much how you fixed the issue, I do.

Please use an "int rc = 0" and remove remove the return statement
(instead, assigning rc = -EINVAL; and a break;).  It makes far more
readable and understandable code, which is better in the long run.

~Andrew

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] xen: enable per-VCPU parameter for RTDS
@ 2016-04-04 22:45 Chong Li
  2016-04-04 23:14 ` Andrew Cooper
  0 siblings, 1 reply; 12+ messages in thread
From: Chong Li @ 2016-04-04 22:45 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
	jbeulich, Chong-Li, dgolomb

From: Chong-Li <lichong659@gmail.com>

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.

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>

Acked-by: Dario Faggioli <dario.faggioli@citrix.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(+)

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;
     }
 
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xen: enable per-VCPU parameter for RTDS
  2016-04-04 21:21 Chong Li
@ 2016-04-04 22:05 ` Dario Faggioli
  0 siblings, 0 replies; 12+ messages in thread
From: Dario Faggioli @ 2016-04-04 22:05 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, Meng Xu, jbeulich, dgolomb


[-- 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] xen: enable per-VCPU parameter for RTDS
@ 2016-04-04 21:21 Chong Li
  2016-04-04 22:05 ` Dario Faggioli
  0 siblings, 1 reply; 12+ messages in thread
From: Chong Li @ 2016-04-04 21:21 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
	jbeulich, Chong-Li, dgolomb

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(+)

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;
     }
 
-- 
1.9.1


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-04-05 12:45 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-05  1:07 [PATCH] xen: enable per-VCPU parameter for RTDS Chong Li
2016-04-05  1:44 ` Meng Xu
2016-04-05  7:57 ` Jan Beulich
2016-04-05  9:25   ` George Dunlap
2016-04-05  9:43     ` Dario Faggioli
2016-04-05 12:45 ` Wei Liu
  -- strict thread matches above, loose matches on Subject: below --
2016-04-04 22:45 Chong Li
2016-04-04 23:14 ` Andrew Cooper
2016-04-05  9:18   ` George Dunlap
2016-04-05  9:39     ` Dario Faggioli
2016-04-04 21:21 Chong Li
2016-04-04 22:05 ` Dario Faggioli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).