Re: [ovs-dev] [PATCH v10 1/3] netdev: Add optional qfill output parameter to rxq_recv()
> > @@ -1846,11 +1846,24 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> > batch->count = nb_rx;
> > dp_packet_batch_init_packet_fields(batch);
> >
> > +if (qfill) {
> > +if (nb_rx == NETDEV_MAX_BURST) {
> > +/* The DPDK API returns a uint32_t which often has invalid
> > bits in
> > + * the upper 16-bits. Need to restrict the value to uint16_t.
> > */
> > +*qfill = rte_vhost_rx_queue_count(netdev_dpdk_get_vid(dev),
>
> I lost count of how many times I talked about this. Please, don't obtain the
> 'vid' twice. You have to check the result of 'netdev_dpdk_get_vid()' always.
> Otherwise this could lead to crash.
>
> Details, as usual, here:
> daf22bf7a826 ("netdev-dpdk: Fix calling vhost API with negative vid.")
>
> I believe, that I already wrote this comment to one of the previous versions
> of this patch-set.
Yes, sorry I missed that one. I will for sure fix it.
As this is fairly non-obvious from looking at the code it might be a good idea
to add some warning comment to the function 'netdev_dpdk_get_vid()' and/or the
places where it is used from the PMD.
/Jan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10 1/3] netdev: Add optional qfill output parameter to rxq_recv()
Oh, how I missed that...
Comment inline.
Best regards, Ilya Maximets.
On 18.03.2018 20:55, Jan Scheurich wrote:
> If the caller provides a non-NULL qfill pointer and the netdev
> implemementation supports reading the rx queue fill level, the rxq_recv()
> function returns the remaining number of packets in the rx queue after
> reception of the packet burst to the caller. If the implementation does
> not support this, it returns -ENOTSUP instead. Reading the remaining queue
> fill level should not substantilly slow down the recv() operation.
>
> A first implementation is provided for ethernet and vhostuser DPDK ports
> in netdev-dpdk.c.
>
> This output parameter will be used in the upcoming commit for PMD
> performance metrics to supervise the rx queue fill level for DPDK
> vhostuser ports.
>
> Signed-off-by: Jan Scheurich
> Acked-by: Billy O'Mahony
> ---
> lib/dpif-netdev.c | 2 +-
> lib/netdev-bsd.c | 8 +++-
> lib/netdev-dpdk.c | 25 +++--
> lib/netdev-dummy.c| 8 +++-
> lib/netdev-linux.c| 7 ++-
> lib/netdev-provider.h | 7 ++-
> lib/netdev.c | 5 +++--
> lib/netdev.h | 3 ++-
> 8 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index b07fc6b..86d8739 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3276,7 +3276,7 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread
> *pmd,
> pmd->ctx.last_rxq = rxq;
> dp_packet_batch_init(&batch);
>
> -error = netdev_rxq_recv(rxq->rx, &batch);
> +error = netdev_rxq_recv(rxq->rx, &batch, NULL);
> if (!error) {
> /* At least one packet received. */
> *recirc_depth_get() = 0;
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 05974c1..b70f327 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -618,7 +618,8 @@ netdev_rxq_bsd_recv_tap(struct netdev_rxq_bsd *rxq,
> struct dp_packet *buffer)
> }
>
> static int
> -netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> +netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> +int *qfill)
> {
> struct netdev_rxq_bsd *rxq = netdev_rxq_bsd_cast(rxq_);
> struct netdev *netdev = rxq->up.netdev;
> @@ -643,6 +644,11 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct
> dp_packet_batch *batch)
> batch->packets[0] = packet;
> batch->count = 1;
> }
> +
> +if (qfill) {
> +*qfill = -ENOTSUP;
> +}
> +
> return retval;
> }
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index af9843a..66f2439 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1808,7 +1808,7 @@ netdev_dpdk_vhost_update_rx_counters(struct
> netdev_stats *stats,
> */
> static int
> netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> - struct dp_packet_batch *batch)
> + struct dp_packet_batch *batch, int *qfill)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
> @@ -1846,11 +1846,24 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +/* The DPDK API returns a uint32_t which often has invalid bits
> in
> + * the upper 16-bits. Need to restrict the value to uint16_t. */
> +*qfill = rte_vhost_rx_queue_count(netdev_dpdk_get_vid(dev),
I lost count of how many times I talked about this. Please, don't obtain the
'vid' twice. You have to check the result of 'netdev_dpdk_get_vid()' always.
Otherwise this could lead to crash.
Details, as usual, here:
daf22bf7a826 ("netdev-dpdk: Fix calling vhost API with negative vid.")
I believe, that I already wrote this comment to one of the previous versions
of this patch-set.
> + qid * VIRTIO_QNUM + VIRTIO_TXQ)
> +& UINT16_MAX;
> +} else {
> +*qfill = 0;
> +}
> +}
> +
> return 0;
> }
>
> static int
> -netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
> +netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch,
> + int *qfill)
> {
> struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq);
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> @@ -1887,6 +1900,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct
> dp_packet_batch *batch)
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +*qfill = rte_eth_rx_queue_count(rx->port_id, rxq->queue_id);
> +} else {
> +*qfill = 0;
> +}
> +}
> +
Re: [ovs-dev] [PATCH v10 1/3] netdev: Add optional qfill output parameter to rxq_recv()
Still don't like the general way of implementation of the patch-set,
but, I guess, we could live with that for some time while thinking
about rework.
One minor comment inline.
Sorry again for late responses.
Best regards, Ilya Maximets.
On 18.03.2018 20:55, Jan Scheurich wrote:
> If the caller provides a non-NULL qfill pointer and the netdev
> implemementation supports reading the rx queue fill level, the rxq_recv()
> function returns the remaining number of packets in the rx queue after
> reception of the packet burst to the caller. If the implementation does
> not support this, it returns -ENOTSUP instead. Reading the remaining queue
> fill level should not substantilly slow down the recv() operation.
>
> A first implementation is provided for ethernet and vhostuser DPDK ports
> in netdev-dpdk.c.
>
> This output parameter will be used in the upcoming commit for PMD
> performance metrics to supervise the rx queue fill level for DPDK
> vhostuser ports.
>
> Signed-off-by: Jan Scheurich
> Acked-by: Billy O'Mahony
> ---
> lib/dpif-netdev.c | 2 +-
> lib/netdev-bsd.c | 8 +++-
> lib/netdev-dpdk.c | 25 +++--
> lib/netdev-dummy.c| 8 +++-
> lib/netdev-linux.c| 7 ++-
> lib/netdev-provider.h | 7 ++-
> lib/netdev.c | 5 +++--
> lib/netdev.h | 3 ++-
> 8 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index b07fc6b..86d8739 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3276,7 +3276,7 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread
> *pmd,
> pmd->ctx.last_rxq = rxq;
> dp_packet_batch_init(&batch);
>
> -error = netdev_rxq_recv(rxq->rx, &batch);
> +error = netdev_rxq_recv(rxq->rx, &batch, NULL);
> if (!error) {
> /* At least one packet received. */
> *recirc_depth_get() = 0;
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 05974c1..b70f327 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -618,7 +618,8 @@ netdev_rxq_bsd_recv_tap(struct netdev_rxq_bsd *rxq,
> struct dp_packet *buffer)
> }
>
> static int
> -netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> +netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> +int *qfill)
> {
> struct netdev_rxq_bsd *rxq = netdev_rxq_bsd_cast(rxq_);
> struct netdev *netdev = rxq->up.netdev;
> @@ -643,6 +644,11 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct
> dp_packet_batch *batch)
> batch->packets[0] = packet;
> batch->count = 1;
> }
> +
> +if (qfill) {
> +*qfill = -ENOTSUP;
> +}
> +
> return retval;
> }
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index af9843a..66f2439 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1808,7 +1808,7 @@ netdev_dpdk_vhost_update_rx_counters(struct
> netdev_stats *stats,
> */
> static int
> netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> - struct dp_packet_batch *batch)
> + struct dp_packet_batch *batch, int *qfill)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
> @@ -1846,11 +1846,24 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +/* The DPDK API returns a uint32_t which often has invalid bits
> in
> + * the upper 16-bits. Need to restrict the value to uint16_t. */
> +*qfill = rte_vhost_rx_queue_count(netdev_dpdk_get_vid(dev),
> + qid * VIRTIO_QNUM + VIRTIO_TXQ)
> +& UINT16_MAX;
> +} else {
> +*qfill = 0;
> +}
> +}
> +
> return 0;
> }
>
> static int
> -netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
> +netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch,
> + int *qfill)
> {
> struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq);
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> @@ -1887,6 +1900,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct
> dp_packet_batch *batch)
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +*qfill = rte_eth_rx_queue_count(rx->port_id, rxq->queue_id);
> +} else {
> +*qfill = 0;
> +}
> +}
> +
> return 0;
> }
>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 8af9e1a..13bc580 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -992,7 +992,8 @@ netdev_dummy_rxq_dealloc(stru
Re: [ovs-dev] [PATCH v10 1/3] netdev: Add optional qfill output parameter to rxq_recv()
Acked-by: Billy O'Mahony
> -Original Message-
> From: Jan Scheurich [mailto:jan.scheur...@ericsson.com]
> Sent: Sunday, March 18, 2018 5:55 PM
> To: d...@openvswitch.org
> Cc: ktray...@redhat.com; Stokes, Ian ;
> i.maxim...@samsung.com; O Mahony, Billy ; Jan
> Scheurich
> Subject: [PATCH v10 1/3] netdev: Add optional qfill output parameter to
> rxq_recv()
>
> If the caller provides a non-NULL qfill pointer and the netdev
> implemementation
> supports reading the rx queue fill level, the rxq_recv() function returns the
> remaining number of packets in the rx queue after reception of the packet
> burst
> to the caller. If the implementation does not support this, it returns
> -ENOTSUP
> instead. Reading the remaining queue fill level should not substantilly slow
> down
> the recv() operation.
>
> A first implementation is provided for ethernet and vhostuser DPDK ports in
> netdev-dpdk.c.
>
> This output parameter will be used in the upcoming commit for PMD
> performance metrics to supervise the rx queue fill level for DPDK vhostuser
> ports.
>
> Signed-off-by: Jan Scheurich
> Acked-by: Billy O'Mahony
> ---
> lib/dpif-netdev.c | 2 +-
> lib/netdev-bsd.c | 8 +++-
> lib/netdev-dpdk.c | 25 +++--
> lib/netdev-dummy.c| 8 +++-
> lib/netdev-linux.c| 7 ++-
> lib/netdev-provider.h | 7 ++-
> lib/netdev.c | 5 +++--
> lib/netdev.h | 3 ++-
> 8 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index b07fc6b..86d8739
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3276,7 +3276,7 @@ dp_netdev_process_rxq_port(struct
> dp_netdev_pmd_thread *pmd,
> pmd->ctx.last_rxq = rxq;
> dp_packet_batch_init(&batch);
>
> -error = netdev_rxq_recv(rxq->rx, &batch);
> +error = netdev_rxq_recv(rxq->rx, &batch, NULL);
> if (!error) {
> /* At least one packet received. */
> *recirc_depth_get() = 0;
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 05974c1..b70f327 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -618,7 +618,8 @@ netdev_rxq_bsd_recv_tap(struct netdev_rxq_bsd *rxq,
> struct dp_packet *buffer) }
>
> static int
> -netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> +netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> +int *qfill)
> {
> struct netdev_rxq_bsd *rxq = netdev_rxq_bsd_cast(rxq_);
> struct netdev *netdev = rxq->up.netdev; @@ -643,6 +644,11 @@
> netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> batch->packets[0] = packet;
> batch->count = 1;
> }
> +
> +if (qfill) {
> +*qfill = -ENOTSUP;
> +}
> +
> return retval;
> }
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index af9843a..66f2439
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1808,7 +1808,7 @@ netdev_dpdk_vhost_update_rx_counters(struct
> netdev_stats *stats,
> */
> static int
> netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> - struct dp_packet_batch *batch)
> + struct dp_packet_batch *batch, int *qfill)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
> @@ -1846,11 +1846,24 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq
> *rxq,
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +/* The DPDK API returns a uint32_t which often has invalid bits
> in
> + * the upper 16-bits. Need to restrict the value to uint16_t. */
> +*qfill = rte_vhost_rx_queue_count(netdev_dpdk_get_vid(dev),
> + qid * VIRTIO_QNUM + VIRTIO_TXQ)
> +& UINT16_MAX;
> +} else {
> +*qfill = 0;
> +}
> +}
> +
> return 0;
> }
>
> static int
> -netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
> +netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch,
> + int *qfill)
> {
> struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq);
> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev); @@ -1887,6
> +1900,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct
> dp_packet_batch *batch)
> batch->count = nb_rx;
> dp_packet_batch_init_packet_fields(batch);
>
> +if (qfill) {
> +if (nb_rx == NETDEV_MAX_BURST) {
> +*qfill = rte_eth_rx_queue_count(rx->port_id, rxq->queue_id);
> +} else {
> +*qfill = 0;
> +}
> +}
> +
> return 0;
> }
>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 8af9e1a..13bc580
> 100644
> --- a/lib/netdev-
