From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 623A62C2E for ; Thu, 25 Jan 2018 12:27:39 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2018 03:27:38 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,411,1511856000"; d="scan'208";a="26249394" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.145]) ([10.237.220.145]) by orsmga001.jf.intel.com with ESMTP; 25 Jan 2018 03:27:36 -0800 To: Jianfeng Tan , dev@dpdk.org Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com, thomas@monjalon.net References: <1512067450-59203-1-git-send-email-jianfeng.tan@intel.com> <1516853783-108023-1-git-send-email-jianfeng.tan@intel.com> <1516853783-108023-2-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: <6047a7cb-89a9-2969-3872-bb22b0d919d2@intel.com> Date: Thu, 25 Jan 2018 11:27:35 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <1516853783-108023-2-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 v3 1/3] eal: add channel for multi-process communication 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: Thu, 25 Jan 2018 11:27:39 -0000 Overall on this patch: Reviewed-by: Anatoly Burakov There are a few nitpicks below in comments. Also, as a general note, i would prefer for sendmsg API's to return 0 on success and -1 on failure, as number of sent messages is not only meaningless to the user (since there's no way to tell if the value returned is the value we expected), but also makes the API unintuitive and prone to usage errors when using common "if (sendmsg()) {// error}" idiom. However, i'm fine with leaving it as is, if everyone else is. It's an experimental API, so we can change it later if need be. On 25-Jan-18 4:16 AM, Jianfeng Tan wrote: > Previouly, there are three channels for multi-process > (i.e., primary/secondary) communication. > 1. Config-file based channel, in which, the primary process writes > info into a pre-defined config file, and the secondary process > reads the info out. > 2. vfio submodule has its own channel based on unix socket for the > secondary process to get container fd and group fd from the > primary process. > 3. pdump submodule also has its own channel based on unix socket for > packet dump. > > It'd be good to have a generic communication channel for multi-process > communication to accomodate the requirements including: > a. Secondary wants to send info to primary, for example, secondary > would like to send request (about some specific vdev to primary). > b. Sending info at any time, instead of just initialization time. > c. Share FDs with the other side, for vdev like vhost, related FDs > (memory region, kick) should be shared. > d. A send message request needs the other side to response immediately. > > This patch proposes to create a communication channel, based on datagram > unix socket, for above requirements. Each process will block on a unix > socket waiting for messages from the peers. > > Three new APIs are added: > > 1. rte_eal_mp_action_register() is used to register an action, > indexed by a string, when a component at receiver side would like > to response the messages from the peer processe. > 2. rte_eal_mp_action_unregister() is used to unregister the action > if the calling component does not want to response the messages. > 3. rte_eal_mp_sendmsg() is used to send a message, and returns > immediately. If there are n secondary processes, the primary > process will send n messages. > > Suggested-by: Konstantin Ananyev > Signed-off-by: Jianfeng Tan > --- > lib/librte_eal/common/eal_common_proc.c | 390 +++++++++++++++++++++++++++++++- > lib/librte_eal/common/eal_filesystem.h | 17 ++ > lib/librte_eal/common/eal_private.h | 10 + > lib/librte_eal/common/include/rte_eal.h | 75 ++++++ > lib/librte_eal/linuxapp/eal/eal.c | 8 + > lib/librte_eal/rte_eal_version.map | 3 + > 6 files changed, 502 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c > index 40fa982..baeb7d1 100644 > --- a/lib/librte_eal/common/eal_common_proc.c > +++ b/lib/librte_eal/common/eal_common_proc.c > @@ -2,14 +2,48 @@ > * Copyright(c) 2016 Intel Corporation Nitpicking - making substantial changes to this files should probably update copyright year (2016-2018?). > */ > > -#include > +#include > +#include > #include > +#include > +#include > +#include > +#include > +#include > #include > +#include > +#include > +#include > +#include > +#include > + > +int > +rte_eal_mp_channel_init(void) > +{ > + char thread_name[RTE_MAX_THREAD_NAME_LEN]; > + char *path; > + pthread_t tid; > + > + snprintf(mp_filter, PATH_MAX, ".%s_unix_*", > + internal_config.hugefile_prefix); > + > + path = strdup(eal_mp_socket_path()); > + snprintf(mp_dir_path, PATH_MAX, "%s", dirname(path)); > + free(path); > + > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) > + unlink_sockets(); > + > + if (open_socket_fd() < 0) > + return -1; > + > + snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "rte_mp_handle"); > + > + if (pthread_create(&tid, NULL, mp_handle, NULL) == 0) { > + /* try best to set thread name */ > + rte_thread_setname(tid, thread_name); > + return 0; > + } > + > + RTE_LOG(ERR, EAL, "failed to create mp thead: %s\n", strerror(errno)); > + close(mp_fd); > + mp_fd = -1; > + return -1; Nitpicking: looks weird, usually early exit is for failures, not success. Maybe move the error part under (pthread_create() != 0). > +} > + > +static int > +send_msg(const char *dst_path, struct rte_mp_msg *msg) > +{ > + int snd; > + struct iovec iov; > + struct msghdr msgh; > + struct cmsghdr *cmsg; > + struct sockaddr_un dst; > + int fd_size = msg->num_fds * sizeof(int); > + char control[CMSG_SPACE(fd_size)]; > + > + memset(&dst, 0, sizeof(dst)); -- Thanks, Anatoly