[dpdk-dev] [PATCH v5] vfio: change to use generic multi-process channel

[Burakov, Anatoly]

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 at intel.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
> ---

<...>

> -		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