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

["Li, Miao" <miao.li@intel.com>]

Hi Chenbo,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Wednesday, September 29, 2021 11:01 AM
> To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: RE: [PATCH v3 2/5] vhost: implement rte_power_monitor API
> 
> 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).

I will modify them in the next version.

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

I will update it in the next version.

Thanks,
Miao

> 
> Thanks,
> Chenbo