Re: [PATCH v2 2/4] eal: add and del secondary processes in the primary

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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