From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id C11E42BE5 for ; Sat, 13 Jan 2018 14:12:02 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jan 2018 05:12:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,353,1511856000"; d="scan'208";a="10012990" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.9.53]) ([10.252.9.53]) by fmsmga002.fm.intel.com with ESMTP; 13 Jan 2018 05:12:00 -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> <1515643654-129489-1-git-send-email-jianfeng.tan@intel.com> <1515643654-129489-3-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: <86c760cb-d679-4b19-df35-7b21387586d8@intel.com> Date: Sat, 13 Jan 2018 13:11:59 +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: <1515643654-129489-3-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 v2 2/4] eal: add and del secondary processes in the primary 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: Sat, 13 Jan 2018 13:12:03 -0000 On 11-Jan-18 4:07 AM, Jianfeng Tan wrote: > By the multi-process channel, we add an mp action named "proc". > > As a secondary process starts, it sends a "proc add" message to > the primary. > > As the primary finds a failure in sending message to a specific > secondary process, that secondary process is treated as exited; > and we remove it from the secondary array by sending a "proc del" > message to the primary itself. > > Test: > 1. Start the primary and the secondary process > $ (testpmd) -c 0x3 -n 4 -- -i > $ (helloworld) -c 0xc -n 4 --proc-type=auto -- > > 2. Check the log of testpmd: > ... > EAL: bind to /var/run/.rte_unix > ... > EAL: add secondary: /var/run/.testpmd_unix_(xxx) > ... > > 3. Check the log of helloworld: > ... > EAL: bind to /var/run/.testpmd_unix_xxx > EAL: bind to /var/run/.testpmd_unix_c_xxx > ... it says "unix" all over the place, but that's an internal implementation detail. "mp_socket" or similar should do, no? > > Signed-off-by: Jianfeng Tan > --- > lib/librte_eal/common/eal_common_proc.c | 88 ++++++++++++++++++++++++++++++++- > 1 file changed, 86 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c > index d700e9e..70519cc 100644 > --- a/lib/librte_eal/common/eal_common_proc.c > +++ b/lib/librte_eal/common/eal_common_proc.c > @@ -54,6 +54,13 @@ struct mp_msghdr { > char params[0]; > } __rte_packed; > > +struct proc_request { > +#define MP_PROC_ADD 0 > +#define MP_PROC_DEL 1 > + int type; > + char path[MAX_UNIX_PATH_LEN]; > +}; > + > int > rte_eal_primary_proc_alive(const char *config_file_path) > { > @@ -214,6 +221,58 @@ mp_handle(void *arg __rte_unused) > return NULL; > } > > +static int > +add_sec_proc(const char *path) > +{ > + int i; > + > + for (i = 0; i < MAX_SECONDARY_PROCS; ++i) > + if (mp_sec_sockets[i] == NULL) > + break; > + if (i < MAX_SECONDARY_PROCS) > + mp_sec_sockets[i] = strdup(path); > + > + return i < MAX_SECONDARY_PROCS; > +} While it's equivalent, the intent behind this isn't clear, it's needlessly complicating the more common idiom of for (i = 0; i < MAX; i++) {} if (i == MAX) return error; do_something; return success; > + > +static int > +del_sec_proc(const char *path) > +{ > + int i; > + > + for (i = 0; i < MAX_SECONDARY_PROCS; ++i) { > + if (!strcmp(mp_sec_sockets[i], path)) { > + free(mp_sec_sockets[i]); > + mp_sec_sockets[i] = NULL; > + break; > + } > + } > + > + return i < MAX_SECONDARY_PROCS; > +} Same as above - maybe rewrite it as a more commonly used idiom. Also, you probably want to use strncmp(), and check for NULL pointers, IIRC strncmp(NULL) is undefined behavior. > + > +static int > +mp_primary_proc(const void *params, > + int len __rte_unused, > + int fds[] __rte_unused, > + int fds_num __rte_unused) > +{ > + const struct proc_request *r = (const struct proc_request *)params; > + > + switch (r->type) { > + case MP_PROC_ADD: > + RTE_LOG(INFO, EAL, "add secondary: %s\n", r->path); > + return add_sec_proc(r->path); > + case MP_PROC_DEL: > + RTE_LOG(INFO, EAL, "del secondary: %s\n", r->path); > + return del_sec_proc(r->path); > + default: > + RTE_LOG(ERR, EAL, "invalid type: %d\n", r->type); > + } > + > + return -1; > +} > + > static inline const char * > get_unix_path(int is_server) > { > @@ -267,6 +326,22 @@ rte_eal_mp_channel_init(void) > if (mp_fd < 0) > return -1; > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > + if (rte_eal_mp_action_register("proc", mp_primary_proc) < 0) { > + RTE_LOG(ERR, EAL, "failed to register handler\n"); > + goto error; > + } > + } else { > + struct proc_request r; > + > + r.type = MP_PROC_ADD; > + snprintf(r.path, MAX_UNIX_PATH_LEN, "%s", get_unix_path(1)); Nitpicking, but maybe just send PID instead of the whole path? Primary/secondary share their prefix and most of their socket path anyway, so the real difference is the PID. This would also eliminate the need for using strings in many places. > + if (rte_eal_mp_sendmsg("proc", &r, sizeof(r), NULL, 0) < 0) { > + RTE_LOG(ERR, EAL, "failed to add into primary\n"); > + goto error; > + } > + } > + > if (pthread_create(&tid, NULL, mp_handle, NULL) < 0) { > RTE_LOG(ERR, EAL, "failed to create mp handle thead: %s\n", > strerror(errno)); > @@ -354,10 +429,19 @@ send_msg(int fd, const char *dst_path, struct mp_msghdr *msg, int fds[]) > if (ret < 0) { > RTE_LOG(ERR, EAL, "failed to send msg: %s\n", strerror(errno)); > > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > + struct proc_request r; > + > RTE_LOG(ERR, EAL, "secondary process (%s) exited\n", > dst_path); > - else if (!rte_eal_primary_proc_alive(NULL)) > + r.type = MP_PROC_DEL; > + snprintf(r.path, MAX_UNIX_PATH_LEN, "%s", dst_path); > + if (rte_eal_mp_sendmsg("proc", &r, > + sizeof(r), NULL, 0) < 0) > + RTE_LOG(ERR, EAL, > + "failed to del secondary %s\n", > + dst_path); > + } else if (!rte_eal_primary_proc_alive(NULL)) > RTE_LOG(ERR, EAL, "primary process exited\n"); > > return 0; > -- Thanks, Anatoly