On 04-Mar-18 2:57 PM, Jianfeng Tan wrote:
Previously, vfio uses its own private channel for the secondary
process to get container fd and group fd from the primary process.
This patch changes to use the generic mp channel.
Test:
1. Bind two NICs to vfio-pci.
2. Start the primary and secondary process.
$ (symmetric_mp) -c 2 -- -p 3 --num-procs=2 --proc-id=0
$ (symmetric_mp) -c 4 --proc-type=auto -- -p 3 \
--num-procs=2 --proc-id=1
Cc: anatoly.bura...@intel.com
Signed-off-by: Jianfeng Tan <jianfeng....@intel.com>
---
<...>
- ret = vfio_mp_sync_receive_request(socket_fd);
- switch (ret) {
- case SOCKET_NO_FD:
- close(socket_fd);
- return 0;
- case SOCKET_OK:
- vfio_group_fd = vfio_mp_sync_receive_fd(socket_fd);
- /* if we got the fd, store it and return it */
- if (vfio_group_fd > 0) {
- close(socket_fd);
- cur_grp->group_no = iommu_group_no;
- cur_grp->fd = vfio_group_fd;
- vfio_cfg.vfio_active_groups++;
- return vfio_group_fd;
- }
- /* fall-through on error */
- default:
- RTE_LOG(ERR, EAL, " cannot get container fd!\n");
- close(socket_fd);
- return -1;
+ p->req = SOCKET_REQ_GROUP;
+ p->group_no = iommu_group_no;
+ strcpy(mp_req.name, "vfio");
"vfio" should probably be a #define. Also, i think the identifier is too
short. Maybe "eal_vfio_mp_sync" or at least "eal_vfio" would be better?
+ mp_req.len_param = sizeof(*p);
+ mp_req.num_fds = 0;
+
+ vfio_group_fd = -1;
+ if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0 &&
+ mp_reply.nb_received == 1) {
+ mp_rep = &mp_reply.msgs[0];
+ p = (struct vfio_mp_param *)mp_rep->param;
+ if (p->result == SOCKET_OK && mp_rep->num_fds == 1) {
+ cur_grp->group_no = iommu_group_no;
+ vfio_group_fd = mp_rep->fds[0];
+ cur_grp->fd = vfio_group_fd;
+ vfio_cfg.vfio_active_groups++;
}
+ free(mp_reply.msgs);
}
- return -1;
+
+ if (vfio_group_fd < 0)
+ RTE_LOG(ERR, EAL, " cannot request group fd\n");
+ return vfio_group_fd;
p->result can be SOCKET_NO_FD, in which case returned value should be
zero. I think this is missing from this code. There probably should be
an "else if (p->result == SOCKET_NO_FD)" clause that sets return value to 0.
You should be able to test this by trying to set up a device for VFIO
that isn't bound to VFIO driver, in a secondary process.
}
@@ -200,7 +185,10 @@ int
rte_vfio_clear_group(int vfio_group_fd)
{
int i;
- int socket_fd, ret;
+ struct rte_mp_msg mp_req, *mp_rep;
+ struct rte_mp_reply mp_reply;
+ struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+ struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
if (internal_config.process_type == RTE_PROC_PRIMARY) {
@@ -214,43 +202,24 @@ rte_vfio_clear_group(int vfio_group_fd)
return 0;
}
- /* This is just for SECONDARY processes */
- socket_fd = vfio_mp_sync_connect_to_primary();
-
- if (socket_fd < 0) {
- RTE_LOG(ERR, EAL, " cannot connect to primary process!\n");
- return -1;
- }
-
- if (vfio_mp_sync_send_request(socket_fd, SOCKET_CLR_GROUP) < 0) {
- RTE_LOG(ERR, EAL, " cannot request container fd!\n");
- close(socket_fd);
- return -1;
- }
+ p->req = SOCKET_CLR_GROUP;
+ p->group_no = vfio_group_fd;
+ strcpy(mp_req.name, "vfio");
Same here, please use a #define here.
+ mp_req.len_param = sizeof(*p);
+ mp_req.num_fds = 0;
- if (vfio_mp_sync_send_request(socket_fd, vfio_group_fd) < 0) {
- RTE_LOG(ERR, EAL, " cannot send group fd!\n");
- close(socket_fd);
- return -1;
+ if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0 &&
+ mp_reply.nb_received == 1) {
+ mp_rep = &mp_reply.msgs[0];
+ p = (struct vfio_mp_param *)mp_rep->param;
+ if (p->result == SOCKET_OK) {
+ free(mp_reply.msgs);
+ return 0;
+ }
+ free(mp_reply.msgs);
}
- ret = vfio_mp_sync_receive_request(socket_fd);
- switch (ret) {
- case SOCKET_NO_FD:
- RTE_LOG(ERR, EAL, " BAD VFIO group fd!\n");
- close(socket_fd);
- break;
- case SOCKET_OK:
- close(socket_fd);
- return 0;
- case SOCKET_ERR:
- RTE_LOG(ERR, EAL, " Socket error\n");
- close(socket_fd);
- break;
- default:
- RTE_LOG(ERR, EAL, " UNKNOWN reply, %d\n", ret);
- close(socket_fd);
- }
+ RTE_LOG(ERR, EAL, " BAD VFIO group fd!\n");
Old error messages distinguished between "bad VFIO group fd" and other
errors. You should probably only output this message of response was
SOCKET_NO_FD, and output another message in case of other errors.
return -1;
}
@@ -561,6 +530,11 @@ int
vfio_get_container_fd(void)
{
<...>
- vfio_container_fd = vfio_mp_sync_receive_fd(socket_fd);
- if (vfio_container_fd < 0) {
- RTE_LOG(ERR, EAL, " cannot get container fd!\n");
- close(socket_fd);
- return -1;
+ }
+ /*
+ * if we're in a secondary process, request container fd from the
+ * primary process via mp channel
+ */
+ p->req = SOCKET_REQ_CONTAINER;
+ strcpy(mp_req.name, "vfio");
Same here, please use #define here.
+ mp_req.len_param = sizeof(*p);
+ mp_req.num_fds = 0;
+
+ vfio_container_fd = -1;
+ if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0 &&
+ mp_reply.nb_received == 1) {
+ mp_rep = &mp_reply.msgs[0];
+ p = (struct vfio_mp_param *)mp_rep->param;
+ if (p->result == SOCKET_OK && mp_rep->num_fds == 1) {
+ free(mp_reply.msgs);
+ return mp_rep->fds[0];
}
- close(socket_fd);
- return vfio_container_fd;
+ free(mp_reply.msgs);
<...>
- /* set up a socket */
- socket_fd = socket(AF_UNIX, SOCK_SEQPACKET, 0);
- if (socket_fd < 0) {
- RTE_LOG(ERR, EAL, "Failed to create socket!\n");
- return -1;
- }
-
- get_socket_path(addr.sun_path, sizeof(addr.sun_path));
- addr.sun_family = AF_UNIX;
-
- sockaddr_len = sizeof(struct sockaddr_un);
-
- unlink(addr.sun_path);
-
- ret = bind(socket_fd, (struct sockaddr *) &addr, sockaddr_len);
- if (ret) {
- RTE_LOG(ERR, EAL, "Failed to bind socket: %s!\n",
strerror(errno));
- close(socket_fd);
+ break;
+ case SOCKET_CLR_GROUP:
+ r->req = SOCKET_CLR_GROUP;
+ r->group_no = m->group_no;
+ if (rte_vfio_clear_group(m->group_no) < 0)
+ r->result = SOCKET_NO_FD;
+ else
+ r->result = SOCKET_OK;
+ break;
+ case SOCKET_REQ_CONTAINER:
+ r->req = SOCKET_REQ_CONTAINER;
+ fd = vfio_get_container_fd();
+ if (fd < 0)
+ r->result = SOCKET_ERR;
+ else {
+ r->result = SOCKET_OK;
+ num = 1;
+ }
+ break;
+ default:
+ RTE_LOG(ERR, EAL, "vfio received invalid message!\n");
return -1;
}
- ret = listen(socket_fd, 50);
- if (ret) {
- RTE_LOG(ERR, EAL, "Failed to listen: %s!\n", strerror(errno));
- close(socket_fd);
- return -1;
+ if (num == 1) {
+ reply.num_fds = 1;
+ reply.fds[0] = fd;
}
You're not saving any lines of code with this, but you are sacrificing
code clarity :) I think this should be done inline, e.g. in "else"
clause of SOCKET_REQ_CONTAINER and SOCKET_REQ_GROUP.
+ strcpy(reply.name, "vfio");
Same here, please use #define.
+ reply.len_param = sizeof(*r);
- /* save the socket in local configuration */
- mp_socket_fd = socket_fd;
-
- return 0;
+ ret = rte_mp_reply(&reply, peer);
+ if (m->req == SOCKET_REQ_CONTAINER && num == 1)
Why not just "fd >= 0"? No need for num variable then.
+ close(fd);
+ return ret;
}
-/*
- * set up a local socket and tell it to listen for incoming connections
- */
int
vfio_mp_sync_setup(void)
{
- int ret;
- char thread_name[RTE_MAX_THREAD_NAME_LEN];
-
- if (vfio_mp_sync_socket_setup() < 0) {
- RTE_LOG(ERR, EAL, "Failed to set up local socket!\n");
- return -1;
- }
-
- ret = pthread_create(&socket_thread, NULL,
- vfio_mp_sync_thread, NULL);
- if (ret) {
- RTE_LOG(ERR, EAL,
- "Failed to create thread for communication with secondary
processes!\n");
- close(mp_socket_fd);
- return -1;
- }
-
- /* Set thread_name for aid in debugging. */
- snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "vfio-sync");
- ret = rte_thread_setname(socket_thread, thread_name);
- if (ret)
- RTE_LOG(DEBUG, EAL,
- "Failed to set thread name for secondary processes!\n");
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ return rte_mp_action_register("vfio", vfio_mp_primary);
Same here, please use #define.
return 0;
}
Thanks for doing this patch!
--
Thanks,
Anatoly
