| Message ID | 1459809913-1958-1-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }