Re: [dpdk-dev] [PATCH] [v1]vhost: support inflight share memory protocol feature

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Jin,

On 6/26/19 6:38 PM, JinYu wrote:
> This patch introduces two new messages VHOST_USER_GET_INFLIGHT_FD
> and VHOST_USER_SET_INFLIGHT_FD to support transferring a shared
> buffer between qemu and backend.
> 
> Firstly, qemu uses VHOST_USER_GET_INFLIGHT_FD to get the
> shared buffer from backend. Then qemu should send it back
> through VHOST_USER_SET_INFLIGHT_FD each time we start vhost-user.
> 
> This shared buffer is used to process inflight I/O when backend
> reconnect.
> 
> Signed-off-by: LinLi <lilin24@baidu.com>
> Signed-off-by: XunNi <nixun@baidu.com>
> Signed-off-by: YuZhang <zhangyu31@baidu.com>
> Signed-off-by: JinYu <jin.yu@intel.com>
> ---
> V1 - specify the APIs are split-ring only
> ---
>   lib/librte_vhost/rte_vhost.h           |  99 +++++++++
>   lib/librte_vhost/rte_vhost_version.map |   4 +
>   lib/librte_vhost/vhost.c               | 135 ++++++++++++
>   lib/librte_vhost/vhost.h               |  12 +
>   lib/librte_vhost/vhost_user.c          | 292 +++++++++++++++++++++++++
>   lib/librte_vhost/vhost_user.h          |  13 +-
>   6 files changed, 554 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 0226b3eff..c9959c15a 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -71,6 +71,10 @@ extern "C" {
>   #define VHOST_USER_PROTOCOL_F_HOST_NOTIFIER 11
>   #endif
>   
> +#ifndef VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD
> +#define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12
> +#endif
> +
>   /** Indicate whether protocol features negotiation is supported. */
>   #ifndef VHOST_USER_F_PROTOCOL_FEATURES
>   #define VHOST_USER_F_PROTOCOL_FEATURES	30
> @@ -98,12 +102,41 @@ struct rte_vhost_memory {
>   	struct rte_vhost_mem_region regions[];
>   };
>   
> +struct inflight_desc_split {
> +	uint8_t		inflight;
> +	uint8_t		padding[5];
> +	uint16_t	next;
> +	uint64_t	counter;
> +};
> +
> +struct inflight_info_split {
> +	uint64_t		features;
> +	uint16_t		version;
> +	uint16_t		desc_num;
> +	uint16_t		last_inflight_io;
> +	uint16_t		used_idx;
> +	struct inflight_desc_split desc[0];
> +};
> +
> +struct resubmit_desc {
> +	uint16_t index;
> +	uint64_t counter;
> +};
> +
> +struct resubmit_info {
> +	struct resubmit_desc	*resubmit_list;
> +	uint16_t		resubmit_num;
> +};
> +
>   struct rte_vhost_vring {
>   	struct vring_desc	*desc;
>   	struct vring_avail	*avail;
>   	struct vring_used	*used;
>   	uint64_t		log_guest_addr;
>   
> +	struct inflight_info_split *inflight;
> +	struct resubmit_info	*resubmit_inflight;
> +

This is breaking the ABI, and discussing with Tiwei, we think it could
be avoided by defining its own structure of inflight and inflight
resubmit pointers. More below:

>   	/** Deprecated, use rte_vhost_vring_call() instead. */
>   	int			callfd;
>   
> @@ -603,6 +636,22 @@ uint16_t rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>    */
>   int rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem);
>   
> +/**
> + * Get guest vring info, including the vring address, vring size, inflight, etc.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @param vring
> + *  the structure to hold the requested vring info
> + * @return
> + *  0 on success, -1 on failure
> + */
> +int __rte_experimental
> +rte_vhost_get_vhost_vring_with_inflight_split(int vid, uint16_t vring_idx,
> +			      struct rte_vhost_vring *vring);

Here a new API is introduced to get vting info with inflight.
It could be just changed to get only the inflight info. It means the
caller would first call rte_vhost_get_vhost_vring() and then the new
function, which I guess may be renamed to:

rte_vhost_get_vhost_vring_inflight_split

Other than that, the patch looks good to me.

(Next time, please run get_maintainer.sh script so that all the
maintainers are in Cc:).

Thanks,
Maxime