Re: [dpdk-dev] [PATCH v3 2/5] 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 24, 2021 6:23 PM
> To: dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Li, Miao
> <miao.li@intel.com>
> Subject: [PATCH v3 2/5] 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 value size of monitor address, the match flag used to distinguish the
> value used to match something or not match something. 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 | 41 +++++++++++++++++++++++++++++++++++++++++
>  lib/vhost/version.map |  3 +++
>  lib/vhost/vhost.c     | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index 8d875e9322..4e1f2de12f 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -292,6 +292,30 @@ 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;

Will be read and masked, then compared with this value?

> +	uint64_t mask; /**< 64-bit mask to extract value read from `addr` */
> +	/**< Data size (in bytes) that will be read from the
> +	 *   monitored memory location (`addr`). Can be 1, 2,
> +	 *   4, or 8. Supplying any other value will result in
> +	 *   an error.
> +	 */
> +	uint8_t size;
> +	/**< If 1, checking if `val` matches something.
> +	 *  If 0, checking if `val` *doesn't* match a
> +	 *  particular value.
> +	 */

If 1, and masked value that read from `addr` equals `val`, drivers can exit the
power-saving state.

If 0, ....

The overall comment can't make me understand how this struct is used if I read
the next patch.

> +	uint8_t match;

The comment style is a bit messy here. You can make every comment above variable
Definition (like val/size/match ) or make the first line the same as the variable
line (like addr/mask). 

And please update the release note(release_20_11.rst), it's a new feature.

Thanks,
Chenbo