Re: [dpdk-dev] [PATCH 2/5] lib/vhost: implement rte_power_monitor API

["Xia, Chenbo" <chenbo.xia@intel.com>]

Hi Miao,

> -----Original Message-----
> From: Li, Miao <miao.li@intel.com>
> Sent: Friday, September 10, 2021 9:06 PM
> To: dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Li, Miao
> <miao.li@intel.com>
> Subject: [PATCH 2/5] lib/vhost: implement rte_power_monitor API

Should be 'vhost: implement rte_power_monitor API'

> 
> This patch defines rte_vhost_power_monitor_cond which is used to pass
> some information to vhost driver. The information is including the address
> to monitor, the expected value, the mask to extract value read from 'addr',
> the flag used to distinguish packed ring or split ring. Vhost driver can
> use these information to fill rte_power_monitor_cond.
> 
> Signed-off-by: Miao Li <miao.li@intel.com>
> ---
>  lib/vhost/rte_vhost.h | 33 +++++++++++++++++++++++++++++++++
>  lib/vhost/version.map |  3 +++
>  lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index 8d875e9322..f58643b0a3 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -38,6 +38,8 @@ extern "C" {
>  #define RTE_VHOST_USER_ASYNC_COPY	(1ULL << 7)
>  #define RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS	(1ULL << 8)
> 
> +#define VHOST_POWER_MONITOR_RING_PACKED (1ULL << 0)

I'd say I don't quite like introducing this flag so that vhost lib
app needs to know the vring is split or packed. I have another suggestion
to do the same thing, please check below comment.

> +
>  /* Features. */
>  #ifndef VIRTIO_NET_F_GUEST_ANNOUNCE
>   #define VIRTIO_NET_F_GUEST_ANNOUNCE 21
> @@ -292,6 +294,20 @@ struct vhost_device_ops {
>  	void *reserved[1]; /**< Reserved for future extension */
>  };
> 
> +/**
> + * Power monitor condition.
> + */
> +struct rte_vhost_power_monitor_cond {
> +	volatile void *addr;  /**< Address to monitor for changes */
> +	/**< If the `mask` is non-zero, location pointed
> +	 *   to by `addr` will be read and compared
> +	 *   against this value.
> +	 */
> +	uint64_t val;
> +	uint64_t mask; /**< 64-bit mask to extract value read from `addr` */
> +	uint8_t flag;  /**< if 1, vhost packed ring, otherwise split ring */

What about define two values instead of the flag. One value for
'(value & m) == v ?' is True, another for False.

> +};
> +
>  /**
>   * Convert guest physical address to host virtual address
>   *
> @@ -914,6 +930,23 @@ int rte_vhost_vring_call(int vid, uint16_t vring_idx);
>   */
>  uint32_t rte_vhost_rx_queue_count(int vid, uint16_t qid);
> 
> +/**
> + * Get power monitor address of the vhost device
> + *
> + * @param vid
> + *  vhost device ID
> + * @param queue_id
> + *  vhost queue ID
> + * @param pmc
> + *  power monitor condition
> + * @return
> + *  0 on success, -1 on failure
> + */
> +__rte_experimental
> +int
> +rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
> +		struct rte_vhost_power_monitor_cond *pmc);
> +
>  /**
>   * Get log base and log size of the vhost device
>   *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index c92a9d4962..0a9667ef1e 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -85,4 +85,7 @@ EXPERIMENTAL {
>  	rte_vhost_async_channel_register_thread_unsafe;
>  	rte_vhost_async_channel_unregister_thread_unsafe;
>  	rte_vhost_clear_queue_thread_unsafe;
> +
> +	# added in 21.11
> +	rte_vhost_get_monitor_addr;
>  };
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 355ff37651..f7374d3f94 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1886,5 +1886,35 @@ int rte_vhost_async_get_inflight(int vid, uint16_t
> queue_id)
>  	return ret;
>  }
> 
> +int
> +rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
> +		struct rte_vhost_power_monitor_cond *pmc)
> +{
> +	struct virtio_net *dev = get_device(vid);

Check dev is not NULL before accessing its member.

> +	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
> +	if (vq == NULL)
> +		return -1;
> +	if (vq_is_packed(dev)) {
> +		struct vring_packed_desc *desc;
> +		desc = vq->desc_packed;
> +		pmc->addr = &desc[vq->last_avail_idx].flags;
> +		if (vq->avail_wrap_counter)
> +			pmc->val = VRING_DESC_F_AVAIL;
> +		else
> +			pmc->val = VRING_DESC_F_USED;
> +		pmc->mask = VRING_DESC_F_AVAIL | VRING_DESC_F_USED;
> +		pmc->flag = VHOST_POWER_MONITOR_RING_PACKED;
> +	} else {
> +		pmc->addr = &vq->avail->idx;
> +		pmc->val = vq->last_avail_idx & (vq->size - 1);
> +		pmc->mask = vq->size - 1;
> +		pmc->flag = 0;
> +	}
> +	if (pmc->addr == NULL)
> +		return -1;

Is it possible that addr == NULL?

Thanks,
Chenbo

> +
> +	return 0;
> +}
> +
>  RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
>  RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
> --
> 2.25.1