[v7,1/5] net/virtio: implement rte_power_monitor API

  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.

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(+)
  

On 10/18/2021 3:16 PM, Miao Li wrote:
> This patch implements rte_power_monitor API in virtio PMD to reduce

Hi Miao,

If there will be a new version can you please drop the "This patch implements"
part, you can directly start explaining what patch does.

> 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.
> 
> Signed-off-by: Miao Li<miao.li@intel.com>
> Reviewed-by: Chenbo Xia<chenbo.xia@intel.com>


<...>
  

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

> 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.

Same comment for other patches in the set.
  

Hi,

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Friday, October 22, 2021 12:51 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
> 
> Hi Miao,
> 
> If there will be a new version can you please drop the "This patch implements"
> part, you can directly start explaining what patch does.

I will remove this part in the next version.

Thanks,
Miao

> 
> > 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.
> >
> > Signed-off-by: Miao Li<miao.li@intel.com>
> > Reviewed-by: Chenbo Xia<chenbo.xia@intel.com>
> 
> 
> <...>
  

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.

> 
> Same comment for other patches in the set.

I will modify them in the next version.

Thanks,
Miao

>
  

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
> 
> >
  

On 10/22/2021 9:51 AM, Li, Miao wrote:
> 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'?
> 

I think sort it by 'v' is good, unless Maxime & Chenbo has better idea.
  

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Friday, October 22, 2021 5:00 PM
> 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/22/2021 9:51 AM, Li, Miao wrote:
> > 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'?
> >
> 
> I think sort it by 'v' is good, unless Maxime & Chenbo has better idea.

Good idea to sort by v, so we are aligned that we use the name 'Virtio' and 'Vhost'
instead of vendor name to sort in later release.

Thanks,
Chenbo
  

On 18/10/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.
>
> 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.
> +
>   * **Updated af_packet ethdev driver.**
>   
>     * Default VLAN strip behavior was changed. VLAN tag won't be stripped
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 6aa36b3f39..1227f3f1f4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -74,6 +74,8 @@ static int virtio_mac_addr_set(struct rte_eth_dev *dev,
>   				struct rte_ether_addr *mac_addr);
>   
>   static int virtio_intr_disable(struct rte_eth_dev *dev);
> +static int virtio_get_monitor_addr(void *rx_queue,
> +				struct rte_power_monitor_cond *pmc);
>   
>   static int virtio_dev_queue_stats_mapping_set(
>   	struct rte_eth_dev *eth_dev,
> @@ -982,6 +984,7 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
>   	.mac_addr_add            = virtio_mac_addr_add,
>   	.mac_addr_remove         = virtio_mac_addr_remove,
>   	.mac_addr_set            = virtio_mac_addr_set,
> +	.get_monitor_addr        = virtio_get_monitor_addr,
>   };
>   
>   /*
> @@ -1313,6 +1316,59 @@ virtio_mac_addr_set(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr)
>   	return 0;
>   }
>   
> +#define CLB_VAL_IDX 0
> +#define CLB_MSK_IDX 1
> +#define CLB_MATCH_IDX 2
> +static int
> +virtio_monitor_callback(const uint64_t value,
> +		const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])
> +{
> +	const uint64_t m = opaque[CLB_MSK_IDX];
> +	const uint64_t v = opaque[CLB_VAL_IDX];
> +	const uint64_t c = opaque[CLB_MATCH_IDX];
> +
> +	if (c)
> +		return (value & m) == v ? -1 : 0;
> +	else
> +		return (value & m) == v ? 0 : -1;
> +}
> +
> +static int
> +virtio_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)
> +{
> +	struct virtnet_rx *rxvq = rx_queue;
> +	struct virtqueue *vq = virtnet_rxq_to_vq(rxvq);
> +	struct virtio_hw *hw;
> +
> +	if (vq == NULL)
> +		return -EINVAL;
> +
> +	hw = vq->hw;
> +	if (virtio_with_packed_queue(hw)) {
> +		struct vring_packed_desc *desc;
> +		desc = vq->vq_packed.ring.desc;
> +		pmc->addr = &desc[vq->vq_used_cons_idx].flags;
> +		if (vq->vq_packed.used_wrap_counter)
> +			pmc->opaque[CLB_VAL_IDX] =
> +						VRING_PACKED_DESC_F_AVAIL_USED;
> +		else
> +			pmc->opaque[CLB_VAL_IDX] = 0;
> +		pmc->opaque[CLB_MSK_IDX] = VRING_PACKED_DESC_F_AVAIL_USED;
> +		pmc->opaque[CLB_MATCH_IDX] = 1;
> +		pmc->size = sizeof(desc[vq->vq_used_cons_idx].flags);
> +	} else {
> +		pmc->addr = &vq->vq_split.ring.used->idx;
> +		pmc->opaque[CLB_VAL_IDX] = vq->vq_used_cons_idx
> +					& (vq->vq_nentries - 1);
> +		pmc->opaque[CLB_MSK_IDX] = vq->vq_nentries - 1;
> +		pmc->opaque[CLB_MATCH_IDX] = 0;
> +		pmc->size = sizeof(vq->vq_split.ring.used->idx);
> +	}
> +	pmc->fn = virtio_monitor_callback;
> +
> +	return 0;
> +}
> +
>   static int
>   virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
>   {


LGTM.

Acked-by: David Hunt <david.hunt@intel.com>