Re: [dpdk-dev] [PATCH v5 2/5] vhost: implement rte_power_monitor API
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Friday, October 15, 2021 3:38 PM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH v5 2/5] vhost: implement rte_power_monitor API
>
> Hi,
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, October 15, 2021 11:12 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li,
> Miao
> >
> > Subject: [PATCH v5 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
> > ---
> > doc/guides/rel_notes/release_21_11.rst | 4 +++
> > lib/vhost/rte_vhost.h | 44 ++
> > lib/vhost/version.map | 3 ++
> > lib/vhost/vhost.c | 38 ++
> > 4 files changed, 89 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > b/doc/guides/rel_notes/release_21_11.rst
> > index 27dc896703..ad6d256a55 100644
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -72,6 +72,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.
> >
> > +* **Added power monitor API in vhost library.**
> > +
> > + Added an API to support power monitor in vhost library.
> > +
> > * **Updated virtio PMD.**
> >
> >Implement rte_power_monitor API in virtio PMD.
> > diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> > index fd372d5259..42bda95e96 100644
> > --- a/lib/vhost/rte_vhost.h
> > +++ b/lib/vhost/rte_vhost.h
> > @@ -292,6 +292,33 @@ struct vhost_device_ops {
> > void *reserved[1]; /**< Reserved for future extension */
> > };
> >
> > +/**
> > + * Power monitor condition.
> > + */
> > +struct rte_vhost_power_monitor_cond {
> > + /**< Address to monitor for changes */
> > + volatile void *addr;
> > + /**< If the `mask` is non-zero, location pointed
> > +* to by `addr` will be read and masked, then
> > +* compared with this value.
> > +*/
> > + uint64_t val;
> > + /**< 64-bit mask to extract value read from `addr` */
> > + uint64_t mask;
> > + /**< 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.
>
> 'Can be ...' part is not necessary, as this value is defined in vhost
> lib and currently only has two different values for packed or split.
I will remove it in the next version.
>
> > +*/
> > + uint8_t size;
> > + /**< If 1, and masked value that read from 'addr' equals
> > +* 'val', the driver will skip core sleep. If 0, and
>
> 'will' -> 'should'. As it's a suggestion for vhost driver.
>
> > +* masked value that read from 'addr' does not equal 'val',
> > +* the driver will skip core sleep.
>
> Ditto.
I will modify them in the next version.
Thanks,
Miao
>
> Thanks,
> Chenbo
>
> > +*/
> > + uint8_t match;
> > +};
> > +
> > /**
> > * Convert guest physical address to host virtual address
> > *
> > @@ -903,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);
> >
Re: [dpdk-dev] [PATCH v5 3/5] net/vhost: implement rte_power_monitor API
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Friday, October 15, 2021 3:40 PM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH v5 3/5] net/vhost: implement rte_power_monitor API
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, October 15, 2021 11:12 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li,
> Miao
> >
> > Subject: [PATCH v5 3/5] net/vhost: implement rte_power_monitor API
> >
> > This patch implements rte_power_monitor API in vhost 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
> > vhost_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
> > ---
> > doc/guides/rel_notes/release_21_11.rst | 4 +++
> > drivers/net/vhost/rte_eth_vhost.c | 40 ++
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > b/doc/guides/rel_notes/release_21_11.rst
> > index ad6d256a55..e6f9c284ae 100644
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -76,6 +76,10 @@ New Features
> >
> >Added an API to support power monitor in vhost library.
> >
> > +* **Updated vhost PMD.**
> > +
> > + Implement rte_power_monitor API in vhost PMD.
> > +
> > * **Updated virtio PMD.**
> >
> >Implement rte_power_monitor API in virtio PMD.
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > b/drivers/net/vhost/rte_eth_vhost.c
> > index 2e24e5f7ff..ee665ee64d 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -1386,6 +1386,45 @@ eth_rx_queue_count(struct rte_eth_dev *dev,
> uint16_t
> > rx_queue_id)
> > return rte_vhost_rx_queue_count(vq->vid, vq->virtqueue_id);
> > }
> >
> > +#define CLB_VAL_IDX 0
> > +#define CLB_MSK_IDX 1
> > +#define CLB_MATCH_IDX 2
> > +static int
> > +vhost_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
> > +vhost_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond
> *pmc)
> > +{
> > + struct vhost_queue *vq = rx_queue;
> > + struct rte_vhost_power_monitor_cond vhost_pmc;
> > + int ret;
> > + if (vq == NULL)
> > + return -EINVAL;
> > + ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
> > + &vhost_pmc);
> > + if (ret < 0)
> > + return -EINVAL;
> > + pmc->addr = vhost_pmc.addr;
> > + pmc->opaque[CLB_VAL_IDX] = vhost_pmc.val;
> > + pmc->opaque[CLB_MSK_IDX] = vhost_pmc.mask;
> > + pmc->opaque[CLB_MATCH_IDX] = vhost_pmc.match;
> > + pmc->size = vhost_pmc.size;
> > + pmc->fn = vhost_monitor_callback;
> > +
> > + return 0;
> > +}
> > +
> > static const struct eth_dev_ops ops = {
> > .dev_start = eth_dev_start,
> > .dev_stop = eth_dev_stop,
> > @@ -1405,6 +1444,7 @@ static const struct eth_dev_ops ops = {
> > .xstats_get_names = vhost_dev_xstats_get_names,
> > .rx_queue_intr_enable = eth_rxq_intr_enable,
> > .rx_queue_intr_disable = eth_rxq_intr_disable,
> > + .get_monitor_addr= vhost_get_monitor_addr,
>
> Please align the format with above callbacks: one space is enough after
> 'get_monitor_addr'
I will remove extra space in the next version.
Thanks,
Miao
>
> Thanks,
> Chenbo
>
> > };
> >
> > static int
> > --
> > 2.25.1
Re: [dpdk-dev] [PATCH v5 4/5] power: modify return of queue_stopped
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Friday, October 15, 2021 3:47 PM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com; Burakov, Anatoly
>
> Subject: RE: [PATCH v5 4/5] power: modify return of queue_stopped
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, October 15, 2021 11:12 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li,
> Miao
> > ; Burakov, Anatoly
> > Subject: [PATCH v5 4/5] power: modify return of queue_stopped
> >
> > Since some vdevs like virtio and vhost do not support rxq_info_get and
> > queue state inquiry, the error return value -ENOTSUP need to be ignored
> > when queue_stopped cannot get rx queue information and rx queue state.
> > This patch changes the return value of queue_stopped when
> > rte_eth_rx_queue_info_get return ENOTSUP to support vdevs which cannot
>
> ENOTSUP -> -ENOTSUP
>
> With this fixed:
>
> Reviewed-by: Chenbo Xia
I will fix it in the next version.
Thanks,
Miao
>
> > provide rx queue information and rx queue state enable power management.
> >
> > Signed-off-by: Miao Li
> > Acked-by: Anatoly Burakov
> > ---
> > lib/power/rte_power_pmd_mgmt.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/power/rte_power_pmd_mgmt.c
> b/lib/power/rte_power_pmd_mgmt.c
> > index 0ce40f0875..39a2b4cd23 100644
> > --- a/lib/power/rte_power_pmd_mgmt.c
> > +++ b/lib/power/rte_power_pmd_mgmt.c
> > @@ -382,8 +382,13 @@ queue_stopped(const uint16_t port_id, const
> uint16_t
> > queue_id)
> > {
> > struct rte_eth_rxq_info qinfo;
> >
> > - if (rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo) < 0)
> > - return -1;
> > + int ret = rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo);
> > + if (ret < 0) {
> > + if (ret == -ENOTSUP)
> > + return 1;
> > + else
> > + return -1;
> > + }
> >
> > return qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED;
> > }
> > --
> > 2.25.1
Re: [dpdk-dev] [PATCH v5 5/5] examples/l3fwd-power: support virtio/vhost
Hi Chenbo, > -Original Message- > From: Xia, Chenbo > Sent: Friday, October 15, 2021 4:14 PM > To: Li, Miao ; dev@dpdk.org > Cc: maxime.coque...@redhat.com > Subject: RE: [PATCH v5 5/5] examples/l3fwd-power: support virtio/vhost > > > -Original Message- > > From: Li, Miao > > Sent: Friday, October 15, 2021 11:12 PM > > To: dev@dpdk.org > > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li, > Miao > > > > Subject: [PATCH v5 5/5] examples/l3fwd-power: support virtio/vhost > > > > In l3fwd-power, there is default port configuration which requires > > RSS and IPV4/UDP/TCP checksum. Once device does not support these, > > the l3fwd-power will exit and report an error. > > This patch updates the port configuration based on device capabilities > > after getting the device information to support devices like virtio > > and vhost. > > > > Signed-off-by: Miao Li > > --- > > examples/l3fwd-power/main.c | 9 - > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c > > index 73a3ab5bc0..61c15e01d2 100644 > > --- a/examples/l3fwd-power/main.c > > +++ b/examples/l3fwd-power/main.c > > @@ -505,7 +505,9 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t > > link_len) > > return -1; > > > > /* 2. The IP checksum must be correct. */ > > - /* this is checked in H/W */ > > + /* if this is not checked in H/W, check it. */ > > + if ((port_conf.rxmode.offloads & DEV_RX_OFFLOAD_IPV4_CKSUM) == 0) > > + rte_ipv4_cksum(pkt); > > This is not correct. The correct handling should be: > > 1. get actual cksum from pkt and save it > 2. set pkt cksum to zero > 3. compute correct cksum using rte_ipv4_cksum > 4. compare to know if actual cksum == correct cksum > > You can refer to test_ipsec_l3_csum_verify in test_cryptodev_security_ipsec.c > > Thanks, > Chenbo I will fix it in the next version. Thanks, Miao > > > > > /* > > * 3. The IP version number must be 4. If the version number is not 4 > > @@ -2637,6 +2639,11 @@ main(int argc, char **argv) > > local_port_conf.rx_adv_conf.rss_conf.rss_hf); > > } > > > > + if (local_port_conf.rx_adv_conf.rss_conf.rss_hf == 0) > > + local_port_conf.rxmode.mq_mode = > ETH_MQ_RX_NONE; > > + local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa; > > + port_conf.rxmode.offloads = local_port_conf.rxmode.offloads; > > + > > ret = rte_eth_dev_configure(portid, nb_rx_queue, > > (uint16_t)n_tx_queue, > &local_port_conf); > > if (ret < 0) > > -- > > 2.25.1
Re: [dpdk-dev] [PATCH v6 0/5] Implement rte_power_monitor API in virtio/vhost PMD
Hi, > -Original Message- > From: Maxime Coquelin > Sent: Friday, October 15, 2021 8:58 PM > To: Li, Miao ; dev@dpdk.org > Cc: Xia, Chenbo > Subject: Re: [PATCH v6 0/5] Implement rte_power_monitor API in virtio/vhost > PMD > > Hi, > > On 10/15/21 19:09, Miao Li wrote: > > This patchset implements rte_power_monitor API in virtio and vhost PMD > > to reduce power consumption when no packet come in. This API can be > > called and tested in l3fwd-power after adding vhost and virtio support > > in l3fwd-power and ignoring the rx queue information check in > > queue_stopped(). > > > > v6: > > -modify comment > > -remove extra space > > -fix IPv4 CKSUM check > > > > v5: > > -Rebase on lastest repo > > > > v4: > > -modify comment > > -update the release note > > -add IPv4 CKSUM check > > > > v3: > > -fix some code format issues > > -fix spelling mistake > > > > v2: > > -remove flag and add match and size in rte_vhost_power_monitor_cond > > -modify power callback function > > -add dev and queue id check and remove unnecessary check > > -fix the assignment of pmc->size > > -update port configuration according to the device information and > > remove adding command line arguments > > -modify some titles > > > > Miao Li (5): > >net/virtio: implement rte_power_monitor API > >vhost: implement rte_power_monitor API > >net/vhost: implement rte_power_monitor API > >power: modify return of queue_stopped > >examples/l3fwd-power: support virtio/vhost > > > > doc/guides/rel_notes/release_21_11.rst | 12 ++ > > drivers/net/vhost/rte_eth_vhost.c | 40 ++ > > drivers/net/virtio/virtio_ethdev.c | 56 ++ > > examples/l3fwd-power/main.c| 15 ++- > > lib/power/rte_power_pmd_mgmt.c | 9 - > > lib/vhost/rte_vhost.h | 42 +++ > > lib/vhost/version.map | 3 ++ > > lib/vhost/vhost.c | 38 + > > 8 files changed, 212 insertions(+), 3 deletions(-) > > > > Please run checkpatch and check-git-log scripts, there are some issues. Ok, I will fix them in the next version. Thanks, Miao > > Thanks, > Maxime
Re: [dpdk-dev] [PATCH v7 4/5] power: modify return of queue_stopped
Hi,
> -Original Message-
> From: Yigit, Ferruh
> Sent: Friday, October 22, 2021 12:48 AM
> To: Li, Miao ; dev@dpdk.org; Hunt, David
>
> Cc: Xia, Chenbo ; maxime.coque...@redhat.com;
> Burakov, Anatoly
> Subject: Re: [dpdk-dev] [PATCH v7 4/5] power: modify return of queue_stopped
>
> On 10/18/2021 3:16 PM, Miao Li wrote:
> > Since some vdevs like virtio and vhost do not support rxq_info_get and
> > queue state inquiry, the error return value -ENOTSUP need to be ignored
> > when queue_stopped cannot get rx queue information and rx queue state.
> > This patch changes the return value of queue_stopped when
> > rte_eth_rx_queue_info_get return -ENOTSUP to support vdevs which cannot
> > provide rx queue information and rx queue state enable power management.
> >
>
> Don't we want to backport this patch? In case later a patch in the main
> repo relies on this return type and that needs to be merged to LTS...
>
> Also need to clarify if this is a fix or not.
Yes, it is a fix. I will backport this patch to LTS in the next version.
Thanks,
Miao
>
> > Signed-off-by: Miao Li
> > Acked-by: Anatoly Burakov
> > Reviewed-by: Chenbo Xia
>
> power library maintainer ack/review is missing.
>
> > ---
> > lib/power/rte_power_pmd_mgmt.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/power/rte_power_pmd_mgmt.c
> b/lib/power/rte_power_pmd_mgmt.c
> > index 0ce40f0875..39a2b4cd23 100644
> > --- a/lib/power/rte_power_pmd_mgmt.c
> > +++ b/lib/power/rte_power_pmd_mgmt.c
> > @@ -382,8 +382,13 @@ queue_stopped(const uint16_t port_id, const
> uint16_t queue_id)
> > {
> > struct rte_eth_rxq_info qinfo;
> >
> > - if (rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo) < 0)
> > - return -1;
> > + int ret = rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo);
> > + if (ret < 0) {
> > + if (ret == -ENOTSUP)
> > + return 1;
> > + else
> > + return -1;
> > + }
> >
> > return qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED;
> > }
> >
Re: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement rte_power_monitor API
Hi, > -Original Message- > From: Yigit, Ferruh > Sent: Friday, October 22, 2021 12:51 AM > To: Li, Miao ; dev@dpdk.org > Cc: Xia, Chenbo ; maxime.coque...@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 > > Reviewed-by: Chenbo Xia > > > <...>
Re: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement rte_power_monitor API
Hi, > -Original Message- > From: Yigit, Ferruh > Sent: Friday, October 22, 2021 12:59 AM > To: Li, Miao ; dev@dpdk.org > Cc: Xia, Chenbo ; maxime.coque...@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 > > Reviewed-by: Chenbo Xia > > --- > > 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 >
Re: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement rte_power_monitor API
Hi, > -Original Message- > From: Li, Miao > Sent: Friday, October 22, 2021 4:28 PM > To: Yigit, Ferruh ; dev@dpdk.org > Cc: Xia, Chenbo ; maxime.coque...@redhat.com > Subject: RE: [dpdk-dev] [PATCH v7 1/5] net/virtio: implement > rte_power_monitor API > > Hi, > > > -Original Message- > > From: Yigit, Ferruh > > Sent: Friday, October 22, 2021 12:59 AM > > To: Li, Miao ; dev@dpdk.org > > Cc: Xia, Chenbo ; maxime.coque...@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 > > > Reviewed-by: Chenbo Xia > > > --- > > > 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 > > >
Re: [dpdk-dev] [PATCH v7 4/5] power: modify return of queue_stopped
Hi,
> -Original Message-
> From: Yigit, Ferruh
> Sent: Friday, October 22, 2021 5:01 PM
> To: Li, Miao ; dev@dpdk.org; Hunt, David
>
> Cc: Xia, Chenbo ; maxime.coque...@redhat.com;
> Burakov, Anatoly
> Subject: Re: [dpdk-dev] [PATCH v7 4/5] power: modify return of queue_stopped
>
> On 10/22/2021 9:20 AM, Li, Miao wrote:
> > Hi,
> >
> >> -Original Message-
> >> From: Yigit, Ferruh
> >> Sent: Friday, October 22, 2021 12:48 AM
> >> To: Li, Miao ; dev@dpdk.org; Hunt, David
> >>
> >> Cc: Xia, Chenbo ; maxime.coque...@redhat.com;
> >> Burakov, Anatoly
> >> Subject: Re: [dpdk-dev] [PATCH v7 4/5] power: modify return of
> queue_stopped
> >>
> >> On 10/18/2021 3:16 PM, Miao Li wrote:
> >>> Since some vdevs like virtio and vhost do not support rxq_info_get and
> >>> queue state inquiry, the error return value -ENOTSUP need to be ignored
> >>> when queue_stopped cannot get rx queue information and rx queue state.
> >>> This patch changes the return value of queue_stopped when
> >>> rte_eth_rx_queue_info_get return -ENOTSUP to support vdevs which
> cannot
> >>> provide rx queue information and rx queue state enable power management.
> >>>
> >>
> >> Don't we want to backport this patch? In case later a patch in the main
> >> repo relies on this return type and that needs to be merged to LTS...
> >>
> >> Also need to clarify if this is a fix or not.
> >
> > Yes, it is a fix. I will backport this patch to LTS in the next version.
> >
>
> If so can you please update the Fixes and stable tags?
>
> This way it will be part of backporting process.
I will update them in the next version.
Thanks,
Miao
>
> > Thanks,
> > Miao
> >
> >>
> >>> Signed-off-by: Miao Li
> >>> Acked-by: Anatoly Burakov
> >>> Reviewed-by: Chenbo Xia
> >>
> >> power library maintainer ack/review is missing.
> >>
> >>> ---
> >>>lib/power/rte_power_pmd_mgmt.c | 9 +++--
> >>>1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/power/rte_power_pmd_mgmt.c
> >> b/lib/power/rte_power_pmd_mgmt.c
> >>> index 0ce40f0875..39a2b4cd23 100644
> >>> --- a/lib/power/rte_power_pmd_mgmt.c
> >>> +++ b/lib/power/rte_power_pmd_mgmt.c
> >>> @@ -382,8 +382,13 @@ queue_stopped(const uint16_t port_id, const
> >> uint16_t queue_id)
> >>>{
> >>> struct rte_eth_rxq_info qinfo;
> >>>
> >>> - if (rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo) < 0)
> >>> - return -1;
> >>> + int ret = rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo);
> >>> + if (ret < 0) {
> >>> + if (ret == -ENOTSUP)
> >>> + return 1;
> >>> + else
> >>> + return -1;
> >>> + }
> >>>
> >>> return qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED;
> >>>}
> >>>
> >
Re: [dpdk-dev] [PATCH 5/5] examples/l3fwd-power: support virtio/vhost
In l3fwd-power, there is default port configuration which requires RSS and
IPV4/UDP/TCP checksum. Once device does not support these, the l3fwd-power will
exit and report an error.
Miao
-Original Message-
From: Maxime Coquelin
Sent: Friday, September 10, 2021 3:24 PM
To: Li, Miao ; dev@dpdk.org
Cc: Xia, Chenbo ; David Marchand
Subject: Re: [PATCH 5/5] examples/l3fwd-power: support virtio/vhost
On 9/10/21 3:05 PM, Miao Li wrote:
> This patch adds two command line arguments which will be needed when
> using virtio/vhost vdev. One argument sets rx offloads capabilities
> DEV_RX_OFFLOAD_VLAN_STRIP. The other argument sets DCB, PSS and VMDQ
> off for RX side.
>
> Signed-off-by: Miao Li
> ---
> examples/l3fwd-power/main.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index aa7b8db44a..4e28008578 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -1811,6 +1811,8 @@ parse_args(int argc, char **argv)
> {"high-perf-cores", 1, 0, 0},
> {"no-numa", 0, 0, 0},
> {"enable-jumbo", 0, 0, 0},
> + {"vlan-strip", 0, 0, 0},
> + {"rx-none", 0, 0, 0},
> {CMD_LINE_OPT_EMPTY_POLL, 1, 0, 0},
> {CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
> {CMD_LINE_OPT_LEGACY, 0, 0, 0},
> @@ -1986,6 +1988,19 @@ parse_args(int argc, char **argv)
> (unsigned int)port_conf.rxmode.max_rx_pkt_len);
> }
>
> + if (!strncmp(lgopts[option_index].name,
> + "vlan-strip", 10)) {
> + printf("set vlan strip\n");
> + port_conf.rxmode.offloads =
> + DEV_RX_OFFLOAD_VLAN_STRIP;
> + }
> +
> + if (!strncmp(lgopts[option_index].name,
> + "rx-none", 7)) {
> + printf("none of DCB,RSS or VMDQ mode\n");
> + port_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
> + }
> +
> if (!strncmp(lgopts[option_index].name,
>CMD_LINE_OPT_PARSE_PTYPE,
>sizeof(CMD_LINE_OPT_PARSE_PTYPE))) {
>
Why not just rely on the capabilities exposed by the driver?
Maxime
Re: [dpdk-dev] [PATCH 5/5] examples/l3fwd-power: support virtio/vhost
Got it. I will change the codes and add the port configuration updating according to the device information. Thanks! Miao > -Original Message- > From: David Marchand > Sent: Friday, September 10, 2021 4:50 PM > To: Li, Miao > Cc: Maxime Coquelin ; dev@dpdk.org; Xia, > Chenbo > Subject: Re: [dpdk-dev] [PATCH 5/5] examples/l3fwd-power: support > virtio/vhost > > On Fri, Sep 10, 2021 at 10:34 AM Li, Miao wrote: > > > > In l3fwd-power, there is default port configuration which requires RSS and > IPV4/UDP/TCP checksum. Once device does not support these, the l3fwd- > power will exit and report an error. > > Maxime suggested to update the port configuration based on its capabilities. > > For RSS, it would be something like what I proposed for OVS: > https://patchwork.ozlabs.org/project/openvswitch/patch/20210830101304.1 > 3689-1-david.march...@redhat.com/ > > As for IPv4/UDP/TCP rx checksums, I am not sure there is any actual > requirement for this app. > Probably something to investigate wrt DO_RFC_1812_CHECKS. > > > -- > David Marchand
Re: [dpdk-dev] [PATCH 1/5] net/virtio: implement rte_power_monitor API
Hi chenbo
> -Original Message-
> From: Xia, Chenbo
> Sent: Wednesday, September 15, 2021 4:45 PM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH 1/5] net/virtio: implement rte_power_monitor API
>
> Hi Miao,
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, September 10, 2021 9:06 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com;
> > Li, Miao
> > Subject: [PATCH 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
> > ---
> > drivers/net/virtio/virtio_ethdev.c | 57
> > ++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index e58085a2c9..4ce49936f5 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -73,6 +73,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,
> > @@ -975,6 +977,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,
> > };
> >
> > /*
> > @@ -1306,6 +1309,60 @@ 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
> > +static int
> > +virtio_packed_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];
> > +
> > + return (value & m) == v ? -1 : 0;
> > +}
> > +
> > +static int
> > +virtio_split_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];
> > +
> > + 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 = vq->hw;
> > + if (vq == NULL)
> > + return -EINVAL;
> > + 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->fn = virtio_packed_monitor_callback;
> > + pmc->size = sizeof(uint16_t);
>
> I suggest to use sizeof(desc[vq->vq_used_cons_idx].flags) or
> sizeof(desc->flags)
> in case the flag type changes.
Thanks for your suggestion. I will fix it in the next version.
>
> > + } 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->fn = virtio_split_monitor_callback;
> > + pmc->size = sizeof(uint16_t);
>
> Same here.
I will fix it in the next version, too.
Thanks,
Miao
>
> Thanks,
> Chenbo
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int
> > on) {
> > --
> > 2.25.1
Re: [dpdk-dev] [PATCH 2/5] lib/vhost: implement rte_power_monitor API
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Wednesday, September 15, 2021 4:52 PM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH 2/5] lib/vhost: implement rte_power_monitor API
>
> Hi Miao,
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, September 10, 2021 9:06 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com;
> > Li, Miao
> > 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
> > ---
> > 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
>
RE: [PATCH v1] net/vhost: add queue status check
Hi > -Original Message- > From: Maxime Coquelin > Sent: Tuesday, November 16, 2021 5:36 PM > To: Li, Miao ; dev@dpdk.org > Cc: Xia, Chenbo > Subject: Re: [PATCH v1] net/vhost: add queue status check > > > > On 11/16/21 10:34, Maxime Coquelin wrote: > > > > > > On 11/16/21 17:44, Miao Li wrote: > >> This patch adds queue status check to make sure that vhost monitor > >> address will not be got until the link between backend and frontend > > s/got/gone/? > >> up and the packets are allowed to be queued. > > > > It needs a fixes tag. If we don't add this check, rte_vhost_get_monitor_addr will return -EINVAL when check if dev is null. But before return, get_device() will be called and print error log "device not found". So we want to add this check and return -EINVAL before call rte_vhost_get_monitor_addr. If we don't add this check, the vhost monitor address will also not be got but vhost will print error log continuously. It have no function impact, so I think it is not a fix. > > > >> Signed-off-by: Miao Li > >> --- > >> drivers/net/vhost/rte_eth_vhost.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/net/vhost/rte_eth_vhost.c > >> b/drivers/net/vhost/rte_eth_vhost.c > >> index 070f0e6dfd..9d600054d8 100644 > >> --- a/drivers/net/vhost/rte_eth_vhost.c > >> +++ b/drivers/net/vhost/rte_eth_vhost.c > >> @@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct > >> rte_power_monitor_cond *pmc) > >> int ret; > >> if (vq == NULL) > >> return -EINVAL; > >> + if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0)) > >> + return -EINVAL; > > Also, EINVAL might not be the right return value here. I don't know which return value will be better. Do you have any suggestions? Thanks! > > > How does it help? > > What does prevent allow_queuing to become zero between the check and the > > call to rte_vhost_get_monitor_addr? This check will prevent vhost to print error log continuously. > > > > I think you need to implement the same logic as in eth_vhost_rx(), i.e. > > check allow_queueing, set while_queueing, check allow_queueing, do your > > stuff and clear while_queuing. I think the while_queuing is unnecessary because we only read the value in vq and this API will only be called as a callback of RX. Thanks, Miao > > > >> ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id, > >> &vhost_pmc); > >> if (ret < 0) > >> > > > > Maxime
RE: [PATCH v1 4/4] bus/pci: add VFIO sparse mmap support
Hi,
I will add errno print in patch v3.
> -Original Message-
> From: Stephen Hemminger
> Sent: Monday, May 15, 2023 11:53 PM
> To: Li, Miao
> Cc: dev@dpdk.org; sk...@marvell.com; tho...@monjalon.net;
> david.march...@redhat.com; ferruh.yi...@amd.com; Xia, Chenbo
> ; Cao, Yahui ; Burakov, Anatoly
>
> Subject: Re: [PATCH v1 4/4] bus/pci: add VFIO sparse mmap support
>
> On Mon, 15 May 2023 06:47:00 +
> Miao Li wrote:
>
> > + map_addr = pci_map_resource(addr,
> vfio_dev_fd,
> > + bar->offset + sparse->offset, sparse-
> >size,
> > + RTE_MAP_FORCE_ADDRESS);
> > + if (map_addr == NULL) {
> > + munmap(bar_addr, bar->size);
> > + RTE_LOG(ERR, EAL, "Failed to map pci
> BAR%d\n",
> > + bar_index);
>
> If mmap() fails then printing errno would help diagnose why.
RE: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process
Hi,
> -Original Message-
> From: David Marchand
> Sent: Monday, July 3, 2023 3:48 PM
> To: Li, Miao
> Cc: dev@dpdk.org; sta...@dpdk.org; Maxime Coquelin
> ; Xia, Chenbo
> Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in
> secondary process
>
> On Thu, Jun 29, 2023 at 4:27 AM Miao Li wrote:
> >
> > When doing IO port map for legacy device in secondary process,
> > vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd is
> > missing. So, in secondary process, rte_pci_map_device is added for
> > legacy device to setup vfio_cfg and fill in region info like in
> > primary process.
>
> I think, in legacy mode, there is no PCI mappable memory.
> So there should be no need for this call to rte_pci_map_device.
>
> What is missing is a vfio setup, is this correct?
> I'd rather see this issue be fixed in the pci_vfio_ioport_map() function.
>
If adding vfio setup in the pci_vfio_ioport_map() function, vfio will be setup
twice in primary process because rte_pci_map_device will be called for legacy
device in primary process.
I add IO port region check to skip region map in the next patch.
>
> >> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI
> >> ethdev init")
>
> This commit only moved code, and at this point, there was no need for a call
> to
> rte_pci_map_device in the secondary process case.
> It seems unlikely this is a faulty change.
>
> The recent addition on the vfio side seems a better culprit, but I am fine
> with
> being proven wrong :-).
>
Yes, the fix commit is wrong, but not the recent addition commit on the vfio
side. Because the root cause is missing a vfio setup. After adding recent
addition commit, the uninitialized vfio_cfg info(like vfio_dev_fd, region info)
will be used so this bug will be found. I think the correct fix commit will be
6d890f8ab512("net/virtio: fix multiple process support").
>
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Miao Li
>
>
> --
> David Marchand
Thanks,
Miao Li
Re: [dpdk-dev] [PATCH v3 2/5] vhost: implement rte_power_monitor API
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Wednesday, September 29, 2021 11:01 AM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH v3 2/5] vhost: implement rte_power_monitor API
>
> Hi Miao,
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, September 24, 2021 6:23 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li,
> Miao
> >
> > 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
> > ---
> > 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
Re: [dpdk-dev] [PATCH v3 4/5] power: modify return of queue_stopped
Hi Chenbo,
> -Original Message-
> From: Xia, Chenbo
> Sent: Wednesday, September 29, 2021 11:03 AM
> To: Li, Miao ; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH v3 4/5] power: modify return of queue_stopped
>
> > -Original Message-
> > From: Li, Miao
> > Sent: Friday, September 24, 2021 6:23 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li,
> Miao
> >
> > Subject: [PATCH v3 4/5] power: modify return of queue_stopped
> >
> > Since some vdevs like virtio and vhost do not support rxq_info_get and
> > queue state inquiry, the error return value -ENOTSUP need to be ignored
> > when queue_stopped cannot get rx queue information and rx queue state.
> > This patch changes the return value of queue_stopped when
> > rte_eth_rx_queue_info_get return ENOTSUP to support vdevs which cannot
> > provide rx queue information and rx queue state enable power management.
> >
> > Signed-off-by: Miao Li
> > ---
> > lib/power/rte_power_pmd_mgmt.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/power/rte_power_pmd_mgmt.c
> b/lib/power/rte_power_pmd_mgmt.c
> > index 0ce40f0875..39a2b4cd23 100644
> > --- a/lib/power/rte_power_pmd_mgmt.c
> > +++ b/lib/power/rte_power_pmd_mgmt.c
> > @@ -382,8 +382,13 @@ queue_stopped(const uint16_t port_id, const
> uint16_t
> > queue_id)
> > {
> > struct rte_eth_rxq_info qinfo;
> >
> > - if (rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo) < 0)
> > - return -1;
> > + int ret = rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo);
> > + if (ret < 0) {
> > + if (ret == -ENOTSUP)
> > + return 1;
> > + else
> > + return -1;
> > + }
> >
> > return qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED;
> > }
> > --
> > 2.25.1
>
> Anatoly's ACK is missed.
I will add it in the next version.
Thanks,
Miao
Re: [dpdk-dev] [PATCH v3 5/5] examples/l3fwd-power: support virtio/vhost
Hi Chenbo, > -Original Message- > From: Xia, Chenbo > Sent: Wednesday, September 29, 2021 2:53 PM > To: Li, Miao ; dev@dpdk.org > Cc: maxime.coque...@redhat.com; david.march...@redhat.com > Subject: RE: [PATCH v3 5/5] examples/l3fwd-power: support virtio/vhost > > > -Original Message- > > From: Li, Miao > > Sent: Friday, September 24, 2021 6:23 PM > > To: dev@dpdk.org > > Cc: Xia, Chenbo ; maxime.coque...@redhat.com; Li, > Miao > > > > Subject: [PATCH v3 5/5] examples/l3fwd-power: support virtio/vhost > > > > In l3fwd-power, there is default port configuration which requires > > RSS and IPV4/UDP/TCP checksum. Once device does not support these, > > the l3fwd-power will exit and report an error. > > This patch updates the port configuration based on device capabilities > > after getting the device information to support devices like virtio > > and vhost. > > > > Signed-off-by: Miao Li > > --- > > examples/l3fwd-power/main.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c > > index aa7b8db44a..14ae87a9d5 100644 > > --- a/examples/l3fwd-power/main.c > > +++ b/examples/l3fwd-power/main.c > > @@ -2637,6 +2637,11 @@ main(int argc, char **argv) > > local_port_conf.rx_adv_conf.rss_conf.rss_hf); > > } > > > > + if (local_port_conf.rx_adv_conf.rss_conf.rss_hf == 0) > > + local_port_conf.rxmode.mq_mode = > ETH_MQ_RX_NONE; > > + local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa; > > + port_conf.rxmode.offloads = local_port_conf.rxmode.offloads; > > + > > ret = rte_eth_dev_configure(portid, nb_rx_queue, > > (uint16_t)n_tx_queue, > &local_port_conf); > > if (ret < 0) > > -- > > 2.25.1 > > Just as David suggested, I think we'd better consider RFC 1812, if HW does not > support RX IP CKSUM offload, check IP CKSUM in is_valid_ipv4_pkt. I will fix it in the next version. Thanks, Miao > > Thanks, > Chenbo
RE: [PATCH v1] power: add wakeup log
Hi, > -Original Message- > From: Hunt, David > Sent: Wednesday, February 23, 2022 12:32 AM > To: Li, Miao ; dev@dpdk.org > Cc: Wang, Yinan ; step...@networkplumber.org > Subject: Re: [PATCH v1] power: add wakeup log > > > On 22/2/2022 1:52 PM, Miao Li wrote: > > This patch adds a log in rte_power_monitor to show the core has been > > waked up. > > > > Signed-off-by: Miao Li > > --- > > lib/eal/x86/rte_power_intrinsics.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/eal/x86/rte_power_intrinsics.c > b/lib/eal/x86/rte_power_intrinsics.c > > index f749da9b85..dd63e2b6eb 100644 > > --- a/lib/eal/x86/rte_power_intrinsics.c > > +++ b/lib/eal/x86/rte_power_intrinsics.c > > @@ -128,6 +128,14 @@ rte_power_monitor(const struct > rte_power_monitor_cond *pmc, > > : "D"(0), /* enter C0.2 */ > > "a"(tsc_l), "d"(tsc_h)); > > > > + cur_value = __get_umwait_val(pmc->addr, pmc->size); > > + > > + /* check if core has been waked up by changing monitoring value */ > > + if (pmc->fn(cur_value, pmc->opaque) != 0) > > + RTE_LOG(INFO, EAL, > > + "lcore %u is waked up from value change\n", > > + rte_lcore_id()); > > + > > end: > > /* erase sleep address */ > > rte_spinlock_lock(&s->lock); > > > Hi Li, > > If I'm not mistaken, a similar patch was added to a previous DPDK > release and then removed because of the enormous performance impact. > > This looks to be something similar, and it's adding a log message to a > low-level function. Also, as mentioned before, the intention in the > future is to call this function much more agressively, so there would be > hundreds of thousands of messages every second. > > We cannot add an RTE_LOG here. Please rework and put the log in the test > case instead. I add a judgment before the output. When no packet arriver, the log will not be printed. When a lot of packets arriver, the rte_power_monitor will not be called. So I think the performance impact is small. > > Also, regarding the wording, I would suggest "lcore %u awoke due to > monitor address value change\n" I will change the log content in next version. > > Rgds, > > Dave. > Thanks, Miao
RE: [PATCH v1] power: add wakeup log
Hi, > -Original Message- > From: Stephen Hemminger > Sent: Wednesday, February 23, 2022 12:07 AM > To: Li, Miao > Cc: dev@dpdk.org; Hunt, David ; Wang, Yinan > > Subject: Re: [PATCH v1] power: add wakeup log > > On Tue, 22 Feb 2022 13:52:27 + > Miao Li wrote: > > > + "lcore %u is waked up from value change\n", > > That is awkward phrasing in English and should be DEBUG not INFO level > because it may happen often. I will fix it in the next version. > > Maybe: > "lcore %u awoke because value changed\n" > or something better. I will change the log content in next version. > > PS: sometimes it is good idea to using grammar tools when wording commit > or messages. Thanks, Miao
