>> On 3/29/2016 at 08:05 AM, in message <56F9C6D0.5000702(a)suse.com&gt;, Jim Fehlig

On 03/21/2016 02:11 AM, Chunyan Liu wrote: > Add codes to support creating domain with network defition of assigning > SRIOV VF from a pool. > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; > --- > src/libxl/libxl_domain.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ > tests/Makefile.am | 3 +++ > 2 files changed, 53 insertions(+) > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index c8d09b1..d11bf3a 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -36,6 +36,7 @@ > #include "virtime.h" > #include "locking/domain_lock.h" > #include "xen_common.h" > +#include "network/bridge_driver.h" > > #define VIR_FROM_THIS VIR_FROM_LIBXL > > @@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, > if (net->ifname && > STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) > VIR_FREE(net->ifname); > + > + /* cleanup actual device */ > + virDomainNetRemoveHostdev(vm->def, net); > + networkReleaseActualDevice(vm->def, net); > } > } > > @@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) > } > } > > +static int > +libxlNetworkPrepareDevices(virDomainDefPtr def) > +{ > + int ret = -1; > + size_t i; > + > + for (i = 0; i < def->nnets; i++) { > + virDomainNetDefPtr net = def->nets[i]; > + int actualType; > + > + /* If appropriate, grab a physical device from the configured > + * network's pool of devices, or resolve bridge device name > + * to the one defined in the network definition. > + */ > + if (networkAllocateActualDevice(def, net) < 0) > + goto cleanup; > + > + actualType = virDomainNetGetActualType(net); > + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && > + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + /* Each type='hostdev' network device must also have a > + * corresponding entry in the hostdevs array. For netdevs > + * that are hardcoded as type='hostdev', this is already > + * done by the parser, but for those allocated from a > + * network / determined at runtime, we need to do it > + * separately. > + */ > + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); > + virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; > + > + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && > + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; > + > + if (virDomainHostdevInsert(def, hostdev) < 0) > + goto cleanup; > + } > + } > + ret = 0; > + cleanup: > + return ret; Nothing to cleanup here. I think we should just return -1 on failure paths and 0 on success. > +} > > /* > * Start a domain through libxenlight. > @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > vm, true) < 0) > goto cleanup; > > + if (libxlNetworkPrepareDevices(vm->def) < 0) > + goto cleanup; I accessed a machine to test this series and have found a few problems. 1. When attaching an SR-IOV vf from a pool, the attach appears successful. I see that xen's pciback driver is bound to the device in the host. I didn't see the device in the guest (could be a guest problem), nor in dumpxml or /var/run/libvirt/libxl/<dom-name>.xml. Not surprisingly, this causes problems with device-detach # virsh detach-device dom sriov-pool-vif.xml error: Failed to detach device from sriov-pool-vif.xml error: operation failed: no device matching mac address 00:16:3e:7a:35:df found 2. After starting a domain containing an interface from sr-iov vf pool, the interface xml is changed from type='network' to type='hostdev'. E.g. before creating domain <interface type='network'> <source network='passthrough'/> <mac address='00:16:3e:7a:35:df'/> </interface> after creating domain <interface type='hostdev' managed='yes'> <mac address='00:16:3e:7a:35:df'/> <driver name='xen'/> <source> <address type='pci' domain='0x0000' bus='0x0a' slot='0x11' function='0x1'/> </source> </interface> Maybe this is intended behavior, but it seems odd. 3. I started a domain containing an interface from sr-iov vf pool. Looks good. I then tried 'virsh detach-device ...', which never returned until keepalive timeout. The device was removed from the guest, but libvirtd got hung up in the process. Oddly, I wasn't able to interrupt libvirtd with gdb to see what its threads were doing. Do you have access to an appropriate machine to test these scenarios, to see if the issues are reproducible or just a problem with my configuration?

Regards, Jim

>> On 3/29/2016 at 09:00 AM, in message <56F9D3C0.7020907(a)suse.com&gt;, Jim Fehlig

On 03/21/2016 02:11 AM, Chunyan Liu wrote: > Functions like libxlNetworkPrepareDevices, libxlBuildDomainConfig, > virHostdevPrepareDomainDevices will allocate and reserve some > resources. In libxlDomainStart, after calling these functions, > in case of failure, we need to call libxlDomainCleanup to check > and release resoureces. > Besides, since libxlDomainCleanup will call virDomainLockProcessPause, > so we move virDomainLockProcessStart/Resume to earlier stage. I think this cleanup should be done before applying 4/6. It is really independent of support for vf from an SR-IOV pool. > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; > --- > src/libxl/libxl_domain.c | 41 ++++++++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 19 deletions(-) > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index d11bf3a..6855ce4 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -1083,20 +1083,13 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > vm, true) < 0) > goto cleanup; > > - if (libxlNetworkPrepareDevices(vm->def) < 0) > - goto cleanup; > - > - if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, > - cfg->ctx, &d_config) < 0) > - goto cleanup; > - > - if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) > - goto cleanup; > - > - if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > - vm->def, VIR_HOSTDEV_SP_PCI) < 0) > - goto cleanup; > - > + /* libxlNetworkPrepareDevices, libxlBuildDomainConfig, > + * virHostdevPrepareDomainDevices will allocate and reserve > + * some resources. In case of failure, need to release > + * resoureces. This can be done by calling libxlDomainCleanup. > + * Since libxlDomainCleanup will call virDomainLockProcessPause, > + * so we move virDomainLockProcessStart/Resume to here. > + */ I don't think the comment is necessary. Without looking at git history, one wouldn't know where it was moved from :-). > if (virDomainLockProcessStart(driver->lockManager, > "xen:///system", > vm, > @@ -1111,6 +1104,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > goto cleanup; > VIR_FREE(priv->lockState); > > + if (libxlNetworkPrepareDevices(vm->def) < 0) > + goto release_dom; > + > + if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, > + cfg->ctx, &d_config) < 0) > + goto release_dom; > + > + if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) > + goto release_dom; > + > + if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def, VIR_HOSTDEV_SP_PCI) < 0) > + goto release_dom; > + > /* Unlock virDomainObj while creating the domain */ > virObjectUnlock(vm); > > @@ -1194,16 +1201,12 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > > cleanup_dom: > ret = -1; > - if (priv->deathW) { > - libxl_evdisable_domain_death(cfg->ctx, priv->deathW); > - priv->deathW = NULL; > - } Disabling the death event should be done in another patch IMO, after adding the call to libxlDomainCleanup. > libxlDomainDestroyInternal(driver, vm); > vm->def->id = -1; > virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); > > release_dom: > - virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState); > + libxlDomainCleanup(driver, vm); release_dom is not the best name for the label after this change. I've split this patch into 3 patches and posted the result https://www.redhat.com/archives/libvir-list/2016-March/msg01310.html

Can you review and comment on that series? As mentioned above, I'd like to get the resource leaks plugged in libxlDomainStart before adding 4/6. Regards, Jim