On 05.08.25 11:49, Jan Beulich wrote:
On 05.08.2025 11:33, Stewart Hildebrand wrote:On 8/5/25 05:17, Jan Beulich wrote:On 05.08.2025 11:06, Stewart Hildebrand wrote:On 8/5/25 03:44, Jan Beulich wrote:On 04.08.2025 18:57, Stewart Hildebrand wrote:On 8/4/25 03:57, Jan Beulich wrote:On 01.08.2025 22:24, Stewart Hildebrand wrote:@@ -839,6 +839,9 @@ void sched_destroy_vcpu(struct vcpu *v) { struct sched_unit *unit = v->sched_unit;+ if ( !unit )+ return; + kill_timer(&v->periodic_timer); kill_timer(&v->singleshot_timer); kill_timer(&v->poll_timer);What if it's the 2nd error path in sched_init_vcpu() that is taken?^^ This ^^ is what I'm confused aboutIf sched_init_vcpu() took the indicated path,What path? In the one I'm looking at, sched_free_unit() gets called,Oh, I see - that wasn't quite obvious, though. Yet of course ...setting v->sched_unit = NULL, and in that case ...the if() you add here won't help, and ...... the condition is true, and ...Then we might take this path (just out of context here) if ( unit->vcpu_list == v ) { rcu_read_lock(&sched_res_rculock); sched_remove_unit(vcpu_scheduler(v), unit); sched_free_udata(vcpu_scheduler(v), unit->priv); and at least Credit1's hook doesn't look to be safe against being passed NULL. (Not to speak of the risk of unit->priv being used elsewhere while cleaning up.)... this latter part of my remark still applies. IOW I continue to think that discussing the correctness of this change needs to be extended. Unless of course a scheduler maintainer wants to ack it as is.
It doesn't apply. vcpus of normal domains (i.e. not the idle domain) are created in sequential order. If the 2nd error path of sched_init_vcpu() is taken, this means we are handling the first vcpu of the unit. In this case sched_free_unit() will free the unit itself completely, so the path you are referring to can't be reached. I think only Andrew's comments need to be worked on. Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
