[ovs-dev,v2,1/4] netdev-dpdk.c: Support the multi-queue QoS configuration for dpdk datapath

Message ID 1500951228-18646-2-git-send-email-zhaozhanxu@163.com
State New
Headers show

Commit Message

zhao zhaozhanxu July 25, 2017, 2:53 a.m.
This patch adds similar QoS configration with kernel datapath.

It adds some functions whitch is pointed by `get_queue`, `set_queue`
and `delete_queue` of structure `netdev_class` to support configuration.

Then the configuration command changed from command A(see below) to
command B, but it only support to configure and rate limitation function
is not ready now.

Command A: (original command)

$ ovs-vsctl set port vhost-user0 qos=@newqos -- \
    --id=@newqos create qos type=egress-policer \
    other-config:cir=46000000 other-config:cbs=2048`

Command B: (new command)

$ ovs-vsctl set port vhost-user0 qos=@newqos -- \
    --id=@newqos create qos type=egress-policer \
    other-config:cir=46000000 other-config:cbs=2048 \
    queues:123=@q123 queues:234=@q234 -- \
    --id=@q123 create queue \
    other-config:cir=12800000 other-config:cbs=2048 -- \
    --id=@q234 create queue \
    other-config:cir=25600000 other-config:cbs=2048`

Signed-off-by: zhaozhanxu <zhaozhanxu@163.com>
---
 lib/netdev-dpdk.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 193 insertions(+), 4 deletions(-)

Comments

Stokes, Ian Oct. 17, 2017, 8:52 a.m. | #1
> This patch adds similar QoS configration with kernel datapath.
> 
> It adds some functions whitch is pointed by `get_queue`, `set_queue` and
> `delete_queue` of structure `netdev_class` to support configuration.
> 
> Then the configuration command changed from command A(see below) to
> command B, but it only support to configure and rate limitation function
> is not ready now.
> 
> Command A: (original command)
> 
> $ ovs-vsctl set port vhost-user0 qos=@newqos -- \
>     --id=@newqos create qos type=egress-policer \
>     other-config:cir=46000000 other-config:cbs=2048`
> 
> Command B: (new command)
> 
> $ ovs-vsctl set port vhost-user0 qos=@newqos -- \
>     --id=@newqos create qos type=egress-policer \
>     other-config:cir=46000000 other-config:cbs=2048 \
>     queues:123=@q123 queues:234=@q234 -- \
>     --id=@q123 create queue \
>     other-config:cir=12800000 other-config:cbs=2048 -- \
>     --id=@q234 create queue \
>     other-config:cir=25600000 other-config:cbs=2048`

It's probably clearer in the code below, but does this break backwards compatibility with the previous implementation of the egress policer? I.e. after this patchset can a user still use method A to setup a policer on a per port basis rather than a per queue basis for that port? 
> 
> Signed-off-by: zhaozhanxu <zhaozhanxu@163.com>
> ---
>  lib/netdev-dpdk.c | 197
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 193 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ea17b97..089ad64
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -259,6 +259,31 @@ struct dpdk_qos_ops {
>       */
>      int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
>                     int pkt_cnt);
> +
> +    /* Called to construct a QoS Queue. The implementation should make
> +     * the appropriate calls to configure QoS Queue according to
> 'details'.
> +     *
> +     * The contents of 'details' should be documented as valid for
> 'ovs_name'
> +     * in the "other_config" column in the "QoS" table in
> vswitchd/vswitch.xml
> +     * (which is built as ovs-vswitchd.conf.db(8)).
> +     *
> +     * This function must return 0 if and only if it constructs
> +     * qos queue successfully.
> +     */
> +    int (*qos_queue_construct)(const struct smap *details,
> +                               uint32_t queue_id, struct qos_conf
> + *conf);
> +
> +    /* Destroys the Qos Queue */
> +    void (*qos_queue_destruct)(struct qos_conf *conf, uint32_t
> + queue_id);
> +
> +    /* Retrieves details of QoS Queue configuration into 'details'.
> +     *
> +     * The contents of 'details' should be documented as valid for
> 'ovs_name'
> +     * in the "other_config" column in the "QoS" table in
> vswitchd/vswitch.xml
> +     * (which is built as ovs-vswitchd.conf.db(8)).
> +     */
> +    int (*qos_queue_get)(struct smap *details, uint32_t queue_id,
> +                         const struct qos_conf *conf);
>  };
> 
>  /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -
> 2986,14 +3011,94 @@ netdev_dpdk_set_qos(struct netdev *netdev, const char
> *type,
>      return error;
>  }
> 
> +static int
> +netdev_dpdk_get_queue(const struct netdev *netdev, uint32_t queue_id,
> +                      struct smap *details) {
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    struct qos_conf *qos_conf;
> +    int error = 0;
> +
> +    ovs_mutex_lock(&dev->mutex);
> +
> +    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
> +    if (!qos_conf || !qos_conf->ops || !qos_conf->ops->qos_queue_get) {
> +        error = EOPNOTSUPP;
> +    } else {
> +        error = qos_conf->ops->qos_queue_get(details, queue_id,
> qos_conf);
> +    }
> +
> +    ovs_mutex_unlock(&dev->mutex);
> +
> +    return error;
> +}
> +
> +static int
> +netdev_dpdk_set_queue(struct netdev *netdev, uint32_t queue_id,
> +                      const struct smap *details) {
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    struct qos_conf *qos_conf;
> +    int error = 0;
> +
> +    ovs_mutex_lock(&dev->mutex);
> +
> +    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
> +    if (!qos_conf || !qos_conf->ops || !qos_conf->ops-
> >qos_queue_construct) {
> +        error = EOPNOTSUPP;
> +    } else {
> +        error = qos_conf->ops->qos_queue_construct(details, queue_id,
> +                                                   qos_conf);
> +    }
> +
> +    if (error) {
> +        VLOG_ERR("Failed to set QoS queue %d on port %s: %s",
> +                 queue_id, netdev->name, rte_strerror(error));
> +    }
> +
> +    ovs_mutex_unlock(&dev->mutex);
> +
> +    return error;
> +}
> +
> +static int
> +netdev_dpdk_delete_queue(struct netdev *netdev, uint32_t queue_id) {
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    struct qos_conf *qos_conf;
> +
> +    ovs_mutex_lock(&dev->mutex);
> +
> +    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
> +    if (qos_conf && qos_conf->ops && qos_conf->ops->qos_queue_destruct) {
> +        qos_conf->ops->qos_queue_destruct(qos_conf, queue_id);
> +    }
> +
> +    ovs_mutex_unlock(&dev->mutex);
> +
> +    return 0;
> +}
> +
>  /* egress-policer details */
> 
>  struct egress_policer {
>      struct qos_conf qos_conf;
>      struct rte_meter_srtcm_params app_srtcm_params;
>      struct rte_meter_srtcm egress_meter;
> +    struct hmap queue;
>  };
> 
> +struct egress_queue_policer {

Minor nit but the struct name would sound/read better as 'egress_policer_queue', i.e. the QoS type followed by queue.

> +    struct hmap_node hmap_node;
> +    uint32_t queue_id;
> +    struct rte_meter_srtcm_params app_srtcm_params;
> +    struct rte_meter_srtcm egress_meter; };

Coding style infraction, closing brace and semi colon above should be moved to next line.
> +
> +static struct egress_queue_policer *
> +egress_policer_qos_find_queue(struct egress_policer *policer,
> +                              uint32_t queue_id);
> +
>  static void
>  egress_policer_details_to_param(const struct smap *details,
>                                  struct rte_meter_srtcm_params *params) @@
> -3018,6 +3123,7 @@ egress_policer_qos_construct(const struct smap
> *details,
>                                   &policer->app_srtcm_params);
>      if (!err) {
>          *conf = &policer->qos_conf;
> +        hmap_init(&policer->queue);
>      } else {
>          free(policer);
>          *conf = NULL;
> @@ -3032,6 +3138,13 @@ egress_policer_qos_destruct(struct qos_conf *conf)
> {
>      struct egress_policer *policer = CONTAINER_OF(conf, struct
> egress_policer,
>                                                    qos_conf);
> +    struct egress_queue_policer *queue_policer, *next_queue_policer;

Again, minor nit and same point as above, I'd prefer to see the variables 'queue_policer' renamed to 'policer_queueu', the same with 'next_queue_policer'. If anything it establishes that a queue is associated with the policer struct and reads better, can be applied to all other occurrences below.

> +    HMAP_FOR_EACH_SAFE (queue_policer, next_queue_policer, hmap_node,
> +                        &policer->queue) {
> +        hmap_remove(&policer->queue, &queue_policer->hmap_node);
> +        free(queue_policer);
> +    }
> +    hmap_destroy(&policer->queue);
>      free(policer);
>  }
> 
> @@ -3072,13 +3185,89 @@ egress_policer_run(struct qos_conf *conf, struct
> rte_mbuf **pkts, int pkt_cnt)
>      return cnt;
>  }
> 
> +static struct egress_queue_policer *
> +egress_policer_qos_find_queue(struct egress_policer *policer,
> +                              uint32_t queue_id) {
> +    struct egress_queue_policer *queue_policer;
> +    HMAP_FOR_EACH_WITH_HASH (queue_policer, hmap_node,
> +                             hash_2words(queue_id, 0), &policer->queue) {
> +        if (queue_policer->queue_id == queue_id) {
> +            return queue_policer;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int
> +egress_policer_qos_queue_construct(const struct smap *details,
> +                                   uint32_t queue_id, struct qos_conf
> +*conf) {
> +    int err = 0;
> +    struct egress_policer *policer = CONTAINER_OF(conf, struct
> egress_policer,
> +                                                  qos_conf);
> +    struct egress_queue_policer *queue_policer;
> +    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
> +    if (!queue_policer) {
> +        queue_policer = xmalloc(sizeof *queue_policer);
> +        hmap_insert(&policer->queue, &queue_policer->hmap_node,
> +                    hash_2words(queue_id, 0));
> +        queue_policer->queue_id = queue_id;
> +    }
> +    egress_policer_details_to_param(details, &queue_policer-
> >app_srtcm_params);
> +    err = rte_meter_srtcm_config(&queue_policer->egress_meter,
> +                                 &queue_policer->app_srtcm_params);
> +    if (err) {
> +        hmap_remove(&policer->queue, &queue_policer->hmap_node);
> +        free(queue_policer);
> +        err = -err;
> +    }
> +
> +    return err;
> +}
> +
> +static void
> +egress_policer_qos_queue_destruct(struct qos_conf *conf, uint32_t
> +queue_id) {
> +    struct egress_policer *policer = CONTAINER_OF(conf, struct
> egress_policer,
> +                                                  qos_conf);
> +    struct egress_queue_policer *queue_policer;
> +    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
> +    hmap_remove(&policer->queue, &queue_policer->hmap_node);
> +    free(queue_policer);
> +}
> +
> +static int
> +egress_policer_qos_queue_get(struct smap *details, uint32_t queue_id,
> +                             const struct qos_conf *conf) {
> +    struct egress_policer *policer =
> +        CONTAINER_OF(conf, struct egress_policer, qos_conf);
> +
> +    struct egress_queue_policer *queue_policer;
> +    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
> +    if (!queue_policer) {
> +      return -1;
> +    }
> +
> +    smap_add_format(details, "cir", "%"PRIu64,
> +                    queue_policer->app_srtcm_params.cir);
> +    smap_add_format(details, "cbs", "%"PRIu64,
> +                    queue_policer->app_srtcm_params.cbs);
> +
> +    return 0;
> +}
> +
>  static const struct dpdk_qos_ops egress_policer_ops = {
>      "egress-policer",    /* qos_name */
>      egress_policer_qos_construct,
>      egress_policer_qos_destruct,
>      egress_policer_qos_get,
>      egress_policer_qos_is_equal,
> -    egress_policer_run
> +    egress_policer_run,
> +    egress_policer_qos_queue_construct,
> +    egress_policer_qos_queue_destruct,
> +    egress_policer_qos_queue_get
>  };
> 
>  static int
> @@ -3260,9 +3449,9 @@ unlock:
>      NULL,                       /* get_qos_capabilities */    \
>      netdev_dpdk_get_qos,                                      \
>      netdev_dpdk_set_qos,                                      \
> -    NULL,                       /* get_queue */               \
> -    NULL,                       /* set_queue */               \
> -    NULL,                       /* delete_queue */            \
> +    netdev_dpdk_get_queue,                                    \
> +    netdev_dpdk_set_queue,                                    \
> +    netdev_dpdk_delete_queue,                                 \
>      NULL,                       /* get_queue_stats */         \
>      NULL,                       /* queue_dump_start */        \
>      NULL,                       /* queue_dump_next */         \
> --
> 2.7.4
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ea17b97..089ad64 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -259,6 +259,31 @@  struct dpdk_qos_ops {
      */
     int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
                    int pkt_cnt);
+
+    /* Called to construct a QoS Queue. The implementation should make
+     * the appropriate calls to configure QoS Queue according to 'details'.
+     *
+     * The contents of 'details' should be documented as valid for 'ovs_name'
+     * in the "other_config" column in the "QoS" table in vswitchd/vswitch.xml
+     * (which is built as ovs-vswitchd.conf.db(8)).
+     *
+     * This function must return 0 if and only if it constructs
+     * qos queue successfully.
+     */
+    int (*qos_queue_construct)(const struct smap *details,
+                               uint32_t queue_id, struct qos_conf *conf);
+
+    /* Destroys the Qos Queue */
+    void (*qos_queue_destruct)(struct qos_conf *conf, uint32_t queue_id);
+
+    /* Retrieves details of QoS Queue configuration into 'details'.
+     *
+     * The contents of 'details' should be documented as valid for 'ovs_name'
+     * in the "other_config" column in the "QoS" table in vswitchd/vswitch.xml
+     * (which is built as ovs-vswitchd.conf.db(8)).
+     */
+    int (*qos_queue_get)(struct smap *details, uint32_t queue_id,
+                         const struct qos_conf *conf);
 };
 
 /* dpdk_qos_ops for each type of user space QoS implementation */
@@ -2986,14 +3011,94 @@  netdev_dpdk_set_qos(struct netdev *netdev, const char *type,
     return error;
 }
 
+static int
+netdev_dpdk_get_queue(const struct netdev *netdev, uint32_t queue_id,
+                      struct smap *details)
+{
+    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+    struct qos_conf *qos_conf;
+    int error = 0;
+
+    ovs_mutex_lock(&dev->mutex);
+
+    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
+    if (!qos_conf || !qos_conf->ops || !qos_conf->ops->qos_queue_get) {
+        error = EOPNOTSUPP;
+    } else {
+        error = qos_conf->ops->qos_queue_get(details, queue_id, qos_conf);
+    }
+
+    ovs_mutex_unlock(&dev->mutex);
+
+    return error;
+}
+
+static int
+netdev_dpdk_set_queue(struct netdev *netdev, uint32_t queue_id,
+                      const struct smap *details)
+{
+    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+    struct qos_conf *qos_conf;
+    int error = 0;
+
+    ovs_mutex_lock(&dev->mutex);
+
+    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
+    if (!qos_conf || !qos_conf->ops || !qos_conf->ops->qos_queue_construct) {
+        error = EOPNOTSUPP;
+    } else {
+        error = qos_conf->ops->qos_queue_construct(details, queue_id,
+                                                   qos_conf);
+    }
+
+    if (error) {
+        VLOG_ERR("Failed to set QoS queue %d on port %s: %s",
+                 queue_id, netdev->name, rte_strerror(error));
+    }
+
+    ovs_mutex_unlock(&dev->mutex);
+
+    return error;
+}
+
+static int
+netdev_dpdk_delete_queue(struct netdev *netdev, uint32_t queue_id)
+{
+    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+    struct qos_conf *qos_conf;
+
+    ovs_mutex_lock(&dev->mutex);
+
+    qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
+    if (qos_conf && qos_conf->ops && qos_conf->ops->qos_queue_destruct) {
+        qos_conf->ops->qos_queue_destruct(qos_conf, queue_id);
+    }
+
+    ovs_mutex_unlock(&dev->mutex);
+
+    return 0;
+}
+
 /* egress-policer details */
 
 struct egress_policer {
     struct qos_conf qos_conf;
     struct rte_meter_srtcm_params app_srtcm_params;
     struct rte_meter_srtcm egress_meter;
+    struct hmap queue;
 };
 
+struct egress_queue_policer {
+    struct hmap_node hmap_node;
+    uint32_t queue_id;
+    struct rte_meter_srtcm_params app_srtcm_params;
+    struct rte_meter_srtcm egress_meter;
+};
+
+static struct egress_queue_policer *
+egress_policer_qos_find_queue(struct egress_policer *policer,
+                              uint32_t queue_id);
+
 static void
 egress_policer_details_to_param(const struct smap *details,
                                 struct rte_meter_srtcm_params *params)
@@ -3018,6 +3123,7 @@  egress_policer_qos_construct(const struct smap *details,
                                  &policer->app_srtcm_params);
     if (!err) {
         *conf = &policer->qos_conf;
+        hmap_init(&policer->queue);
     } else {
         free(policer);
         *conf = NULL;
@@ -3032,6 +3138,13 @@  egress_policer_qos_destruct(struct qos_conf *conf)
 {
     struct egress_policer *policer = CONTAINER_OF(conf, struct egress_policer,
                                                   qos_conf);
+    struct egress_queue_policer *queue_policer, *next_queue_policer;
+    HMAP_FOR_EACH_SAFE (queue_policer, next_queue_policer, hmap_node,
+                        &policer->queue) {
+        hmap_remove(&policer->queue, &queue_policer->hmap_node);
+        free(queue_policer);
+    }
+    hmap_destroy(&policer->queue);
     free(policer);
 }
 
@@ -3072,13 +3185,89 @@  egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt)
     return cnt;
 }
 
+static struct egress_queue_policer *
+egress_policer_qos_find_queue(struct egress_policer *policer,
+                              uint32_t queue_id)
+{
+    struct egress_queue_policer *queue_policer;
+    HMAP_FOR_EACH_WITH_HASH (queue_policer, hmap_node,
+                             hash_2words(queue_id, 0), &policer->queue) {
+        if (queue_policer->queue_id == queue_id) {
+            return queue_policer;
+        }
+    }
+    return NULL;
+}
+
+static int
+egress_policer_qos_queue_construct(const struct smap *details,
+                                   uint32_t queue_id, struct qos_conf *conf)
+{
+    int err = 0;
+    struct egress_policer *policer = CONTAINER_OF(conf, struct egress_policer,
+                                                  qos_conf);
+    struct egress_queue_policer *queue_policer;
+    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
+    if (!queue_policer) {
+        queue_policer = xmalloc(sizeof *queue_policer);
+        hmap_insert(&policer->queue, &queue_policer->hmap_node,
+                    hash_2words(queue_id, 0));
+        queue_policer->queue_id = queue_id;
+    }
+    egress_policer_details_to_param(details, &queue_policer->app_srtcm_params);
+    err = rte_meter_srtcm_config(&queue_policer->egress_meter,
+                                 &queue_policer->app_srtcm_params);
+    if (err) {
+        hmap_remove(&policer->queue, &queue_policer->hmap_node);
+        free(queue_policer);
+        err = -err;
+    }
+
+    return err;
+}
+
+static void
+egress_policer_qos_queue_destruct(struct qos_conf *conf, uint32_t queue_id)
+{
+    struct egress_policer *policer = CONTAINER_OF(conf, struct egress_policer,
+                                                  qos_conf);
+    struct egress_queue_policer *queue_policer;
+    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
+    hmap_remove(&policer->queue, &queue_policer->hmap_node);
+    free(queue_policer);
+}
+
+static int
+egress_policer_qos_queue_get(struct smap *details, uint32_t queue_id,
+                             const struct qos_conf *conf)
+{
+    struct egress_policer *policer =
+        CONTAINER_OF(conf, struct egress_policer, qos_conf);
+
+    struct egress_queue_policer *queue_policer;
+    queue_policer = egress_policer_qos_find_queue(policer, queue_id);
+    if (!queue_policer) {
+      return -1;
+    }
+
+    smap_add_format(details, "cir", "%"PRIu64,
+                    queue_policer->app_srtcm_params.cir);
+    smap_add_format(details, "cbs", "%"PRIu64,
+                    queue_policer->app_srtcm_params.cbs);
+
+    return 0;
+}
+
 static const struct dpdk_qos_ops egress_policer_ops = {
     "egress-policer",    /* qos_name */
     egress_policer_qos_construct,
     egress_policer_qos_destruct,
     egress_policer_qos_get,
     egress_policer_qos_is_equal,
-    egress_policer_run
+    egress_policer_run,
+    egress_policer_qos_queue_construct,
+    egress_policer_qos_queue_destruct,
+    egress_policer_qos_queue_get
 };
 
 static int
@@ -3260,9 +3449,9 @@  unlock:
     NULL,                       /* get_qos_capabilities */    \
     netdev_dpdk_get_qos,                                      \
     netdev_dpdk_set_qos,                                      \
-    NULL,                       /* get_queue */               \
-    NULL,                       /* set_queue */               \
-    NULL,                       /* delete_queue */            \
+    netdev_dpdk_get_queue,                                    \
+    netdev_dpdk_set_queue,                                    \
+    netdev_dpdk_delete_queue,                                 \
     NULL,                       /* get_queue_stats */         \
     NULL,                       /* queue_dump_start */        \
     NULL,                       /* queue_dump_next */         \