[2/5] lib/vhost: implement rte_power_monitor API
Checks
Commit Message
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(+)
Comments
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
Hi Chenbo,
> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Wednesday, September 15, 2021 4:52 PM
> To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: RE: [PATCH 2/5] lib/vhost: implement rte_power_monitor API
>
> 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'
I will modify it in the next version.
>
> >
> > 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.
Yes, I agree with you. I will remove it in the next version.
>
> > +
> > /* 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.
I think one value to represent true or false is enough. I will fix it in the next version.
>
> > +};
> > +
> > /**
> > * 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.
I will add dev and queue_id check here in the next version.
>
> > + 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?
Yes, it is unnecessary. I will delete it in the next version.
Thanks,
Miao
>
> 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
@@ -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)
+
/* 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 */
+};
+
/**
* 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
*
@@ -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;
};
@@ -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);
+ 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;
+
+ return 0;
+}
+
RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);