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 about
>>
>> If 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.
Jan
