[dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend

Ilya Maximets i.maximets at samsung.com
Wed Mar 13 17:09:25 CET 2019


On 13.03.2019 18:55, Maxime Coquelin wrote:
> External backends may have specific requests to handle, and so
> we don't want the vhost-user lib to handle these requests as
> errors.
> 
> This patch also changes the experimental API by introducing
> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
> can report an error if a message is handled neither by
> the vhost-user library nor by the external backend.
> 
> The logic changes a bit so that if the callback returns
> with ERR, OK or REPLY, it is considered the message
> is handled by the external backend so it won't be
> handled by the vhost-user library.
> It is still possible for an external backend to listen
> to requests that have to be handled by the vhost-user
> library like SET_MEM_TABLE, but the callback have to
> return NOT_HANDLED in that case.
> 
> Vhost-crypto backend is ialso adapted to this API change.
> 
> Suggested-by: Ilya Maximets <i.maximets at samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
> Tested-by: Darek Stojaczyk <dariusz.stojaczyk at intel.com>
> ---
>  lib/librte_vhost/rte_vhost.h    | 33 ++++---------
>  lib/librte_vhost/vhost_crypto.c | 10 +++-
>  lib/librte_vhost/vhost_user.c   | 82 +++++++++++++++++++++------------
>  3 files changed, 69 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index c9c392975..8ab4ff299 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
>  	RTE_VHOST_MSG_RESULT_OK =  0,
>  	/* Message handling successful and reply prepared */
>  	RTE_VHOST_MSG_RESULT_REPLY =  1,
> +	/* Message not handled */
> +	RTE_VHOST_MSG_RESULT_NOT_HANDLED,
>  };
>  
>  /**
> @@ -131,37 +133,20 @@ enum rte_vhost_msg_result {

/**
 * Function prototype for the vhost backend to handler specific vhost user
 * messages prior to the master message handling

Now you have definition of pre handler for a common handler type.

It should be updated. Probably just cropped a bit (and s/handler/handle/):

/**
 * Function prototype for the vhost backend to handle specific vhost user
 * messages.

>   *  vhost device id
>   * @param msg
>   *  Message pointer.
> - * @param skip_master
> - *  If the handler requires skipping the master message handling, this variable
> - *  shall be written 1, otherwise 0.
>   * @return
> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - *  VH_RESULT_ERR on failure
> + *  RTE_VHOST_MSG_RESULT_OK on success,
> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>   */
> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
> -		void *msg, uint32_t *skip_master);
> -
> -/**
> - * Function prototype for the vhost backend to handler specific vhost user
> - * messages after the master message handling is done
> - *
> - * @param vid
> - *  vhost device id
> - * @param msg
> - *  Message pointer.
> - * @return
> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - *  VH_RESULT_ERR on failure
> - */
> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
> -		void *msg);
> +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg);
>  
>  /**
>   * Optional vhost user message handlers.
>   */
>  struct rte_vhost_user_extern_ops {
> -	rte_vhost_msg_pre_handle pre_msg_handle;
> -	rte_vhost_msg_post_handle post_msg_handle;

What about some comments here?

        /* Called prior to the master message handling. */
> +	rte_vhost_msg_handle pre_msg_handle;

        /* Called after the master message handling. */
> +	rte_vhost_msg_handle post_msg_handle;
>  };
>  
>  /**
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index 0f437c4a1..9b4b850e8 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg)
>  		return RTE_VHOST_MSG_RESULT_ERR;
>  	}
>  
> -	if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) {
> +	switch (vmsg->request.master) {
> +	case VHOST_USER_CRYPTO_CREATE_SESS:
>  		vhost_crypto_create_sess(vcrypto,
>  				&vmsg->payload.crypto_session);
>  		vmsg->fd_num = 0;
>  		ret = RTE_VHOST_MSG_RESULT_REPLY;
> -	} else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) {
> +		break;
> +	case VHOST_USER_CRYPTO_CLOSE_SESS:
>  		if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64))
>  			ret = RTE_VHOST_MSG_RESULT_ERR;
> +		break;
> +	default:
> +		ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED;
> +		break;
>  	}
>  
>  	return ret;
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 555d09ad9..39756fce7 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1910,7 +1910,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	int did = -1;
>  	int ret;
>  	int unlock_required = 0;
> -	uint32_t skip_master = 0;
> +	bool handled;
>  	int request;
>  
>  	dev = get_device(vid);
> @@ -1928,27 +1928,30 @@ vhost_user_msg_handler(int vid, int fd)
>  	}
>  
>  	ret = read_vhost_message(fd, &msg);
> -	if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
> +	if (ret <= 0) {
>  		if (ret < 0)
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"vhost read message failed\n");
> -		else if (ret == 0)
> +		else
>  			RTE_LOG(INFO, VHOST_CONFIG,
>  				"vhost peer closed\n");
> -		else
> -			RTE_LOG(ERR, VHOST_CONFIG,
> -				"vhost read incorrect message\n");
>  
>  		return -1;
>  	}
>  
>  	ret = 0;
> -	if (msg.request.master != VHOST_USER_IOTLB_MSG)
> -		RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> -			vhost_message_str[msg.request.master]);
> -	else
> -		RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> -			vhost_message_str[msg.request.master]);
> +	request = msg.request.master;
> +	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX &&
> +			vhost_message_str[request]) {
> +		if (request != VHOST_USER_IOTLB_MSG)
> +			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);
> +		else
> +			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);
> +	} else {
> +		RTE_LOG(DEBUG, VHOST_CONFIG, "External request %d\n", request);
> +	}
>  
>  	ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
>  	if (ret < 0) {
> @@ -1964,7 +1967,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	 * inactive, so it is safe. Otherwise taking the access_lock
>  	 * would cause a dead lock.
>  	 */
> -	switch (msg.request.master) {
> +	switch (request) {
>  	case VHOST_USER_SET_FEATURES:
>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>  	case VHOST_USER_SET_OWNER:
> @@ -1989,19 +1992,24 @@ vhost_user_msg_handler(int vid, int fd)
>  
>  	}
>  
> +	handled = false;
>  	if (dev->extern_ops.pre_msg_handle) {
>  		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
> -				(void *)&msg, &skip_master);
> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
> -			goto skip_to_reply;
> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> +				(void *)&msg);
> +		switch (ret) {
> +		case RTE_VHOST_MSG_RESULT_REPLY:
>  			send_vhost_reply(fd, &msg);
> -
> -		if (skip_master)
> +			/* Fall-through */
> +		case RTE_VHOST_MSG_RESULT_ERR:
> +		case RTE_VHOST_MSG_RESULT_OK:
> +			handled = true;
>  			goto skip_to_post_handle;
> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> +		default:
> +			break;
> +		}
>  	}
>  
> -	request = msg.request.master;
>  	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>  		if (!vhost_message_handlers[request])
>  			goto skip_to_post_handle;
> @@ -2012,40 +2020,54 @@ vhost_user_msg_handler(int vid, int fd)
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"Processing %s failed.\n",
>  				vhost_message_str[request]);
> +			handled = true;
>  			break;
>  		case RTE_VHOST_MSG_RESULT_OK:
>  			RTE_LOG(DEBUG, VHOST_CONFIG,
>  				"Processing %s succeeded.\n",
>  				vhost_message_str[request]);
> +			handled = true;
>  			break;
>  		case RTE_VHOST_MSG_RESULT_REPLY:
>  			RTE_LOG(DEBUG, VHOST_CONFIG,
>  				"Processing %s succeeded and needs reply.\n",
>  				vhost_message_str[request]);
>  			send_vhost_reply(fd, &msg);
> +			handled = true;
> +			break;
> +		default:
>  			break;
>  		}
> -	} else {
> -		RTE_LOG(ERR, VHOST_CONFIG,
> -			"Requested invalid message type %d.\n", request);
> -		ret = RTE_VHOST_MSG_RESULT_ERR;
>  	}
>  
>  skip_to_post_handle:
>  	if (ret != RTE_VHOST_MSG_RESULT_ERR &&
>  			dev->extern_ops.post_msg_handle) {
> -		ret = (*dev->extern_ops.post_msg_handle)(
> -				dev->vid, (void *)&msg);
> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
> -			goto skip_to_reply;
> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> +		ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
> +				(void *)&msg);
> +		switch (ret) {
> +		case RTE_VHOST_MSG_RESULT_REPLY:
>  			send_vhost_reply(fd, &msg);
> +			/* Fall-through */
> +		case RTE_VHOST_MSG_RESULT_ERR:
> +		case RTE_VHOST_MSG_RESULT_OK:
> +			handled = true;
> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> +		default:
> +			break;
> +		}
>  	}
>  
> -skip_to_reply:
>  	if (unlock_required)
>  		vhost_user_unlock_all_queue_pairs(dev);
>  
> +	/* If message was not handled at this stage, treat it as an error */
> +	if (!handled) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"vhost message (req: %d) was not handled.\n", request);
> +		ret = RTE_VHOST_MSG_RESULT_ERR;
> +	}
> +
>  	/*
>  	 * If the request required a reply that was already sent,
>  	 * this optional reply-ack won't be sent as the
> 


More information about the dev mailing list