From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id E97894CBD for ; Wed, 14 Mar 2018 14:27:23 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2018 06:27:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,470,1515484800"; d="scan'208";a="25630630" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga006.jf.intel.com with ESMTP; 14 Mar 2018 06:27:18 -0700 To: Jianfeng Tan , dev@dpdk.org Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com, thomas@monjalon.net References: <1515643654-129489-5-git-send-email-jianfeng.tan@intel.com> <1520175456-52990-1-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Wed, 14 Mar 2018 13:27:17 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1520175456-52990-1-git-send-email-jianfeng.tan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5] vfio: change to use generic multi-process channel X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Mar 2018 13:27:24 -0000 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 > --- <...> > - 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