diff mbox series

[v1,2/2] libxl: add removing XS backend path for PV devices on domain destroy

Message ID 20191008141024.10885-3-al1img@gmail.com (mailing list archive)
State Superseded
Headers show
Series Remove backend xen store entry on domain destroy | expand

Commit Message

Oleksandr Grytsov Oct. 8, 2019, 2:10 p.m. UTC 
From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Currently backend path is remove for specific devices: VBD, VIF, QDISK.
This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_device.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Ian Jackson Oct. 11, 2019, 3:07 p.m. UTC | #1 
Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Thanks for this and your previous patch.

This one looks to me like it is probably correct.  But I have not been
able to understand why this code was limited to vbds and vifs before
so I am hesitant.

Searching the git history, I think this has been like this since
a0eaa86e7537
 "libxl: add device backend listener in order to launch backends"
and subsequent commits have just reorganised things but not
fundamentally changed them.  I've CC'd Roger who wrote this code.

I think:

>      switch(ddev->dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VDISPL:
> +    case LIBXL__DEVICE_KIND_VSND:
> +    case LIBXL__DEVICE_KIND_VINPUT:
>      case LIBXL__DEVICE_KIND_VBD:
>      case LIBXL__DEVICE_KIND_VIF:

If we do want this to handle *all* kinds of device, maybe it should
have a fallback that aborts, or something ?  (I don't think it is
easy to make it a compile error to forget to add an entry here but if
we could do that it would probably be best.)

All of that assuming that the basic idea is right, which I would like
Roger's opinion about.

Thanks,
Ian.
Roger Pau Monné Oct. 11, 2019, 3:32 p.m. UTC | #2 
On Fri, Oct 11, 2019 at 04:07:11PM +0100, Ian Jackson wrote:
> Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > 
> > Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> > This commit adds removing backend path for: VDISPL, VSND, VINPUT.
> 
> Thanks for this and your previous patch.
> 
> This one looks to me like it is probably correct.  But I have not been
> able to understand why this code was limited to vbds and vifs before
> so I am hesitant.
> 
> Searching the git history, I think this has been like this since
> a0eaa86e7537
>  "libxl: add device backend listener in order to launch backends"
> and subsequent commits have just reorganised things but not
> fundamentally changed them.  I've CC'd Roger who wrote this code.

When this code was added (devd) those where the only backends handled
by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
the issue is that when support for those was added, it wasn't properly
wired into devd.

> I think:
> 
> >      switch(ddev->dev->backend_kind) {
> > +    case LIBXL__DEVICE_KIND_VDISPL:
> > +    case LIBXL__DEVICE_KIND_VSND:
> > +    case LIBXL__DEVICE_KIND_VINPUT:
> >      case LIBXL__DEVICE_KIND_VBD:
> >      case LIBXL__DEVICE_KIND_VIF:
> 
> If we do want this to handle *all* kinds of device, maybe it should
> have a fallback that aborts, or something ?  (I don't think it is
> easy to make it a compile error to forget to add an entry here but if
> we could do that it would probably be best.)

Maybe we could have some generic handling for everything != qdisk?

IIRC qdisk needs special handling so that devd can launch and destroy
a QEMU instance when required, but other backends that run in the
kernel don't need such handling and could maybe share the same generic
code path.

> All of that assuming that the basic idea is right, which I would like
> Roger's opinion about.

Looking at the patch itself, you also seem to be doing some changes
related to num_vbds and num_vifs, but those are not mentioned in the
commit message, is that a stray change?

I'm not saying it's wrong, it's just that it feels like it belongs in
a different patch maybe.

Thanks, Roger.
Ian Jackson Oct. 11, 2019, 3:55 p.m. UTC | #3 
Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> When this code was added (devd) those where the only backends handled
> by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> the issue is that when support for those was added, it wasn't properly
> wired into devd.
> 
> > I think:
> > 
> > >      switch(ddev->dev->backend_kind) {
> > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > +    case LIBXL__DEVICE_KIND_VSND:
> > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > >      case LIBXL__DEVICE_KIND_VBD:
> > >      case LIBXL__DEVICE_KIND_VIF:
> > 
> > If we do want this to handle *all* kinds of device, maybe it should
> > have a fallback that aborts, or something ?  (I don't think it is
> > easy to make it a compile error to forget to add an entry here but if
> > we could do that it would probably be best.)
> 
> Maybe we could have some generic handling for everything != qdisk?

So make that the "default:" ?  That would be fine by me.

> IIRC qdisk needs special handling so that devd can launch and destroy
> a QEMU instance when required, but other backends that run in the
> kernel don't need such handling and could maybe share the same generic
> code path.

Right.

> > All of that assuming that the basic idea is right, which I would like
> > Roger's opinion about.
> 
> Looking at the patch itself, you also seem to be doing some changes
> related to num_vbds and num_vifs, but those are not mentioned in the
> commit message, is that a stray change?

No, I don't think so.  Those variables just tell us when the thing is
torn down but Oleksandr's code is able to use the devices slist itself
for that.  Please do check to see if you agree.

Thanks,
Ian.
Roger Pau Monné Oct. 15, 2019, 3:39 p.m. UTC | #4 
On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > When this code was added (devd) those where the only backends handled
> > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > the issue is that when support for those was added, it wasn't properly
> > wired into devd.
> > 
> > > I think:
> > > 
> > > >      switch(ddev->dev->backend_kind) {
> > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > >      case LIBXL__DEVICE_KIND_VBD:
> > > >      case LIBXL__DEVICE_KIND_VIF:
> > > 
> > > If we do want this to handle *all* kinds of device, maybe it should
> > > have a fallback that aborts, or something ?  (I don't think it is
> > > easy to make it a compile error to forget to add an entry here but if
> > > we could do that it would probably be best.)
> > 
> > Maybe we could have some generic handling for everything != qdisk?
> 
> So make that the "default:" ?  That would be fine by me.

If possible yes, that would be my preference, and would prevent having
to add new device types to this switch unless special handling is
required.

> 
> > IIRC qdisk needs special handling so that devd can launch and destroy
> > a QEMU instance when required, but other backends that run in the
> > kernel don't need such handling and could maybe share the same generic
> > code path.
> 
> Right.
> 
> > > All of that assuming that the basic idea is right, which I would like
> > > Roger's opinion about.
> > 
> > Looking at the patch itself, you also seem to be doing some changes
> > related to num_vbds and num_vifs, but those are not mentioned in the
> > commit message, is that a stray change?
> 
> No, I don't think so.  Those variables just tell us when the thing is
> torn down but Oleksandr's code is able to use the devices slist itself
> for that.  Please do check to see if you agree.

Yes, that's fine. I don't think those changes are wrong, I just think
that at least they should be mentioned in the commit message. Ie:
"while there remove the num_vifs and num_vbds since they are not
needed and the same can be achieved by checking that the device list
is empty." or some such.

Thanks, Roger.
Oleksandr Grytsov Oct. 16, 2019, 10:39 a.m. UTC | #5 
On Tue, Oct 15, 2019 at 6:39 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> > Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > > When this code was added (devd) those where the only backends handled
> > > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > > the issue is that when support for those was added, it wasn't properly
> > > wired into devd.
> > >
> > > > I think:
> > > >
> > > > >      switch(ddev->dev->backend_kind) {
> > > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > > >      case LIBXL__DEVICE_KIND_VBD:
> > > > >      case LIBXL__DEVICE_KIND_VIF:
> > > >
> > > > If we do want this to handle *all* kinds of device, maybe it should
> > > > have a fallback that aborts, or something ?  (I don't think it is
> > > > easy to make it a compile error to forget to add an entry here but if
> > > > we could do that it would probably be best.)
> > >
> > > Maybe we could have some generic handling for everything != qdisk?
> >
> > So make that the "default:" ?  That would be fine by me.
>
> If possible yes, that would be my preference, and would prevent having
> to add new device types to this switch unless special handling is
> required.
>
> >
> > > IIRC qdisk needs special handling so that devd can launch and destroy
> > > a QEMU instance when required, but other backends that run in the
> > > kernel don't need such handling and could maybe share the same generic
> > > code path.
> >
> > Right.
> >
> > > > All of that assuming that the basic idea is right, which I would like
> > > > Roger's opinion about.
> > >
> > > Looking at the patch itself, you also seem to be doing some changes
> > > related to num_vbds and num_vifs, but those are not mentioned in the
> > > commit message, is that a stray change?
> >
> > No, I don't think so.  Those variables just tell us when the thing is
> > torn down but Oleksandr's code is able to use the devices slist itself
> > for that.  Please do check to see if you agree.
>
> Yes, that's fine. I don't think those changes are wrong, I just think
> that at least they should be mentioned in the commit message. Ie:
> "while there remove the num_vifs and num_vbds since they are not
> needed and the same can be achieved by checking that the device list
> is empty." or some such.
>
> Thanks, Roger.

Ian, Roger,

Thanks for reviewing and comments. I will update the patch with your
comments above.
diff mbox series

Patch

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 1402b61a81..8ce70661e5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1477,7 +1477,7 @@  typedef struct libxl__ddomain_device {
  */
 typedef struct libxl__ddomain_guest {
     uint32_t domid;
-    int num_vifs, num_vbds, num_qdisks;
+    int num_qdisks;
     LIBXL_SLIST_HEAD(, struct libxl__ddomain_device) devices;
     LIBXL_SLIST_ENTRY(struct libxl__ddomain_guest) next;
 } libxl__ddomain_guest;
@@ -1530,8 +1530,7 @@  static void check_and_maybe_remove_guest(libxl__gc *gc,
 {
     assert(ddomain);
 
-    if (dguest != NULL &&
-        dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks == 0) {
+    if (dguest != NULL && LIBXL_SLIST_FIRST(&dguest->devices) == NULL) {
         LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
                            next);
         LOGD(DEBUG, dguest->domid, "Removed domain from the list of active guests");
@@ -1573,9 +1572,6 @@  static int add_device(libxl__egc *egc, libxl__ao *ao,
     switch(dev->backend_kind) {
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds++;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs++;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
@@ -1619,11 +1615,11 @@  static int remove_device(libxl__egc *egc, libxl__ao *ao,
     int rc = 0;
 
     switch(ddev->dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VDISPL:
+    case LIBXL__DEVICE_KIND_VSND:
+    case LIBXL__DEVICE_KIND_VINPUT:
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*