Re: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement rte_power_monitor API

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

Hi,

> -----Original Message-----
> From: Li, Miao
> Sent: Friday, October 22, 2021 4:28 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <Chenbo.Xia@intel.com>; maxime.coquelin@redhat.com
> Subject: RE: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement
> rte_power_monitor API
> 
> Hi,
> 
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: Friday, October 22, 2021 12:59 AM
> > To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
> > Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com
> > Subject: Re: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement
> > rte_power_monitor API
> >
> > On 10/18/2021 3:16 PM, Miao Li wrote:
> > > This patch implements rte_power_monitor API in virtio PMD to reduce
> > > power consumption when no packet come in. According to current
> semantics
> > > of power monitor, this commit adds a callback function to decide whether
> > > aborts the sleep by checking current value against the expected value and
> > > virtio_get_monitor_addr to provide address to monitor. When no packet
> > come
> > > in, the value of address will not be changed and the running core will
> > > sleep. Once packets arrive, the value of address will be changed and the
> > > running core will wakeup.
> > >
> >
> > A minor comment but instead of patch title mentioning what is implemented,
> > better to describe what feature is added, like:
> > net/virtio: support power monitor
> 
> I will modify it in the next version.
> 
> >
> > > Signed-off-by: Miao Li <miao.li@intel.com>
> > > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > > ---
> > >   doc/guides/rel_notes/release_21_11.rst |  4 ++
> > >   drivers/net/virtio/virtio_ethdev.c     | 56 ++++++++++++++++++++++++++
> > >   2 files changed, 60 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > b/doc/guides/rel_notes/release_21_11.rst
> > > index d5435a64aa..c298844898 100644
> > > --- a/doc/guides/rel_notes/release_21_11.rst
> > > +++ b/doc/guides/rel_notes/release_21_11.rst
> > > @@ -80,6 +80,10 @@ New Features
> > >     Added macros ETH_RSS_IPV4_CHKSUM and ETH_RSS_L4_CHKSUM, now
> > IPv4 and
> > >     TCP/UDP/SCTP header checksum field can be used as input set for RSS.
> > >
> > > +* **Updated virtio PMD.**
> > > +
> > > +  Implement rte_power_monitor API in virtio PMD.
> > > +
> >
> > The release note updates are grouped as described in the documents section
> > comment, and in a group it is sorted alphabetically, can you please re-arrange
> > the update accordingly. And similar comment, instead of documenting what
> API
> > implemented, you can mention what support is added.
> 
> I will re-arrange the update and modify the description in the next version.

The updates are requested to be ordered alphabetically by vendor name. But
Vhost/virtio do not have a vendor name. So should I order virtio/vhost by 'v'?

Thanks,
Miao

> 
> >
> > Same comment for other patches in the set.
> 
> I will modify them in the next version.
> 
> Thanks,
> Miao
> 
> >