[ovs-dev] [dpdk-latest PATCH v3 1/2] netdev-dpdk: Add support for multi-queue QoS to the DPDK datapath
Stokes, Ian
ian.stokes at intel.com
Mon Nov 11 19:12:52 UTC 2019
On 10/1/2019 3:10 PM, Eelco Chaudron wrote:
> This patch adds support for multi-queue QoS to the DPDK datapath. Most of
> the code is based on an earlier patch from a patchset sent out by
> zhaozhanxu. The patch was titled "[ovs-dev, v2, 1/4] netdev-dpdk.c: Support
> the multi-queue QoS configuration for dpdk datapath"
Thanks for working on this Eelco, a few comments below.
>
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
Should there be a co-author for zhaozhanxu also on this patch?
> ---
> lib/netdev-dpdk.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 210 insertions(+), 6 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ba92e89..072ce96 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -197,6 +197,13 @@ struct qos_conf {
> rte_spinlock_t lock;
> };
>
> +/* QoS queue information used by the netdev queue dump functions*/
Minor, end comment above with period and space before comment termination.
> +struct netdev_dpdk_queue_state {
> + uint32_t *queues;
> + size_t cur_queue;
> + size_t n_queues;
> +};
> +
> /* A particular implementation of dpdk QoS operations.
> *
> * The functions below return 0 if successful or a positive errno value on
> @@ -263,6 +270,41 @@ struct dpdk_qos_ops {
> */
> int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
> int pkt_cnt, bool should_steal);
> +
> + /* 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.
Should qos above be QoS?
> + */
> + int (*qos_queue_construct)(const struct smap *details,
> + uint32_t queue_id, struct qos_conf *conf);
> +
> + /* Destroys the Qos Queue */
Minor, missing period.
> + 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);
> +
> + /* Retrieves statistics of QoS Queue configuration into 'stats'. */
> + int (*qos_queue_get_stats)(const struct qos_conf *conf, uint32_t queue_id,
> + struct netdev_queue_stats *stats);
> +
> + /* Setup the 'netdev_dpdk_queue_state' structure used by the dpdk queue
> + * dump functions.
> + */
> + int (*qos_queue_dump_state_init)(const struct qos_conf *conf,
> + struct netdev_dpdk_queue_state *state);
> };
>
> /* dpdk_qos_ops for each type of user space QoS implementation */
> @@ -4032,6 +4074,161 @@ 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 && error != EOPNOTSUPP) {
> + VLOG_ERR("Failed to set QoS queue %d on port %s: %s",
> + queue_id, netdev->name, rte_strerror(error));
Can we use 'netdev_get_name(&dev->up)' instead of 'netdev->name'. It's
just to keep it in the same style as other VLOG_ERR messages reference
device names.
> + }
> +
> + 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;
It seems we'll always return 0 here, should it not be the case we return
EOPNOTSUPP if queue delete is not supported?
> +}
> +
> +static int
> +netdev_dpdk_get_queue_stats(const struct netdev *netdev, uint32_t queue_id,
> + struct netdev_queue_stats *stats)
> +{
> + 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_stats) {
> + qos_conf->ops->qos_queue_get_stats(qos_conf, queue_id, stats);
> + } else {
> + error = EOPNOTSUPP;
> + }
> +
> + ovs_mutex_unlock(&dev->mutex);
> +
> + return error;
> +}
> +
> +static int
> +netdev_dpdk_queue_dump_start(const struct netdev *netdev, void **statep)
> +{
> + int error = 0;
> + struct qos_conf *qos_conf;
> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +
> + 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_dump_state_init) {
> + struct netdev_dpdk_queue_state *state;
> +
> + *statep = state = xmalloc(sizeof *state);
> + error = qos_conf->ops->qos_queue_dump_state_init(qos_conf, state);
> + } else {
> + error = EOPNOTSUPP;
> + }
> +
> + ovs_mutex_unlock(&dev->mutex);
> +
> + return error;
> +}
> +
> +static int
> +netdev_dpdk_queue_dump_next(const struct netdev *netdev, void *state_,
> + uint32_t *queue_idp, struct smap *details)
> +{
> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + struct netdev_dpdk_queue_state *state = state_;
> + struct qos_conf *qos_conf;
> + int error = EOF;
> +
> + ovs_mutex_lock(&dev->mutex);
> +
> + while (state->cur_queue < state->n_queues) {
> + uint32_t queue_id = state->queues[state->cur_queue++];
> +
> + qos_conf = ovsrcu_get_protected(struct qos_conf *, &dev->qos_conf);
> + if (qos_conf && qos_conf->ops && qos_conf->ops->qos_queue_get) {
> + *queue_idp = queue_id;
> + error = qos_conf->ops->qos_queue_get(details, queue_id, qos_conf);
> + break;
> + }
> + }
> +
> + ovs_mutex_unlock(&dev->mutex);
> +
> + return error;
> +}
> +
> +static int
> +netdev_dpdk_queue_dump_done(const struct netdev *netdev OVS_UNUSED,
> + void *state_)
> +{
> + struct netdev_dpdk_queue_state *state = state_;
> +
> + free(state->queues);
> + free(state);
> + return 0;
> +}
> +
> +
> +
> /* egress-policer details */
>
> struct egress_policer {
> @@ -4129,12 +4326,12 @@ egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt,
> }
>
> 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
> + .qos_name = "egress-policer", /* qos_name */
> + .qos_construct = egress_policer_qos_construct,
> + .qos_destruct = egress_policer_qos_destruct,
> + .qos_get = egress_policer_qos_get,
> + .qos_is_equal = egress_policer_qos_is_equal,
> + .qos_run = egress_policer_run
> };
>
> static int
> @@ -4392,6 +4589,13 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev,
> .get_qos_types = netdev_dpdk_get_qos_types, \
> .get_qos = netdev_dpdk_get_qos, \
> .set_qos = netdev_dpdk_set_qos, \
> + .get_queue = netdev_dpdk_get_queue, \
> + .set_queue = netdev_dpdk_set_queue, \
> + .delete_queue = netdev_dpdk_delete_queue, \
> + .get_queue_stats = netdev_dpdk_get_queue_stats, \
> + .queue_dump_start = netdev_dpdk_queue_dump_start, \
> + .queue_dump_next = netdev_dpdk_queue_dump_next, \
> + .queue_dump_done = netdev_dpdk_queue_dump_done, \
> .update_flags = netdev_dpdk_update_flags, \
> .rxq_alloc = netdev_dpdk_rxq_alloc, \
> .rxq_construct = netdev_dpdk_rxq_construct, \
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list