On 7/17/19 7:39 PM, Dario Faggioli wrote:
>>> @@ -518,6 +521,14 @@ static void null_vcpu_remove(const struct
>>> scheduler *ops, struct vcpu *v)
>>>
>>> lock = vcpu_schedule_lock_irq(v);
>>>
>>> + /* If offline, the vcpu shouldn't be assigned, nor in the
>>> waitqueue */
>>> + if ( unlikely(!is_vcpu_online(v)) )
>>> + {
>>> + ASSERT(per_cpu(npc, v->processor).vcpu != v);
>>> + ASSERT(list_empty(&nvc->waitq_elem));
>>> + goto out;
>>> + }
>>
>> * Handle the case of an offline vcpu being removed (ASSERTing that
>> it's
>> neither on a processor nor on the waitqueue)
>>
> So, IIRC (sorry, it's been a while :-D ), this is for dealing with
> remove_vcpu() being called on a vcpu which is offline. So, yes,
> basically what you said. :-)
>
> Point is the work of removing such vCPU from any CPU and from the wait
> list has been done already, in null_vcpu_sleep(), while the vCPU was
> going offline. So, here, we only need to make sure that we don't do
> anything, i.e., that we don't call _vcpu_remove().
Right; I'm mainly saying, if the commit message had said what I wrote
above, then I would have immediately been able to see what this hunk
was doing and understand why it was needed.
>> But wait, isn't this fixing a important regression in patch 2? If
>> after
>> patch 2 but before patch 3, a VM is created with offline vcpus, and
>> then
>> destroyed, won't the offline vcpus reach here neither on the waitlist
>> nor on a vcpu?
>>
> I'm not sure I understand the point you're trying to make here, sorry.
>
> In general, considering what we've said in other mails, if you think
> that patch 2 and 3 should be merged, we can do that.
>
> My reasoning, when putting together the series, was the one I already
> stated: this is broken already, so no big deal breaking it "more", and
> I continue to see it that way.
>
> But I appreciate you seeing it differently, while I don't have a too
> strong opinion, so I'd be fine merging the patches (or doing other
> series rearrangements, if you feel strongly that they're necessary).
>
> Or is it something completely different that you meant?
Merging the patches would be one way to avoid the regression, yes.
Sorry to be picky, but I've recently spent a lot of time doing
archaeology, and wishing people in the distant past had been more
careful / informative in their commit hygiene. A little bit of effort
now may save someone in the future -- possibly you or me -- hours of
frustration. :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
