From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <anatoly.burakov@intel.com>
Received: from mga12.intel.com (mga12.intel.com [192.55.52.136])
by dpdk.org (Postfix) with ESMTP id C11E42BE5
for <dev@dpdk.org>; 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 <jianfeng.tan@intel.com>, 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" <anatoly.burakov@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <jianfeng.tan@intel.com>
> ---
> 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