[ovs-dev] [PATCH] netdev-dpdk: Fix crash when changing the vhost-user port.

[Daniele Di Proietto]

Thanks for the patch and the review!

Pushed to master, branch-2.5 and branch-2.4

On 22/03/2016 16:27, "Traynor, Kevin" <kevin.traynor at intel.com> wrote:

>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Tuesday, March 22, 2016 12:42 PM
>> To: dev at openvswitch.org; Daniele Di Proietto <diproiettod at vmware.com>
>> Cc: Dyasly Sergey <s.dyasly at samsung.com>; Ben Pfaff <blp at ovn.org>;
>>Flavio
>> Leitner <fbl at sysclose.org>; Traynor, Kevin <kevin.traynor at intel.com>;
>>Ilya
>> Maximets <i.maximets at samsung.com>
>> Subject: [PATCH] netdev-dpdk: Fix crash when changing the vhost-user
>>port.
>> 
>> According to netdev-provider API:
>> 	'The "destruct" function is not allowed to fail.'
>> 
>> netdev-dpdk breaks this restriction for vhost-user ports.
>> This leads to SIGABRT or SIGSEGV in dpdk_watchdog thread
>> because 'dealloc' will be called anyway indifferently
>> to result of 'destruct'.
>> 
>> For example, if we call
>> 	# ovs-vsctl set interface vhost1 ofport_request=5
>> while QEMU still attached, we'll get:
>> ------------------[cut]------------------
>> |dpdk|ERR|Can not remove port, vhost device still attached
>> VHOST_CONFIG: socket created, fd:98
>> VHOST_CONFIG: fail to bind fd:98, remove file:/home/vhost1 and try
>>again.
>> |dpdk|ERR|vhost-user socket device setup failure for socket /home/vhost1
>> |bridge|WARN|could not open network device vhost1 (Unknown error -1)
>> ovs-vswitchd(dpdk_watchdog1): lib/netdev-dpdk.c:532: ovs_mutex_lock_at()
>>         passed uninitialized ovs_mutex
>> 
>> Program received signal SIGABRT, Aborted.
>> ------------------[cut]------------------
>> 
>> Fix that by removing port anyway even when guest is still
>> attached. Guest becomes an orphan in that case but OVS
>> will not crash and will continue forwarding for other ports.
>> VM restart required to restore connectivity.
>
>The issue in destruct was reported (without a crash) by Jan also.
>http://openvswitch.org/pipermail/discuss/2016-February/020271.html
>
>I wanted to try and fold in re-add without restarting the guest along
>with this. I got it working for most part but without vhost client mode
>support in DPDK, it's hard to cover all possibilities and not at least
>leak the sockets. I think a re-add without restarting the guest is
>something to revisit when vhost pmd and vhost client mode are available
>in DPDK. 
>
>Acked-by: Kevin Traynor <kevin.traynor at intel.com>
>
>> 
>> Fixes: 58397e6c1e6c ("netdev-dpdk: add dpdk vhost-cuse ports")
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>  lib/netdev-dpdk.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 6ac0eec..f4ed210 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -869,10 +869,13 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev_)
>>  {
>>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev_);
>> 
>> -    /* Can't remove a port while a guest is attached to it. */
>> +    /* Guest becomes an orphan if still attached. */
>>      if (netdev_dpdk_get_virtio(dev) != NULL) {
>> -        VLOG_ERR("Can not remove port, vhost device still attached");
>> -                return;
>> +        VLOG_ERR("Removing port '%s' while vhost device still
>>attached.",
>> +                 netdev_->name);
>> +        VLOG_ERR("To restore connectivity after re-adding of port, VM
>>on
>> socket"
>> +                 " '%s' must be restarted.",
>> +                 dev->vhost_id);
>>      }
>> 
>>      if (rte_vhost_driver_unregister(dev->vhost_id)) {
>> --
>> 2.5.0
>