[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events
From: |
David Gibson |
Subject: |
Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events |
Date: |
Thu, 1 Apr 2021 12:31:27 +1100 |
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
- [PATCH 3/4] qapi/machine.json: add DEVICE_UNPLUG_ERROR QAPI event, (continued)
- [PATCH 3/4] qapi/machine.json: add DEVICE_UNPLUG_ERROR QAPI event, Daniel Henrique Barboza, 2021/03/12
- [PATCH 4/4] spapr.c: use DEVICE_UNPLUG_ERROR event in spapr_memory_unplug_rollback(), Daniel Henrique Barboza, 2021/03/12
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, David Gibson, 2021/03/22
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Daniel Henrique Barboza, 2021/03/23
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, David Gibson, 2021/03/23
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Daniel Henrique Barboza, 2021/03/24
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, David Gibson, 2021/03/24
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Igor Mammedov, 2021/03/29
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, David Gibson, 2021/03/30
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Igor Mammedov, 2021/03/31
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events,
David Gibson <=
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Daniel Henrique Barboza, 2021/03/31
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, David Gibson, 2021/03/31
- Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events, Daniel Henrique Barboza, 2021/03/31