Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events

[David Gibson]

On Wed, Mar 31, 2021 at 11:49:14AM +0200, Igor Mammedov wrote:
> On Wed, 31 Mar 2021 10:46:49 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Mar 30, 2021 at 01:28:31AM +0200, Igor Mammedov wrote:
> > > On Wed, 24 Mar 2021 16:09:59 -0300
> > > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > >   
> > > > On 3/23/21 10:40 PM, David Gibson wrote:  
> > > > > On Tue, Mar 23, 2021 at 02:10:22PM -0300, Daniel Henrique Barboza 
> > > > > wrote:    
> > > > >>
> > > > >>
> > > > >> On 3/22/21 10:12 PM, David Gibson wrote:    
> > > > >>> On Fri, Mar 12, 2021 at 05:07:36PM -0300, Daniel Henrique Barboza 
> > > > >>> wrote:    
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>> This series adds 2 new QAPI events, DEVICE_NOT_DELETED and
> > > > >>>> DEVICE_UNPLUG_ERROR. They were (and are still being) discussed in 
> > > > >>>> [1].
> > > > >>>>
> > > > >>>> Patches 1 and 3 are independent of the ppc patches and can be 
> > > > >>>> applied
> > > > >>>> separately. Patches 2 and 4 are based on David's ppc-for-6.0 
> > > > >>>> branch and
> > > > >>>> are dependent on the QAPI patches.    
> > > > >>>
> > > > >>> Implementation looks fine, but I think there's a bit more to discuss
> > > > >>> before we can apply.
> > > > >>>
> > > > >>> I think it would make sense to re-order this and put UNPLUG_ERROR
> > > > >>> first.  Its semantics are clearer, and I think there's a stronger 
> > > > >>> case
> > > > >>> for it.    
> > > > >>
> > > > >> Alright
> > > > >>    
> > > > >>>
> > > > >>> I'm a bit less sold on DEVICE_NOT_DELETED, after consideration.  
> > > > >>> Does
> > > > >>> it really tell the user/management anything useful beyond what
> > > > >>> receiving neither a DEVICE_DELETED nor a DEVICE_UNPLUG_ERROR does?  
> > > > >>>   
> > > > >>
> > > > >>
> > > > >> It informs that the hotunplug operation exceed the timeout that QEMU
> > > > >> internals considers adequate, but QEMU can't assert that it was 
> > > > >> caused
> > > > >> by an error or an unexpected delay. The end result is that the device
> > > > >> is not going to be deleted from QMP, so DEVICE_NOT_DELETED.    
> > > > > 
> > > > > Is it, though?  I mean, it is with this implementation for papr:
> > > > > because we clear the unplug_requested flag, even if the guest later
> > > > > tries to complete the unplug, it will fail.
> > > > > 
> > > > > But if I understand what Markus was saying correctly, that might not
> > > > > be possible for all hotplug systems.  I believe Markus was suggesting
> > > > > that DEVICE_NOT_DELETED could just mean that we haven't deleted the
> > > > > device yet, but it could still happen later.
> > > > > 
> > > > > And in that case, I'm not yet sold on the value of a message that
> > > > > essentially just means "Ayup, still dunno what's happening, sorry".
> > > > >     
> > > > >> Perhaps we should just be straightforward and create a 
> > > > >> DEVICE_UNPLUG_TIMEOUT
> > > > >> event.    
> > > > > 
> > > > > Hm... what if we added a "reason" field to UNPLUG_ERROR.  That could
> > > > > be "guest rejected hotplug", or something more specific, in the rare
> > > > > case that the guest has a way of signalling something more specific,
> > > > > or "timeout" - but the later *only* to be sent in cases where on the
> > > > > timeout we're able to block any later completion of the unplug (as we
> > > > > can on papr).  
> > > 
> > > Is canceling unplug on timeout documented somewhere (like some spec)?  
> > 
> > Uh.. not as such.  In the PAPR model, hotplugs and unplugs are mostly
> > guest directed, so the question doesn't really arise.
> > 
> > > If not it might (theoretically) confuse guest when it tries to unplug
> > > after timeout and leave guest in some unexpected state.  
> > 
> > Possible, but probably not that likely.  The mechanism we use to
> > "cancel" the hotplugs is that we just fail the hypercalls that the
> > guest will need to call to actually complete the hotplug.  We also
> > fail those in some other situations, and that seems to work.
> > 
> > That said, I no longer think this cancelling on timeout is a good
> > idea, since it mismatches what happens on other platforms more than I
> > think we need to.
> > 
> > My now preferred approach is to revert the timeout changes, but
> > instead allow retries of unplugs to be issued.  I think that's just a
> > matter of resending the unplug message to the guest, while making it
> > otherwise a no-op on the qemu side.
> 
> Yep, all we need to do is notify QEMU user, so it knows that unplug
> has failed. Then It can decide on it's own what to do with it and also when.

I'm not sure even that makes sense.  I mean in the cases that the
guest specifically signals failure, then yes, we should definitely
notify the user.  But for the cases the timeout was covering, I'm not
convinced a notification is useful: we *don't* know the unplug has
failed, we only suspect it, and I don't see that qemu really has any
more information than the user about what the expected time for an
unplug should be.

> > > > I believe that's already covered by the existing API:
> > > > 
> > > > 
> > > > +# @DEVICE_UNPLUG_ERROR:
> > > > +#
> > > > +# Emitted when a device hot unplug error occurs.
> > > > +#
> > > > +# @device: device name
> > > > +#
> > > > +# @msg: Informative message
> > > > 
> > > > The 'informative message' would be the reason the event occurred. In 
> > > > patch
> > > > 4/4, for the memory hotunplug refused by the guest, it is being set as:
> > > > 
> > > >       qapi_error = g_strdup_printf("Memory hotunplug rejected by the 
> > > > guest "
> > > >                                    "for device %s", dev->id);
> > > >       qapi_event_send_device_unplug_error(dev->id, qapi_error);
> > > > 
> > > > 
> > > > 
> > > > We could use the same DEVICE_UNPLUG_ERROR event in the CPU hotunplug 
> > > > timeout
> > > > case (currently on patch 2/4) by just changing 'msg', e.g.:
> > > > 
> > > > 
> > > >       qapi_error = g_strdup_printf("CPU hotunplug timeout for device 
> > > > %s", dev->id);
> > > >       qapi_event_send_device_unplug_error(dev->id, qapi_error);
> > > >   
> > > 
> > > lets make everything support ACPI (just kidding).  
> > 
> > Heh.  If nothing else, doesn't help us with existing guests.
> > 
> > > maybe we can reuse already existing ACPI_DEVICE_OST instead of 
> > > DEVICE_UNPLUG_ERROR
> > > which sort of does the same thing (and more) but instead of strings uses 
> > > status codes
> > > defined by spec.  
> > 
> > Hmm.  I'm a bit dubious about issuing ACPI messages for a non ACPI
> > guest, but maybe that could work.
> 
> May be we can rename it to be ACPI agnostic (though I'm not sure how renaming
> QAPI interfaces should be done (it might upset libvirt for example)).
> 
> (My point was that ACPI spec had already gone through all the trouble defining
> status of completion and documenting it. Also libvirt supports this 
> notification.
> It looks like worthwhile thing to consider if can somehow reuse it outside of
> x86 world)

Yeah, that's a fair point.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature