[dpdk-dev] vhost: added user callbacks for socket open/close
Checks
Commit Message
When user receives destroy_device signal, he does not know *why* that
event happened. He does not differ between socket shutdown and virtio
processing pause. User could completely delete device during transition
from BIOS to kernel, causing freeze or possibly kernel panic. Instead
of changing new_device/destroy_device callbacks and breaking the ABI,
a set of new functions new_connection/destroy_connection has been added.
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
lib/librte_vhost/rte_vhost.h | 5 ++++-
lib/librte_vhost/socket.c | 23 +++++++++++++++++++----
2 files changed, 23 insertions(+), 5 deletions(-)
Comments
Hi Dariusz,
On Mon, Aug 21, 2017 at 11:34:42AM +0200, Dariusz Stojaczyk wrote:
>When user receives destroy_device signal, he does not know *why* that
>event happened. He does not differ between socket shutdown and virtio
>processing pause. User could completely delete device during transition
>from BIOS to kernel, causing freeze or possibly kernel panic. Instead
>of changing new_device/destroy_device callbacks and breaking the ABI,
>a set of new functions new_connection/destroy_connection has been added.
>
>Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
>---
> lib/librte_vhost/rte_vhost.h | 5 ++++-
> lib/librte_vhost/socket.c | 23 +++++++++++++++++++----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
>diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>index 8c974eb..8f86167 100644
>--- a/lib/librte_vhost/rte_vhost.h
>+++ b/lib/librte_vhost/rte_vhost.h
>@@ -107,7 +107,10 @@ struct vhost_device_ops {
> */
> int (*features_changed)(int vid, uint64_t features);
>
>- void *reserved[4]; /**< Reserved for future extension */
>+ int (*new_connection)(int vid); /**< Connect to socket. */
>+ void (*destroy_connection)(int vid); /**< Disconnect from socket */
I'm a little uncertain but my gut feeling is that in this context a connection is
something between two sockets, not between devices. I would probably
add these callbacks to struct vhost_user_socket. This is also where we
keep the list of connections.
regards,
Jens
Hi Jens,
> I'm a little uncertain but my gut feeling is that in this context a connection is
> something between two sockets, not between devices.
What do you mean?
This is a unix domain socket connection. DPDK can create the socket, then the client may connect to it via connect(2).
> I would probably add
> these callbacks to struct vhost_user_socket. This is also where we keep the
> list of connections.
I get your point. However, it's vhost_device_ops struct that's being set by the user via rte_vhost_driver_callback_register(). The new_connection callback is there just to mark the device as *in use, can't be deleted*. It doesn't transport any connection data.
Regards,
D.
On Tue, Aug 22, 2017 at 09:55:19AM +0000, Stojaczyk, DariuszX wrote:
>Hi Jens,
>
>> I'm a little uncertain but my gut feeling is that in this context a connection is
>> something between two sockets, not between devices.
>
>What do you mean?
>This is a unix domain socket connection. DPDK can create the socket, then the client may connect to it via connect(2).
yes, I get that.
>
>> I would probably add
>> these callbacks to struct vhost_user_socket. This is also where we keep the
>> list of connections.
>
>I get your point. However, it's vhost_device_ops struct that's being set by the user via rte_vhost_driver_callback_register(). The new_connection callback is there just to mark the device as *in use, can't be deleted*. It doesn't transport any connection data.
You're right, I overlooked that it needs to be set by the user. In
this case your patch is the smallest possible change and looks good to
me.
Do we need a documentation change for this?
regards,
Jens
On Tue, Aug 22, 2017 at 01:58:44PM +0200, Jens Freimann wrote:
>On Tue, Aug 22, 2017 at 09:55:19AM +0000, Stojaczyk, DariuszX wrote:
>Do we need a documentation change for this?
To answer my own question, I think doc/guides/prog_guide/vhost_lib.rst
needs an update.
regards,
Jens
>
>
>regards,
>Jens
@@ -107,7 +107,10 @@ struct vhost_device_ops {
*/
int (*features_changed)(int vid, uint64_t features);
- void *reserved[4]; /**< Reserved for future extension */
+ int (*new_connection)(int vid); /**< Connect to socket. */
+ void (*destroy_connection)(int vid); /**< Disconnect from socket */
+
+ void *reserved[2]; /**< Reserved for future extension */
};
/**
@@ -230,24 +230,36 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
+ if (vsocket->notify_ops->new_connection) {
+ ret = vsocket->notify_ops->new_connection(vid);
+ if (ret < 0) {
+ RTE_LOG(ERR, VHOST_CONFIG,
+ "failed to add vhost user connection with fd %d\n",
+ fd);
+ goto err;
+ }
+ }
+
conn->connfd = fd;
conn->vsocket = vsocket;
conn->vid = vid;
ret = fdset_add(&vhost_user.fdset, fd, vhost_user_read_cb,
NULL, conn);
if (ret < 0) {
- conn->connfd = -1;
- free(conn);
- close(fd);
RTE_LOG(ERR, VHOST_CONFIG,
"failed to add fd %d into vhost server fdset\n",
fd);
- return;
+ goto err;
}
pthread_mutex_lock(&vsocket->conn_mutex);
TAILQ_INSERT_TAIL(&vsocket->conn_list, conn, next);
pthread_mutex_unlock(&vsocket->conn_mutex);
+ return;
+
+err:
+ free(conn);
+ close(fd);
}
/* call back when there is new vhost-user connection from client */
@@ -277,6 +289,9 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
*remove = 1;
vhost_destroy_device(conn->vid);
+ if (vsocket->notify_ops->destroy_connection)
+ vsocket->notify_ops->destroy_connection(conn->vid);
+
pthread_mutex_lock(&vsocket->conn_mutex);
TAILQ_REMOVE(&vsocket->conn_list, conn, next);
pthread_mutex_unlock(&vsocket->conn_mutex);