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

Message ID 1516969207-17326-2-git-send-email-jan.scheurich@ericsson.com
State New
Delegated to: Ian Stokes
Headers show
Series
  • dpif-netdev: Detailed PMD performance metrics and supervision
Related show

Commit Message

Jan Scheurich Jan. 26, 2018, 12:20 p.m.
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@ericsson.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(-)

Comments

Billy O'Mahony Jan. 26, 2018, 3:46 p.m. | #1
LGTM but one thing I don't understand down below...

> -----Original Message-----
> From: Jan Scheurich [mailto:jan.scheurich@ericsson.com]
> Sent: Friday, January 26, 2018 12:20 PM
> To: dev@openvswitch.org
> Cc: ktraynor@redhat.com; Stokes, Ian <ian.stokes@intel.com>;
> i.maximets@samsung.com; O Mahony, Billy <billy.o.mahony@intel.com>;
> Jan Scheurich <jan.scheurich@ericsson.com>
> Subject: [PATCH v8 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 <jan.scheurich@ericsson.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 ba62128..4a0fcbd
> 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 ac2e38e..bb7dece
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1757,7 +1757,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);
> @@ -1795,11 +1795,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;
[[BO'M]] Why the += operator?
> +        } 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); @@ -1836,6
> +1849,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);
[[BO'M]] Why the += operator?
> +        } else {
> +            *qfill = 0;
> +        }
> +    }
> +
>      return 0;
>  }
> 
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index
> 4246af3..229cf96 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 6dae796..5f895a0
> 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1129,7 +1129,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; @@ -1158,6 +1159,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 be05dc6..71035b2 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -694,11 +694,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 *);
> 
> --
> 1.9.1
Jan Scheurich Jan. 26, 2018, 4:05 p.m. | #2
> > @@ -1795,11 +1795,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;
> [[BO'M]] Why the += operator?

Oops: This a leftover from an intermediate version where I returned the total qlen including the received packets. Forgot this when I changed the semantics of the output parameter.

> > +        } 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); @@ -1836,6
> > +1849,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);
> [[BO'M]] Why the += operator?

Same as above.

BR, Jan

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index ba62128..4a0fcbd 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 ac2e38e..bb7dece 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1757,7 +1757,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);
@@ -1795,11 +1795,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);
@@ -1836,6 +1849,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 4246af3..229cf96 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 6dae796..5f895a0 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1129,7 +1129,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;
@@ -1158,6 +1159,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 be05dc6..71035b2 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -694,11 +694,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 *);