Re: [Xen-devel] [PATCH v1 3/5] xen: sched: null: deassign offline vcpus from pcpus

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel