[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V4 4/6] libxl: support creating domain with VF assignment from a pool



On 03/30/2016 03:38 AM, Chun Yan Liu wrote:
>
>>>> On 3/29/2016 at 08:05 AM, in message <56F9C6D0 5000702 suse com>, Jim Fehlig
> <jfehlig suse com> wrote: 
>> 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 suse com> 
>>> --- 
>>>  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), 
> For pv guest, from testing, it is shown in guest. For HVM, seems
> xl pci-attach has the same result.

Yes, I'm seeing the same thing. Looks like a bug in libxl or qemu.

>> nor in 
>>    dumpxml or /var/run/libvirt/libxl/<dom-name>.xml.
> Ah, there is a mistake during a review change to original patch 1/6.
> Didn't notice that earlier.
>
> In libxlDomainAttachNetDevice:
> The if (!ret) is needed, maybe cleanup is not proper.
> Since for actual type is hostdev case, after a successful
> libxlDomainAttachHostDevice, we need to
> update vm->def->nets too.
>
>   cleanup:
>      libxl_device_nic_dispose(&nic);
> - out:
> -    if (!ret)
> +    if (!ret) {
>          vm->def->nets[vm->def->nnets++] = net;
> +    } else {
> +        virDomainNetRemoveHostdev(vm->def, net);
> +        networkReleaseActualDevice(vm->def, net);
> +    }
>
> Similar for libxDomainDetachNetDevice cleanup:
> Original is still needed.
>
>   cleanup:
>      libxl_device_nic_dispose(&nic);
> -    if (!ret)
> +    if (!ret) {
> +        networkReleaseActualDevice(vm->def, detach);
>          virDomainNetRemove(vm->def, detachidx);
> +    }

Yikes! I should have looked at the code closer before making those changes -
lesson learned. I sent a small series to fix it

https://www.redhat.com/archives/libvir-list/2016-March/msg01470.html

>> 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. 
> This is intended.
>
>>  
>> 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. 
> This is libxl problem. With xl pci-detach, it has the same issue.

Yep, another bug in xen-unstable libxl. With the small fixes in the series I
mentioned above, this patch looks and tests good :-). Since it has been around
for quite some time,  I've asked for permission to commit it even though we are
in 1.3.3 freeze.

Regards,
Jim


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]