From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Jianfeng Tan <jianfeng.tan@intel.com>, dev@dpdk.org
Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com,
thomas@monjalon.net
Subject: Re: [dpdk-dev] [PATCH v5] vfio: change to use generic multi-process channel
Date: Wed, 14 Mar 2018 13:27:17 +0000 [thread overview]
Message-ID: <a2fa2595-a1c2-7aea-49ca-499415828541@intel.com> (raw)
In-Reply-To: <1520175456-52990-1-git-send-email-jianfeng.tan@intel.com>
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.burakov@intel.com
>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@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
next prev parent reply other threads:[~2018-03-14 13:27 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-30 18:44 [dpdk-dev] [PATCH 0/3] generic channel for multi-process communication Jianfeng Tan
2017-11-30 18:44 ` [dpdk-dev] [PATCH 1/3] eal: add " Jianfeng Tan
2017-12-11 11:04 ` Burakov, Anatoly
2017-12-11 16:43 ` Ananyev, Konstantin
2017-11-30 18:44 ` [dpdk-dev] [PATCH 2/3] eal: add synchronous " Jianfeng Tan
2017-12-11 11:39 ` Burakov, Anatoly
2017-12-11 16:49 ` Ananyev, Konstantin
2017-11-30 18:44 ` [dpdk-dev] [PATCH 3/3] vfio: use the generic multi-process channel Jianfeng Tan
2017-12-11 12:01 ` Burakov, Anatoly
2017-12-11 9:59 ` [dpdk-dev] [PATCH 0/3] generic channel for multi-process communication Burakov, Anatoly
2017-12-12 7:34 ` Tan, Jianfeng
2017-12-12 16:18 ` Burakov, Anatoly
2018-01-11 4:07 ` [dpdk-dev] [PATCH v2 0/4] " Jianfeng Tan
2018-01-11 4:07 ` [dpdk-dev] [PATCH v2 1/4] eal: add " Jianfeng Tan
2018-01-13 12:57 ` Burakov, Anatoly
2018-01-15 19:52 ` Ananyev, Konstantin
2018-01-11 4:07 ` [dpdk-dev] [PATCH v2 2/4] eal: add and del secondary processes in the primary Jianfeng Tan
2018-01-13 13:11 ` Burakov, Anatoly
2018-01-15 21:45 ` Ananyev, Konstantin
2018-01-11 4:07 ` [dpdk-dev] [PATCH v2 3/4] eal: add synchronous multi-process communication Jianfeng Tan
2018-01-13 13:41 ` Burakov, Anatoly
2018-01-16 0:00 ` Ananyev, Konstantin
2018-01-16 8:10 ` Tan, Jianfeng
2018-01-16 11:12 ` Ananyev, Konstantin
2018-01-16 16:47 ` Tan, Jianfeng
2018-01-17 10:50 ` Ananyev, Konstantin
2018-01-17 13:09 ` Tan, Jianfeng
2018-01-17 13:15 ` Tan, Jianfeng
2018-01-17 17:20 ` Ananyev, Konstantin
2018-01-11 4:07 ` [dpdk-dev] [PATCH v2 4/4] vfio: use the generic multi-process channel Jianfeng Tan
2018-01-13 14:03 ` Burakov, Anatoly
2018-03-04 14:57 ` [dpdk-dev] [PATCH v5] vfio: change to use " Jianfeng Tan
2018-03-14 13:27 ` Burakov, Anatoly [this message]
2018-03-19 6:53 ` Tan, Jianfeng
2018-03-20 10:33 ` Burakov, Anatoly
2018-03-20 10:56 ` Burakov, Anatoly
2018-03-20 8:50 ` [dpdk-dev] [PATCH v6] " Jianfeng Tan
2018-04-05 14:26 ` Tan, Jianfeng
2018-04-05 14:39 ` Burakov, Anatoly
2018-04-12 23:27 ` Thomas Monjalon
2018-04-12 15:26 ` Burakov, Anatoly
2018-04-15 15:06 ` [dpdk-dev] [PATCH v7] " Jianfeng Tan
2018-04-15 15:10 ` Tan, Jianfeng
2018-04-17 23:04 ` Thomas Monjalon
2018-01-25 4:16 ` [dpdk-dev] [PATCH v3 0/3] generic channel for multi-process communication Jianfeng Tan
2018-01-25 4:16 ` [dpdk-dev] [PATCH v3 1/3] eal: add " Jianfeng Tan
2018-01-25 10:41 ` Thomas Monjalon
2018-01-25 11:27 ` Burakov, Anatoly
2018-01-25 11:34 ` Thomas Monjalon
2018-01-25 12:21 ` Ananyev, Konstantin
2018-01-25 4:16 ` [dpdk-dev] [PATCH v3 2/3] eal: add synchronous " Jianfeng Tan
2018-01-25 12:00 ` Burakov, Anatoly
2018-01-25 12:19 ` Burakov, Anatoly
2018-01-25 12:19 ` Ananyev, Konstantin
2018-01-25 12:25 ` Burakov, Anatoly
2018-01-25 13:00 ` Ananyev, Konstantin
2018-01-25 13:05 ` Burakov, Anatoly
2018-01-25 13:10 ` Burakov, Anatoly
2018-01-25 15:03 ` Ananyev, Konstantin
2018-01-25 16:22 ` Burakov, Anatoly
2018-01-25 17:10 ` Tan, Jianfeng
2018-01-25 18:02 ` Burakov, Anatoly
2018-01-25 12:22 ` Ananyev, Konstantin
2018-01-25 4:16 ` [dpdk-dev] [PATCH v3 3/3] vfio: use the generic multi-process channel Jianfeng Tan
2018-01-25 10:47 ` Thomas Monjalon
2018-01-25 10:52 ` Burakov, Anatoly
2018-01-25 10:57 ` Thomas Monjalon
2018-01-25 12:15 ` Burakov, Anatoly
2018-01-25 19:14 ` [dpdk-dev] [PATCH v4 0/2] generic channel for multi-process communication Jianfeng Tan
2018-01-25 19:14 ` [dpdk-dev] [PATCH v4 1/2] eal: add synchronous " Jianfeng Tan
2018-01-25 19:14 ` [dpdk-dev] [PATCH v4 2/2] vfio: use the generic multi-process channel Jianfeng Tan
2018-01-25 19:15 ` [dpdk-dev] [PATCH v4 0/2] generic channel for multi-process communication Tan, Jianfeng
2018-01-25 19:21 ` [dpdk-dev] [PATCH v5 " Jianfeng Tan
2018-01-25 19:21 ` [dpdk-dev] [PATCH v5 1/2] eal: add " Jianfeng Tan
2018-01-25 19:21 ` [dpdk-dev] [PATCH v5 2/2] eal: add synchronous " Jianfeng Tan
2018-01-25 21:23 ` [dpdk-dev] [PATCH v5 0/2] generic channel for " Thomas Monjalon
2018-01-26 3:41 ` [dpdk-dev] [PATCH v6 " Jianfeng Tan
2018-01-26 3:41 ` [dpdk-dev] [PATCH v6 1/2] eal: add " Jianfeng Tan
2018-01-26 10:25 ` Burakov, Anatoly
2018-01-29 6:37 ` Tan, Jianfeng
2018-01-29 9:37 ` Burakov, Anatoly
2018-01-26 3:41 ` [dpdk-dev] [PATCH v6 2/2] eal: add synchronous " Jianfeng Tan
2018-01-26 10:31 ` Burakov, Anatoly
2018-01-29 23:52 ` [dpdk-dev] [PATCH v6 0/2] generic channel for " Thomas Monjalon
2018-01-30 6:58 ` [dpdk-dev] [PATCH v7 " Jianfeng Tan
2018-01-30 6:58 ` [dpdk-dev] [PATCH v7 1/2] eal: add " Jianfeng Tan
2018-01-30 6:58 ` [dpdk-dev] [PATCH v7 2/2] eal: add synchronous " Jianfeng Tan
2018-01-30 14:46 ` [dpdk-dev] [PATCH v7 0/2] generic channel for " Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2fa2595-a1c2-7aea-49ca-499415828541@intel.com \
--to=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jianfeng.tan@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).