* [PATCH] vhost: fix driver unregister for client mode
@ 2016-07-20 8:32 Ilya Maximets
2016-07-20 12:38 ` Yuanhan Liu
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Ilya Maximets @ 2016-07-20 8:32 UTC (permalink / raw)
To: dev, Huawei Xie, Yuanhan Liu
Cc: Dyasly Sergey, Heetae Ahn, Thomas Monjalon, Ilya Maximets
Currently while calling of 'rte_vhost_driver_unregister()' connection
to QEMU will not be closed. This leads to inability to register driver
again and reconnect to same virtual machine.
This scenario is reproducible with OVS. While executing of the following
command vhost port will be re-created (will be executed
'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
network will be broken and QEMU possibly will crash:
ovs-vsctl set Interface vhost1 ofport_request=15
Fix this by closing all established connections on driver unregister and
removing of pending connections from reconnection list.
Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
Patch prepared for master branch because dpdk-next-virtio doesn't contain
commit acbff5c67ea7 ("vhost: fix crash when exceeding file descriptors").
Porting to dpdk-next-virtio/master is trivial and may be performed on
demand.
lib/librte_vhost/vhost_user/fd_man.c | 15 ++++++--
lib/librte_vhost/vhost_user/fd_man.h | 2 +-
lib/librte_vhost/vhost_user/vhost-net-user.c | 56 ++++++++++++++++++++++++----
3 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index c691339..2d3eeb7 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -132,8 +132,10 @@ fdset_init(struct fdset *pfdset)
if (pfdset == NULL)
return;
- for (i = 0; i < MAX_FDS; i++)
+ for (i = 0; i < MAX_FDS; i++) {
pfdset->fd[i].fd = -1;
+ pfdset->fd[i].dat = NULL;
+ }
pfdset->num = 0;
}
@@ -166,14 +168,16 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
/**
* Unregister the fd from the fdset.
+ * Returns context of a given fd or NULL.
*/
-void
+void *
fdset_del(struct fdset *pfdset, int fd)
{
int i;
+ void *dat = NULL;
if (pfdset == NULL || fd == -1)
- return;
+ return NULL;
do {
pthread_mutex_lock(&pfdset->fd_mutex);
@@ -181,13 +185,17 @@ fdset_del(struct fdset *pfdset, int fd)
i = fdset_find_fd(pfdset, fd);
if (i != -1 && pfdset->fd[i].busy == 0) {
/* busy indicates r/wcb is executing! */
+ dat = pfdset->fd[i].dat;
pfdset->fd[i].fd = -1;
pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
+ pfdset->fd[i].dat = NULL;
pfdset->num--;
i = -1;
}
pthread_mutex_unlock(&pfdset->fd_mutex);
} while (i != -1);
+
+ return dat;
}
/**
@@ -203,6 +211,7 @@ fdset_del_slot(struct fdset *pfdset, int index)
pfdset->fd[index].fd = -1;
pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
+ pfdset->fd[index].dat = NULL;
pfdset->num--;
pthread_mutex_unlock(&pfdset->fd_mutex);
diff --git a/lib/librte_vhost/vhost_user/fd_man.h b/lib/librte_vhost/vhost_user/fd_man.h
index 74ecde2..bd66ed1 100644
--- a/lib/librte_vhost/vhost_user/fd_man.h
+++ b/lib/librte_vhost/vhost_user/fd_man.h
@@ -60,7 +60,7 @@ void fdset_init(struct fdset *pfdset);
int fdset_add(struct fdset *pfdset, int fd,
fd_cb rcb, fd_cb wcb, void *dat);
-void fdset_del(struct fdset *pfdset, int fd);
+void *fdset_del(struct fdset *pfdset, int fd);
void fdset_event_dispatch(struct fdset *pfdset);
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index f0ea3a2..f0f92f8 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -60,6 +60,7 @@
struct vhost_user_socket {
char *path;
int listenfd;
+ int connfd;
bool is_server;
bool reconnect;
};
@@ -277,11 +278,13 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
+ vsocket->connfd = fd;
conn->vsocket = vsocket;
conn->vid = vid;
ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
NULL, conn);
if (ret < 0) {
+ vsocket->connfd = -1;
free(conn);
close(fd);
RTE_LOG(ERR, VHOST_CONFIG,
@@ -329,6 +332,7 @@ vhost_user_msg_handler(int connfd, void *dat, int *remove)
RTE_LOG(ERR, VHOST_CONFIG,
"vhost read incorrect message\n");
+ vsocket->connfd = -1;
close(connfd);
*remove = 1;
free(conn);
@@ -635,6 +639,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
goto out;
memset(vsocket, 0, sizeof(struct vhost_user_socket));
vsocket->path = strdup(path);
+ vsocket->connfd = -1;
if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
@@ -664,6 +669,29 @@ out:
return ret;
}
+static bool
+vhost_user_remove_reconnect(struct vhost_user_socket *vsocket)
+{
+ int found = false;
+ struct vhost_user_reconnect *reconn, *next;
+
+ pthread_mutex_lock(&reconn_list.mutex);
+
+ for (reconn = TAILQ_FIRST(&reconn_list.head);
+ reconn != NULL; reconn = next) {
+ next = TAILQ_NEXT(reconn, next);
+
+ if (reconn->vsocket == vsocket) {
+ TAILQ_REMOVE(&reconn_list.head, reconn, next);
+ free(reconn);
+ found = true;
+ break;
+ }
+ }
+ pthread_mutex_unlock(&reconn_list.mutex);
+ return found;
+}
+
/**
* Unregister the specified vhost socket
*/
@@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
{
int i;
int count;
+ struct vhost_user_connection *conn;
pthread_mutex_lock(&vhost_user.mutex);
for (i = 0; i < vhost_user.vsocket_cnt; i++) {
- if (!strcmp(vhost_user.vsockets[i]->path, path)) {
- if (vhost_user.vsockets[i]->is_server) {
- fdset_del(&vhost_user.fdset,
- vhost_user.vsockets[i]->listenfd);
- close(vhost_user.vsockets[i]->listenfd);
+ struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
+
+ if (!strcmp(vsocket->path, path)) {
+ if (vsocket->is_server) {
+ (void) fdset_del(&vhost_user.fdset,
+ vsocket->listenfd);
+ close(vsocket->listenfd);
unlink(path);
+ } else if (vsocket->reconnect) {
+ vhost_user_remove_reconnect(vsocket);
+ }
+
+ conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
+ if (conn) {
+ RTE_LOG(INFO, VHOST_CONFIG, "free connfd = %d"
+ "for device '%s'\n", vsocket->connfd, path);
+ close(vsocket->connfd);
+ vhost_destroy_device(conn->vid);
+ free(conn);
}
- free(vhost_user.vsockets[i]->path);
- free(vhost_user.vsockets[i]);
+ free(vsocket->path);
+ free(vsocket);
count = --vhost_user.vsocket_cnt;
vhost_user.vsockets[i] = vhost_user.vsockets[count];
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] vhost: fix driver unregister for client mode
2016-07-20 8:32 [PATCH] vhost: fix driver unregister for client mode Ilya Maximets
@ 2016-07-20 12:38 ` Yuanhan Liu
2016-07-21 8:24 ` Yuanhan Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Yuanhan Liu @ 2016-07-20 12:38 UTC (permalink / raw)
To: Ilya Maximets; +Cc: dev, Huawei Xie, Dyasly Sergey, Heetae Ahn, Thomas Monjalon
On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote:
> Currently while calling of 'rte_vhost_driver_unregister()' connection
> to QEMU will not be closed. This leads to inability to register driver
> again and reconnect to same virtual machine.
>
> This scenario is reproducible with OVS. While executing of the following
> command vhost port will be re-created (will be executed
> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> network will be broken and QEMU possibly will crash:
>
> ovs-vsctl set Interface vhost1 ofport_request=15
>
> Fix this by closing all established connections on driver unregister and
> removing of pending connections from reconnection list.
>
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Ilya, Thanks a lot for the test and fix. It looks good to me. But
somehow I would like have a test tomorrow.
--yliu
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] vhost: fix driver unregister for client mode
2016-07-20 8:32 [PATCH] vhost: fix driver unregister for client mode Ilya Maximets
2016-07-20 12:38 ` Yuanhan Liu
@ 2016-07-21 8:24 ` Yuanhan Liu
2016-07-21 8:33 ` Ilya Maximets
2016-07-21 8:31 ` [PATCH v2] " Ilya Maximets
2016-07-21 12:55 ` [PATCH v3] " Ilya Maximets
3 siblings, 1 reply; 9+ messages in thread
From: Yuanhan Liu @ 2016-07-21 8:24 UTC (permalink / raw)
To: Ilya Maximets; +Cc: dev, Huawei Xie, Dyasly Sergey, Heetae Ahn, Thomas Monjalon
On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote:
> Currently while calling of 'rte_vhost_driver_unregister()' connection
> to QEMU will not be closed. This leads to inability to register driver
> again and reconnect to same virtual machine.
>
> This scenario is reproducible with OVS. While executing of the following
> command vhost port will be re-created (will be executed
> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> network will be broken and QEMU possibly will crash:
>
> ovs-vsctl set Interface vhost1 ofport_request=15
>
> Fix this by closing all established connections on driver unregister and
> removing of pending connections from reconnection list.
>
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Again, thanks for the fix.
> ---
>
> Patch prepared for master branch because dpdk-next-virtio doesn't contain
> commit acbff5c67ea7 ("vhost: fix crash when exceeding file descriptors").
> Porting to dpdk-next-virtio/master is trivial and may be performed on
> demand.
Yeah, my bad, I haven't updated it after rc2, since Thomas no longer
pull request from it. Anyway, you just remind me that I should have
done that.
> /**
> * Unregister the specified vhost socket
> */
> @@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
> {
> int i;
> int count;
> + struct vhost_user_connection *conn;
>
> pthread_mutex_lock(&vhost_user.mutex);
>
> for (i = 0; i < vhost_user.vsocket_cnt; i++) {
> - if (!strcmp(vhost_user.vsockets[i]->path, path)) {
> - if (vhost_user.vsockets[i]->is_server) {
> - fdset_del(&vhost_user.fdset,
> - vhost_user.vsockets[i]->listenfd);
> - close(vhost_user.vsockets[i]->listenfd);
> + struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
> +
> + if (!strcmp(vsocket->path, path)) {
> + if (vsocket->is_server) {
> + (void) fdset_del(&vhost_user.fdset,
> + vsocket->listenfd);
I would think the (void) cast is not neceessary here.
> + close(vsocket->listenfd);
> unlink(path);
> + } else if (vsocket->reconnect) {
> + vhost_user_remove_reconnect(vsocket);
> + }
> +
> + conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
> + if (conn) {
> + RTE_LOG(INFO, VHOST_CONFIG, "free connfd = %d"
> + "for device '%s'\n", vsocket->connfd, path);
We should try not to break a log message into several lines, which
hurts "git grep". Here, it could be:
RTE_LOG(INFO, VHOST_CONFIG,
"free connfd = %d for device '%s'\n",
vsocket->connfd, path);
Besides the two minor nits,
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
--yliu
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] vhost: fix driver unregister for client mode
2016-07-20 8:32 [PATCH] vhost: fix driver unregister for client mode Ilya Maximets
2016-07-20 12:38 ` Yuanhan Liu
2016-07-21 8:24 ` Yuanhan Liu
@ 2016-07-21 8:31 ` Ilya Maximets
2016-07-21 12:57 ` Ilya Maximets
2016-07-21 12:55 ` [PATCH v3] " Ilya Maximets
3 siblings, 1 reply; 9+ messages in thread
From: Ilya Maximets @ 2016-07-21 8:31 UTC (permalink / raw)
To: dev, Huawei Xie, Yuanhan Liu
Cc: Dyasly Sergey, Heetae Ahn, Thomas Monjalon, Ilya Maximets
Currently while calling of 'rte_vhost_driver_unregister()' connection
to QEMU will not be closed. This leads to inability to register driver
again and reconnect to same virtual machine.
This scenario is reproducible with OVS. While executing of the following
command vhost port will be re-created (will be executed
'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
network will be broken and QEMU possibly will crash:
ovs-vsctl set Interface vhost1 ofport_request=15
Fix this by closing all established connections on driver unregister and
removing of pending connections from reconnection list.
Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
lib/librte_vhost/vhost_user/fd_man.c | 15 ++++++--
lib/librte_vhost/vhost_user/fd_man.h | 2 +-
lib/librte_vhost/vhost_user/vhost-net-user.c | 56 ++++++++++++++++++++++++----
3 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index c691339..2d3eeb7 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -132,8 +132,10 @@ fdset_init(struct fdset *pfdset)
if (pfdset == NULL)
return;
- for (i = 0; i < MAX_FDS; i++)
+ for (i = 0; i < MAX_FDS; i++) {
pfdset->fd[i].fd = -1;
+ pfdset->fd[i].dat = NULL;
+ }
pfdset->num = 0;
}
@@ -166,14 +168,16 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
/**
* Unregister the fd from the fdset.
+ * Returns context of a given fd or NULL.
*/
-void
+void *
fdset_del(struct fdset *pfdset, int fd)
{
int i;
+ void *dat = NULL;
if (pfdset == NULL || fd == -1)
- return;
+ return NULL;
do {
pthread_mutex_lock(&pfdset->fd_mutex);
@@ -181,13 +185,17 @@ fdset_del(struct fdset *pfdset, int fd)
i = fdset_find_fd(pfdset, fd);
if (i != -1 && pfdset->fd[i].busy == 0) {
/* busy indicates r/wcb is executing! */
+ dat = pfdset->fd[i].dat;
pfdset->fd[i].fd = -1;
pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
+ pfdset->fd[i].dat = NULL;
pfdset->num--;
i = -1;
}
pthread_mutex_unlock(&pfdset->fd_mutex);
} while (i != -1);
+
+ return dat;
}
/**
@@ -203,6 +211,7 @@ fdset_del_slot(struct fdset *pfdset, int index)
pfdset->fd[index].fd = -1;
pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
+ pfdset->fd[index].dat = NULL;
pfdset->num--;
pthread_mutex_unlock(&pfdset->fd_mutex);
diff --git a/lib/librte_vhost/vhost_user/fd_man.h b/lib/librte_vhost/vhost_user/fd_man.h
index 74ecde2..bd66ed1 100644
--- a/lib/librte_vhost/vhost_user/fd_man.h
+++ b/lib/librte_vhost/vhost_user/fd_man.h
@@ -60,7 +60,7 @@ void fdset_init(struct fdset *pfdset);
int fdset_add(struct fdset *pfdset, int fd,
fd_cb rcb, fd_cb wcb, void *dat);
-void fdset_del(struct fdset *pfdset, int fd);
+void *fdset_del(struct fdset *pfdset, int fd);
void fdset_event_dispatch(struct fdset *pfdset);
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index f0ea3a2..8c6a096 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -60,6 +60,7 @@
struct vhost_user_socket {
char *path;
int listenfd;
+ int connfd;
bool is_server;
bool reconnect;
};
@@ -277,11 +278,13 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
+ vsocket->connfd = fd;
conn->vsocket = vsocket;
conn->vid = vid;
ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
NULL, conn);
if (ret < 0) {
+ vsocket->connfd = -1;
free(conn);
close(fd);
RTE_LOG(ERR, VHOST_CONFIG,
@@ -329,6 +332,7 @@ vhost_user_msg_handler(int connfd, void *dat, int *remove)
RTE_LOG(ERR, VHOST_CONFIG,
"vhost read incorrect message\n");
+ vsocket->connfd = -1;
close(connfd);
*remove = 1;
free(conn);
@@ -635,6 +639,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
goto out;
memset(vsocket, 0, sizeof(struct vhost_user_socket));
vsocket->path = strdup(path);
+ vsocket->connfd = -1;
if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
@@ -664,6 +669,29 @@ out:
return ret;
}
+static bool
+vhost_user_remove_reconnect(struct vhost_user_socket *vsocket)
+{
+ int found = false;
+ struct vhost_user_reconnect *reconn, *next;
+
+ pthread_mutex_lock(&reconn_list.mutex);
+
+ for (reconn = TAILQ_FIRST(&reconn_list.head);
+ reconn != NULL; reconn = next) {
+ next = TAILQ_NEXT(reconn, next);
+
+ if (reconn->vsocket == vsocket) {
+ TAILQ_REMOVE(&reconn_list.head, reconn, next);
+ free(reconn);
+ found = true;
+ break;
+ }
+ }
+ pthread_mutex_unlock(&reconn_list.mutex);
+ return found;
+}
+
/**
* Unregister the specified vhost socket
*/
@@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
{
int i;
int count;
+ struct vhost_user_connection *conn;
pthread_mutex_lock(&vhost_user.mutex);
for (i = 0; i < vhost_user.vsocket_cnt; i++) {
- if (!strcmp(vhost_user.vsockets[i]->path, path)) {
- if (vhost_user.vsockets[i]->is_server) {
- fdset_del(&vhost_user.fdset,
- vhost_user.vsockets[i]->listenfd);
- close(vhost_user.vsockets[i]->listenfd);
+ struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
+
+ if (!strcmp(vsocket->path, path)) {
+ if (vsocket->is_server) {
+ fdset_del(&vhost_user.fdset, vsocket->listenfd);
+ close(vsocket->listenfd);
unlink(path);
+ } else if (vsocket->reconnect) {
+ vhost_user_remove_reconnect(vsocket);
+ }
+
+ conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
+ if (conn) {
+ RTE_LOG(INFO, VHOST_CONFIG,
+ "free connfd = %d for device '%s'\n",
+ vsocket->connfd, path);
+ close(vsocket->connfd);
+ vhost_destroy_device(conn->vid);
+ free(conn);
}
- free(vhost_user.vsockets[i]->path);
- free(vhost_user.vsockets[i]);
+ free(vsocket->path);
+ free(vsocket);
count = --vhost_user.vsocket_cnt;
vhost_user.vsockets[i] = vhost_user.vsockets[count];
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] vhost: fix driver unregister for client mode
2016-07-21 8:24 ` Yuanhan Liu
@ 2016-07-21 8:33 ` Ilya Maximets
0 siblings, 0 replies; 9+ messages in thread
From: Ilya Maximets @ 2016-07-21 8:33 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, Huawei Xie, Dyasly Sergey, Heetae Ahn, Thomas Monjalon
Thanks. Fixed.
Best regards, Ilya Maximets.
On 21.07.2016 11:24, Yuanhan Liu wrote:
> On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote:
>> Currently while calling of 'rte_vhost_driver_unregister()' connection
>> to QEMU will not be closed. This leads to inability to register driver
>> again and reconnect to same virtual machine.
>>
>> This scenario is reproducible with OVS. While executing of the following
>> command vhost port will be re-created (will be executed
>> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
>> network will be broken and QEMU possibly will crash:
>>
>> ovs-vsctl set Interface vhost1 ofport_request=15
>>
>> Fix this by closing all established connections on driver unregister and
>> removing of pending connections from reconnection list.
>>
>> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>
> Again, thanks for the fix.
>
>> ---
>>
>> Patch prepared for master branch because dpdk-next-virtio doesn't contain
>> commit acbff5c67ea7 ("vhost: fix crash when exceeding file descriptors").
>> Porting to dpdk-next-virtio/master is trivial and may be performed on
>> demand.
>
> Yeah, my bad, I haven't updated it after rc2, since Thomas no longer
> pull request from it. Anyway, you just remind me that I should have
> done that.
>
>> /**
>> * Unregister the specified vhost socket
>> */
>> @@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
>> {
>> int i;
>> int count;
>> + struct vhost_user_connection *conn;
>>
>> pthread_mutex_lock(&vhost_user.mutex);
>>
>> for (i = 0; i < vhost_user.vsocket_cnt; i++) {
>> - if (!strcmp(vhost_user.vsockets[i]->path, path)) {
>> - if (vhost_user.vsockets[i]->is_server) {
>> - fdset_del(&vhost_user.fdset,
>> - vhost_user.vsockets[i]->listenfd);
>> - close(vhost_user.vsockets[i]->listenfd);
>> + struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
>> +
>> + if (!strcmp(vsocket->path, path)) {
>> + if (vsocket->is_server) {
>> + (void) fdset_del(&vhost_user.fdset,
>> + vsocket->listenfd);
>
> I would think the (void) cast is not neceessary here.
>
>> + close(vsocket->listenfd);
>> unlink(path);
>> + } else if (vsocket->reconnect) {
>> + vhost_user_remove_reconnect(vsocket);
>> + }
>> +
>> + conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
>> + if (conn) {
>> + RTE_LOG(INFO, VHOST_CONFIG, "free connfd = %d"
>> + "for device '%s'\n", vsocket->connfd, path);
>
> We should try not to break a log message into several lines, which
> hurts "git grep". Here, it could be:
>
> RTE_LOG(INFO, VHOST_CONFIG,
> "free connfd = %d for device '%s'\n",
> vsocket->connfd, path);
>
> Besides the two minor nits,
>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>
> --yliu
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] vhost: fix driver unregister for client mode
2016-07-20 8:32 [PATCH] vhost: fix driver unregister for client mode Ilya Maximets
` (2 preceding siblings ...)
2016-07-21 8:31 ` [PATCH v2] " Ilya Maximets
@ 2016-07-21 12:55 ` Ilya Maximets
2016-07-21 13:18 ` Yuanhan Liu
3 siblings, 1 reply; 9+ messages in thread
From: Ilya Maximets @ 2016-07-21 12:55 UTC (permalink / raw)
To: dev, Huawei Xie, Yuanhan Liu
Cc: Dyasly Sergey, Heetae Ahn, Thomas Monjalon, Ilya Maximets
Currently while calling of 'rte_vhost_driver_unregister()' connection
to QEMU will not be closed. This leads to inability to register driver
again and reconnect to same virtual machine.
This scenario is reproducible with OVS. While executing of the following
command vhost port will be re-created (will be executed
'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
network will be broken and QEMU possibly will crash:
ovs-vsctl set Interface vhost1 ofport_request=15
Fix this by closing all established connections on driver unregister and
removing of pending connections from reconnection list.
Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
Version 3:
* fixed leak of file descriptors by adding of
'close(reconn->fd)' to 'vhost_user_remove_reconnect()'
Version 2:
* style fixes.
lib/librte_vhost/vhost_user/fd_man.c | 15 ++++++--
lib/librte_vhost/vhost_user/fd_man.h | 2 +-
lib/librte_vhost/vhost_user/vhost-net-user.c | 57 ++++++++++++++++++++++++----
3 files changed, 63 insertions(+), 11 deletions(-)
diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index c691339..2d3eeb7 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -132,8 +132,10 @@ fdset_init(struct fdset *pfdset)
if (pfdset == NULL)
return;
- for (i = 0; i < MAX_FDS; i++)
+ for (i = 0; i < MAX_FDS; i++) {
pfdset->fd[i].fd = -1;
+ pfdset->fd[i].dat = NULL;
+ }
pfdset->num = 0;
}
@@ -166,14 +168,16 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
/**
* Unregister the fd from the fdset.
+ * Returns context of a given fd or NULL.
*/
-void
+void *
fdset_del(struct fdset *pfdset, int fd)
{
int i;
+ void *dat = NULL;
if (pfdset == NULL || fd == -1)
- return;
+ return NULL;
do {
pthread_mutex_lock(&pfdset->fd_mutex);
@@ -181,13 +185,17 @@ fdset_del(struct fdset *pfdset, int fd)
i = fdset_find_fd(pfdset, fd);
if (i != -1 && pfdset->fd[i].busy == 0) {
/* busy indicates r/wcb is executing! */
+ dat = pfdset->fd[i].dat;
pfdset->fd[i].fd = -1;
pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
+ pfdset->fd[i].dat = NULL;
pfdset->num--;
i = -1;
}
pthread_mutex_unlock(&pfdset->fd_mutex);
} while (i != -1);
+
+ return dat;
}
/**
@@ -203,6 +211,7 @@ fdset_del_slot(struct fdset *pfdset, int index)
pfdset->fd[index].fd = -1;
pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
+ pfdset->fd[index].dat = NULL;
pfdset->num--;
pthread_mutex_unlock(&pfdset->fd_mutex);
diff --git a/lib/librte_vhost/vhost_user/fd_man.h b/lib/librte_vhost/vhost_user/fd_man.h
index 74ecde2..bd66ed1 100644
--- a/lib/librte_vhost/vhost_user/fd_man.h
+++ b/lib/librte_vhost/vhost_user/fd_man.h
@@ -60,7 +60,7 @@ void fdset_init(struct fdset *pfdset);
int fdset_add(struct fdset *pfdset, int fd,
fd_cb rcb, fd_cb wcb, void *dat);
-void fdset_del(struct fdset *pfdset, int fd);
+void *fdset_del(struct fdset *pfdset, int fd);
void fdset_event_dispatch(struct fdset *pfdset);
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index f0ea3a2..62c5ec3 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -60,6 +60,7 @@
struct vhost_user_socket {
char *path;
int listenfd;
+ int connfd;
bool is_server;
bool reconnect;
};
@@ -277,11 +278,13 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
+ vsocket->connfd = fd;
conn->vsocket = vsocket;
conn->vid = vid;
ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
NULL, conn);
if (ret < 0) {
+ vsocket->connfd = -1;
free(conn);
close(fd);
RTE_LOG(ERR, VHOST_CONFIG,
@@ -329,6 +332,7 @@ vhost_user_msg_handler(int connfd, void *dat, int *remove)
RTE_LOG(ERR, VHOST_CONFIG,
"vhost read incorrect message\n");
+ vsocket->connfd = -1;
close(connfd);
*remove = 1;
free(conn);
@@ -635,6 +639,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
goto out;
memset(vsocket, 0, sizeof(struct vhost_user_socket));
vsocket->path = strdup(path);
+ vsocket->connfd = -1;
if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
@@ -664,6 +669,30 @@ out:
return ret;
}
+static bool
+vhost_user_remove_reconnect(struct vhost_user_socket *vsocket)
+{
+ int found = false;
+ struct vhost_user_reconnect *reconn, *next;
+
+ pthread_mutex_lock(&reconn_list.mutex);
+
+ for (reconn = TAILQ_FIRST(&reconn_list.head);
+ reconn != NULL; reconn = next) {
+ next = TAILQ_NEXT(reconn, next);
+
+ if (reconn->vsocket == vsocket) {
+ TAILQ_REMOVE(&reconn_list.head, reconn, next);
+ close(reconn->fd);
+ free(reconn);
+ found = true;
+ break;
+ }
+ }
+ pthread_mutex_unlock(&reconn_list.mutex);
+ return found;
+}
+
/**
* Unregister the specified vhost socket
*/
@@ -672,20 +701,34 @@ rte_vhost_driver_unregister(const char *path)
{
int i;
int count;
+ struct vhost_user_connection *conn;
pthread_mutex_lock(&vhost_user.mutex);
for (i = 0; i < vhost_user.vsocket_cnt; i++) {
- if (!strcmp(vhost_user.vsockets[i]->path, path)) {
- if (vhost_user.vsockets[i]->is_server) {
- fdset_del(&vhost_user.fdset,
- vhost_user.vsockets[i]->listenfd);
- close(vhost_user.vsockets[i]->listenfd);
+ struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
+
+ if (!strcmp(vsocket->path, path)) {
+ if (vsocket->is_server) {
+ fdset_del(&vhost_user.fdset, vsocket->listenfd);
+ close(vsocket->listenfd);
unlink(path);
+ } else if (vsocket->reconnect) {
+ vhost_user_remove_reconnect(vsocket);
+ }
+
+ conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
+ if (conn) {
+ RTE_LOG(INFO, VHOST_CONFIG,
+ "free connfd = %d for device '%s'\n",
+ vsocket->connfd, path);
+ close(vsocket->connfd);
+ vhost_destroy_device(conn->vid);
+ free(conn);
}
- free(vhost_user.vsockets[i]->path);
- free(vhost_user.vsockets[i]);
+ free(vsocket->path);
+ free(vsocket);
count = --vhost_user.vsocket_cnt;
vhost_user.vsockets[i] = vhost_user.vsockets[count];
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vhost: fix driver unregister for client mode
2016-07-21 8:31 ` [PATCH v2] " Ilya Maximets
@ 2016-07-21 12:57 ` Ilya Maximets
0 siblings, 0 replies; 9+ messages in thread
From: Ilya Maximets @ 2016-07-21 12:57 UTC (permalink / raw)
To: dev, Huawei Xie, Yuanhan Liu; +Cc: Dyasly Sergey, Heetae Ahn, Thomas Monjalon
I've fixed leak of file descriptors in 'vhost_user_remove_reconnect()'
and sent v3.
On 21.07.2016 11:31, Ilya Maximets wrote:
> Currently while calling of 'rte_vhost_driver_unregister()' connection
> to QEMU will not be closed. This leads to inability to register driver
> again and reconnect to same virtual machine.
>
> This scenario is reproducible with OVS. While executing of the following
> command vhost port will be re-created (will be executed
> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> network will be broken and QEMU possibly will crash:
>
> ovs-vsctl set Interface vhost1 ofport_request=15
>
> Fix this by closing all established connections on driver unregister and
> removing of pending connections from reconnection list.
>
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> lib/librte_vhost/vhost_user/fd_man.c | 15 ++++++--
> lib/librte_vhost/vhost_user/fd_man.h | 2 +-
> lib/librte_vhost/vhost_user/vhost-net-user.c | 56 ++++++++++++++++++++++++----
> 3 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
> index c691339..2d3eeb7 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.c
> +++ b/lib/librte_vhost/vhost_user/fd_man.c
> @@ -132,8 +132,10 @@ fdset_init(struct fdset *pfdset)
> if (pfdset == NULL)
> return;
>
> - for (i = 0; i < MAX_FDS; i++)
> + for (i = 0; i < MAX_FDS; i++) {
> pfdset->fd[i].fd = -1;
> + pfdset->fd[i].dat = NULL;
> + }
> pfdset->num = 0;
> }
>
> @@ -166,14 +168,16 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
>
> /**
> * Unregister the fd from the fdset.
> + * Returns context of a given fd or NULL.
> */
> -void
> +void *
> fdset_del(struct fdset *pfdset, int fd)
> {
> int i;
> + void *dat = NULL;
>
> if (pfdset == NULL || fd == -1)
> - return;
> + return NULL;
>
> do {
> pthread_mutex_lock(&pfdset->fd_mutex);
> @@ -181,13 +185,17 @@ fdset_del(struct fdset *pfdset, int fd)
> i = fdset_find_fd(pfdset, fd);
> if (i != -1 && pfdset->fd[i].busy == 0) {
> /* busy indicates r/wcb is executing! */
> + dat = pfdset->fd[i].dat;
> pfdset->fd[i].fd = -1;
> pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
> + pfdset->fd[i].dat = NULL;
> pfdset->num--;
> i = -1;
> }
> pthread_mutex_unlock(&pfdset->fd_mutex);
> } while (i != -1);
> +
> + return dat;
> }
>
> /**
> @@ -203,6 +211,7 @@ fdset_del_slot(struct fdset *pfdset, int index)
>
> pfdset->fd[index].fd = -1;
> pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
> + pfdset->fd[index].dat = NULL;
> pfdset->num--;
>
> pthread_mutex_unlock(&pfdset->fd_mutex);
> diff --git a/lib/librte_vhost/vhost_user/fd_man.h b/lib/librte_vhost/vhost_user/fd_man.h
> index 74ecde2..bd66ed1 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.h
> +++ b/lib/librte_vhost/vhost_user/fd_man.h
> @@ -60,7 +60,7 @@ void fdset_init(struct fdset *pfdset);
> int fdset_add(struct fdset *pfdset, int fd,
> fd_cb rcb, fd_cb wcb, void *dat);
>
> -void fdset_del(struct fdset *pfdset, int fd);
> +void *fdset_del(struct fdset *pfdset, int fd);
>
> void fdset_event_dispatch(struct fdset *pfdset);
>
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index f0ea3a2..8c6a096 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -60,6 +60,7 @@
> struct vhost_user_socket {
> char *path;
> int listenfd;
> + int connfd;
> bool is_server;
> bool reconnect;
> };
> @@ -277,11 +278,13 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
>
> RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
>
> + vsocket->connfd = fd;
> conn->vsocket = vsocket;
> conn->vid = vid;
> ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
> NULL, conn);
> if (ret < 0) {
> + vsocket->connfd = -1;
> free(conn);
> close(fd);
> RTE_LOG(ERR, VHOST_CONFIG,
> @@ -329,6 +332,7 @@ vhost_user_msg_handler(int connfd, void *dat, int *remove)
> RTE_LOG(ERR, VHOST_CONFIG,
> "vhost read incorrect message\n");
>
> + vsocket->connfd = -1;
> close(connfd);
> *remove = 1;
> free(conn);
> @@ -635,6 +639,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
> goto out;
> memset(vsocket, 0, sizeof(struct vhost_user_socket));
> vsocket->path = strdup(path);
> + vsocket->connfd = -1;
>
> if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
> vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
> @@ -664,6 +669,29 @@ out:
> return ret;
> }
>
> +static bool
> +vhost_user_remove_reconnect(struct vhost_user_socket *vsocket)
> +{
> + int found = false;
> + struct vhost_user_reconnect *reconn, *next;
> +
> + pthread_mutex_lock(&reconn_list.mutex);
> +
> + for (reconn = TAILQ_FIRST(&reconn_list.head);
> + reconn != NULL; reconn = next) {
> + next = TAILQ_NEXT(reconn, next);
> +
> + if (reconn->vsocket == vsocket) {
> + TAILQ_REMOVE(&reconn_list.head, reconn, next);
> + free(reconn);
> + found = true;
> + break;
> + }
> + }
> + pthread_mutex_unlock(&reconn_list.mutex);
> + return found;
> +}
> +
> /**
> * Unregister the specified vhost socket
> */
> @@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
> {
> int i;
> int count;
> + struct vhost_user_connection *conn;
>
> pthread_mutex_lock(&vhost_user.mutex);
>
> for (i = 0; i < vhost_user.vsocket_cnt; i++) {
> - if (!strcmp(vhost_user.vsockets[i]->path, path)) {
> - if (vhost_user.vsockets[i]->is_server) {
> - fdset_del(&vhost_user.fdset,
> - vhost_user.vsockets[i]->listenfd);
> - close(vhost_user.vsockets[i]->listenfd);
> + struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
> +
> + if (!strcmp(vsocket->path, path)) {
> + if (vsocket->is_server) {
> + fdset_del(&vhost_user.fdset, vsocket->listenfd);
> + close(vsocket->listenfd);
> unlink(path);
> + } else if (vsocket->reconnect) {
> + vhost_user_remove_reconnect(vsocket);
> + }
> +
> + conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
> + if (conn) {
> + RTE_LOG(INFO, VHOST_CONFIG,
> + "free connfd = %d for device '%s'\n",
> + vsocket->connfd, path);
> + close(vsocket->connfd);
> + vhost_destroy_device(conn->vid);
> + free(conn);
> }
>
> - free(vhost_user.vsockets[i]->path);
> - free(vhost_user.vsockets[i]);
> + free(vsocket->path);
> + free(vsocket);
>
> count = --vhost_user.vsocket_cnt;
> vhost_user.vsockets[i] = vhost_user.vsockets[count];
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] vhost: fix driver unregister for client mode
2016-07-21 12:55 ` [PATCH v3] " Ilya Maximets
@ 2016-07-21 13:18 ` Yuanhan Liu
2016-07-21 22:26 ` Thomas Monjalon
0 siblings, 1 reply; 9+ messages in thread
From: Yuanhan Liu @ 2016-07-21 13:18 UTC (permalink / raw)
To: Ilya Maximets; +Cc: dev, Huawei Xie, Dyasly Sergey, Heetae Ahn, Thomas Monjalon
On Thu, Jul 21, 2016 at 03:55:36PM +0300, Ilya Maximets wrote:
> Currently while calling of 'rte_vhost_driver_unregister()' connection
> to QEMU will not be closed. This leads to inability to register driver
> again and reconnect to same virtual machine.
>
> This scenario is reproducible with OVS. While executing of the following
> command vhost port will be re-created (will be executed
> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> network will be broken and QEMU possibly will crash:
>
> ovs-vsctl set Interface vhost1 ofport_request=15
>
> Fix this by closing all established connections on driver unregister and
> removing of pending connections from reconnection list.
>
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>
> Version 3:
> * fixed leak of file descriptors by adding of
> 'close(reconn->fd)' to 'vhost_user_remove_reconnect()'
>
> Version 2:
> * style fixes.
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
--yliu
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] vhost: fix driver unregister for client mode
2016-07-21 13:18 ` Yuanhan Liu
@ 2016-07-21 22:26 ` Thomas Monjalon
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2016-07-21 22:26 UTC (permalink / raw)
To: Ilya Maximets; +Cc: Yuanhan Liu, dev, Huawei Xie, Dyasly Sergey, Heetae Ahn
2016-07-21 21:18, Yuanhan Liu:
> On Thu, Jul 21, 2016 at 03:55:36PM +0300, Ilya Maximets wrote:
> > Currently while calling of 'rte_vhost_driver_unregister()' connection
> > to QEMU will not be closed. This leads to inability to register driver
> > again and reconnect to same virtual machine.
> >
> > This scenario is reproducible with OVS. While executing of the following
> > command vhost port will be re-created (will be executed
> > 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> > network will be broken and QEMU possibly will crash:
> >
> > ovs-vsctl set Interface vhost1 ofport_request=15
> >
> > Fix this by closing all established connections on driver unregister and
> > removing of pending connections from reconnection list.
> >
> > Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
> >
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > ---
> >
> > Version 3:
> > * fixed leak of file descriptors by adding of
> > 'close(reconn->fd)' to 'vhost_user_remove_reconnect()'
> >
> > Version 2:
> > * style fixes.
>
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Applied, thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-21 22:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-20 8:32 [PATCH] vhost: fix driver unregister for client mode Ilya Maximets
2016-07-20 12:38 ` Yuanhan Liu
2016-07-21 8:24 ` Yuanhan Liu
2016-07-21 8:33 ` Ilya Maximets
2016-07-21 8:31 ` [PATCH v2] " Ilya Maximets
2016-07-21 12:57 ` Ilya Maximets
2016-07-21 12:55 ` [PATCH v3] " Ilya Maximets
2016-07-21 13:18 ` Yuanhan Liu
2016-07-21 22:26 ` Thomas Monjalon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.