| Message ID | 1406904759-3833-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> On 01.08.14 at 16:52, <julien.grall@linaro.org> wrote: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) > } > /* Filter out non-online cpus */ > cpumask_and(dom_cpumask, dom_cpumask, online); > - ASSERT(!cpumask_empty(dom_cpumask)); > + ASSERT( !d->vcpu || !d->vcpu[0] || !cpumask_empty(dom_cpumask)); > /* And compute the intersection between hard, online and soft */ > cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask); > Actually, with sched_move_domain() having /* Do we have vcpus already? If not, no need to update node-affinity */ if ( d->vcpu ) domain_update_node_affinity(d); it should really just be _that_ if() condition to get extended, and the ASSERT() left alone altogether. Or, if any other path can be proven to possibly reach the function with no vCPU allocated (I just went through them and didn't spot any), then it should really be an early bail from the function rather than a pointlessly complicated ASSERT() expression. (And for the record, your expression has a coding style violation anyway in that it begins with a space.) Jan
On 08/01/2014 04:12 PM, Jan Beulich wrote: >>>> On 01.08.14 at 16:52, <julien.grall@linaro.org> wrote: >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) >> } >> /* Filter out non-online cpus */ >> cpumask_and(dom_cpumask, dom_cpumask, online); >> - ASSERT(!cpumask_empty(dom_cpumask)); >> + ASSERT( !d->vcpu || !d->vcpu[0] || !cpumask_empty(dom_cpumask)); >> /* And compute the intersection between hard, online and soft */ >> cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask); >> > > Actually, with sched_move_domain() having > > /* Do we have vcpus already? If not, no need to update node-affinity */ > if ( d->vcpu ) > domain_update_node_affinity(d); > > it should really just be _that_ if() condition to get extended, and the > ASSERT() left alone altogether. Or, if any other path can be proven > to possibly reach the function with no vCPU allocated (I just went > through them and didn't spot any), then it should really be an early > bail from the function rather than a pointlessly complicated ASSERT() > expression. (And for the record, your expression has a coding style > violation anyway in that it begins with a space.) I think changing the if() was what Julien started with; but overall I think that it makes more sense to update the assumption of the code in question than to require all the callers to be careful not to trip over it. Doing an early bail might make sense as well. -George
On 08/04/2014 03:50 PM, George Dunlap wrote: > On 08/01/2014 04:12 PM, Jan Beulich wrote: >>>>> On 01.08.14 at 16:52, <julien.grall@linaro.org> wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) >>> } >>> /* Filter out non-online cpus */ >>> cpumask_and(dom_cpumask, dom_cpumask, online); >>> - ASSERT(!cpumask_empty(dom_cpumask)); >>> + ASSERT( !d->vcpu || !d->vcpu[0] || >>> !cpumask_empty(dom_cpumask)); >>> /* And compute the intersection between hard, online and >>> soft */ >>> cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask); >>> >> >> Actually, with sched_move_domain() having >> >> /* Do we have vcpus already? If not, no need to update >> node-affinity */ >> if ( d->vcpu ) >> domain_update_node_affinity(d); >> >> it should really just be _that_ if() condition to get extended, and the >> ASSERT() left alone altogether. Or, if any other path can be proven >> to possibly reach the function with no vCPU allocated (I just went >> through them and didn't spot any), then it should really be an early >> bail from the function rather than a pointlessly complicated ASSERT() >> expression. (And for the record, your expression has a coding style >> violation anyway in that it begins with a space.) > > I think changing the if() was what Julien started with; but overall I > think that it makes more sense to update the assumption of the code in > question than to require all the callers to be careful not to trip over it. > > Doing an early bail might make sense as well. Ok. So which one should I choose? The early bail out? Regards,
>>> On 07.08.14 at 17:04, <julien.grall@linaro.org> wrote: > On 08/04/2014 03:50 PM, George Dunlap wrote: >> On 08/01/2014 04:12 PM, Jan Beulich wrote: >>>>>> On 01.08.14 at 16:52, <julien.grall@linaro.org> wrote: >>>> --- a/xen/common/domain.c >>>> +++ b/xen/common/domain.c >>>> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) >>>> } >>>> /* Filter out non-online cpus */ >>>> cpumask_and(dom_cpumask, dom_cpumask, online); >>>> - ASSERT(!cpumask_empty(dom_cpumask)); >>>> + ASSERT( !d->vcpu || !d->vcpu[0] || >>>> !cpumask_empty(dom_cpumask)); >>>> /* And compute the intersection between hard, online and >>>> soft */ >>>> cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask); >>>> >>> >>> Actually, with sched_move_domain() having >>> >>> /* Do we have vcpus already? If not, no need to update >>> node-affinity */ >>> if ( d->vcpu ) >>> domain_update_node_affinity(d); >>> >>> it should really just be _that_ if() condition to get extended, and the >>> ASSERT() left alone altogether. Or, if any other path can be proven >>> to possibly reach the function with no vCPU allocated (I just went >>> through them and didn't spot any), then it should really be an early >>> bail from the function rather than a pointlessly complicated ASSERT() >>> expression. (And for the record, your expression has a coding style >>> violation anyway in that it begins with a space.) >> >> I think changing the if() was what Julien started with; but overall I >> think that it makes more sense to update the assumption of the code in >> question than to require all the callers to be careful not to trip over it. >> >> Doing an early bail might make sense as well. > > Ok. So which one should I choose? The early bail out? I would indeed favor that over the altered assertion. Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index d7a84cf..188b769 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) } /* Filter out non-online cpus */ cpumask_and(dom_cpumask, dom_cpumask, online); - ASSERT(!cpumask_empty(dom_cpumask)); + ASSERT( !d->vcpu || !d->vcpu[0] || !cpumask_empty(dom_cpumask)); /* And compute the intersection between hard, online and soft */ cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask);
The commit bac6334b5 "move domain to cpupool0 before destroying it" make Xen crashes when a domain is destroyed with d->vcpus allocated but no VCPU initialized. Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452 Xen call trace: [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC) [<00000004>] 00000004 (LR) [<00226870>] sched_move_domain+0x3cc/0x42c [<0020925c>] domain_kill+0xc8/0x178 [<00206a0c>] do_domctl+0xaac/0x15e4 [<002529c0>] do_trap_hypervisor+0xc5c/0xf94 [<002559f0>] return_from_trap+0/0x4 Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized. Signed-off-by: Julien Grall <julien.grall@linaro.org> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: George Dunlap <george.dunlap@citrix.com> Cc: Juergen Gross <jgross@suse.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> Cc: Tim Deegan <tim@xen.org> --- This patch should be backported to Xen 4.4 Changes in v2: - Add specify the offended commit ID in the message --- xen/common/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)