[ovs-dev] [PATCH v10 1/3] netdev: Add optional qfill output parameter to rxq_recv()

Ilya Maximets i.maximets at samsung.com
Fri Apr 6 13:38:29 UTC 2018


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 <jan.scheurich at ericsson.com>
> Acked-by: Billy O'Mahony <billy.o.mahony at intel.com>
> ---
>  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;
> +        }
> +    }
> +
>      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(struct netdev_rxq *rxq_)
>  }
>  
>  static int
> -netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> +netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> +                      int *qfill)
>  {
>      struct netdev_rxq_dummy *rx = netdev_rxq_dummy_cast(rxq_);
>      struct netdev_dummy *netdev = netdev_dummy_cast(rx->up.netdev);
> @@ -1037,6 +1038,11 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
>  
>      batch->packets[0] = packet;
>      batch->count = 1;
> +
> +    if (qfill) {
> +        *qfill = -ENOTSUP;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 7ea40a8..c179ce2 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1132,7 +1132,8 @@ netdev_linux_rxq_recv_tap(int fd, struct dp_packet *buffer)
>  }
>  
>  static int
> -netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
> +netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> +                      int *qfill)
>  {
>      struct netdev_rxq_linux *rx = netdev_rxq_linux_cast(rxq_);
>      struct netdev *netdev = rx->up.netdev;
> @@ -1161,6 +1162,10 @@ netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch)
>          dp_packet_batch_init_packet(batch, buffer);
>      }
>  
> +    if (qfill) {
> +        *qfill = -ENOTSUP;
> +    }
> +
>      return retval;
>  }
>  
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index 25bd671..37add95 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -786,12 +786,17 @@ struct netdev_class {
>       * pointers and metadata itself, if desired, e.g. with pkt_metadata_init()
>       * and miniflow_extract().
>       *
> +     * If the caller provides a non-NULL qfill pointer, the implementation
> +     * returns the remaining rx queue fill level (zero or more) after the
> +     * reception of packets, if it supports that, or -ENOTSUP otherwise.
> +     *
>       * Implementations should allocate buffers with DP_NETDEV_HEADROOM bytes of
>       * headroom.
>       *
>       * Returns EAGAIN immediately if no packet is ready to be received or
>       * another positive errno value if an error was encountered. */
> -    int (*rxq_recv)(struct netdev_rxq *rx, struct dp_packet_batch *batch);
> +    int (*rxq_recv)(struct netdev_rxq *rx, struct dp_packet_batch *batch,
> +                    int *qfill);
>  
>      /* Registers with the poll loop to wake up from the next call to
>       * poll_block() when a packet is ready to be received with
> diff --git a/lib/netdev.c b/lib/netdev.c
> index b303a7d..fe646cc 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -695,11 +695,12 @@ netdev_rxq_close(struct netdev_rxq *rx)
>   * Returns EAGAIN immediately if no packet is ready to be received or another
>   * positive errno value if an error was encountered. */
>  int
> -netdev_rxq_recv(struct netdev_rxq *rx, struct dp_packet_batch *batch)
> +netdev_rxq_recv(struct netdev_rxq *rx, struct dp_packet_batch *batch,
> +                int *qfill)
>  {
>      int retval;
>  
> -    retval = rx->netdev->netdev_class->rxq_recv(rx, batch);
> +    retval = rx->netdev->netdev_class->rxq_recv(rx, batch, qfill);
>      if (!retval) {
>          COVERAGE_INC(netdev_received);
>      } else {
> diff --git a/lib/netdev.h b/lib/netdev.h
> index ff1b604..3f51634 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -175,7 +175,8 @@ void netdev_rxq_close(struct netdev_rxq *);
>  const char *netdev_rxq_get_name(const struct netdev_rxq *);
>  int netdev_rxq_get_queue_id(const struct netdev_rxq *);
>  
> -int netdev_rxq_recv(struct netdev_rxq *rx, struct dp_packet_batch *);
> +int netdev_rxq_recv(struct netdev_rxq *rx, struct dp_packet_batch *,
> +                    int *qfill);
>  void netdev_rxq_wait(struct netdev_rxq *);
>  int netdev_rxq_drain(struct netdev_rxq *);
>  
> 


More information about the dev mailing list