[ovs-dev] [PATCH 08/12] datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy tables

Darrell Ball dlu998 at gmail.com
Sat Jul 27 00:44:56 UTC 2019


Thanks for the patch; not a full review

On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> This patch reads the datapath, CT_Zone, and CT_Timeout_Policy tables
> from ovsdb, stores the information in a per datapath internal datapath
> structure, and pushes down the conntrack timeout policy into the
> datapath via dpif interface.
>
> The per datapath internal data structure will be used in
> ofproto-dpif-xlate to implement the zone-based timeout policy.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>  lib/automake.mk       |   2 +
>  lib/datapath-config.c | 379
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/datapath-config.h |  25 ++++
>  vswitchd/bridge.c     |   3 +
>  4 files changed, 409 insertions(+)
>  create mode 100644 lib/datapath-config.c
>  create mode 100644 lib/datapath-config.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 17b36b43d9d7..7532153f5d02 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -67,6 +67,8 @@ lib_libopenvswitch_la_SOURCES = \
>         lib/daemon.c \
>         lib/daemon.h \
>         lib/daemon-private.h \
> +       lib/datapath-config.c \
> +       lib/datapath-config.h \
>         lib/db-ctl-base.c \
>         lib/db-ctl-base.h \
>         lib/dhcp.h \
> diff --git a/lib/datapath-config.c b/lib/datapath-config.c
> new file mode 100644
> index 000000000000..cdd2128a60bc
> --- /dev/null
> +++ b/lib/datapath-config.c
> @@ -0,0 +1,379 @@
> +/* Copyright (c) 2019 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include "datapath-config.h"
> +
> +#include "cmap.h"
> +#include "ct-dpif.h"
> +#include "dpif.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(datapath_config);
> +
> +struct ct_timeout_policy {
> +    struct uuid uuid;
> +    unsigned int last_used_seqno;
> +    unsigned int last_updated_seqno;
>

Can you add a usage comment for 'last_updated_seqno' and explain why it is
needed
in addition to 'last_used_seqno'  ?



> +    struct ct_dpif_timeout_policy cdtp;
> +    struct cmap_node node;          /* Element in struct datapath's
> +                                     * "ct_timeout_policies" cmap. */
> +};
> +
> +struct ct_zone {
> +    uint16_t id;
> +    unsigned int last_used_seqno;
> +    struct uuid tp_uuid;            /* uuid that identifies a timeout
> policy in
> +                                     * struct datapaths's "ct_tps cmap. */
> +    struct cmap_node node;          /* Element in struct datapath's
> "ct_zones"
> +                                     * cmap. */
> +};
> +
> +struct datapath {
> +    char *type;                     /* Datapath type. */
> +    char *dpif_backer_name;
> +    const struct ovsrec_datapath *cfg;
> +
> +    struct hmap_node node;          /* In 'all_datapaths'. */
> +    struct cmap ct_zones;           /* "struct ct_zone"s indexed by zone
> id. */
> +    struct cmap ct_tps;             /* "struct ct_timeout_policy"s
> indexed by
> +                                     * uuid. */
> +};
> +
> +/* All datapaths, indexed by type. */
> +static struct hmap all_datapaths = HMAP_INITIALIZER(&all_datapaths);
> +
> +static void ct_zone_destroy(struct datapath *, struct ct_zone *);
> +static void ct_timeout_policy_destroy(struct datapath *,
> +                                      struct ct_timeout_policy *,
> +                                      struct dpif *);
> +
> +static struct datapath *
> +datapath_lookup(const char *type)
> +{
> +    struct datapath *dp;
> +
> +    HMAP_FOR_EACH_WITH_HASH (dp, node, hash_string(type, 0),
> &all_datapaths) {
> +        if (!strcmp(dp->type, type)) {
> +            return dp;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void
> +datapath_clear_timeout_policy(struct datapath *dp)
> +{
> +    struct ct_dpif_timeout_policy *tp;
> +    struct dpif *dpif;
> +    void *state;
> +    int err;
> +
> +    dpif_open(dp->dpif_backer_name, dp->type, &dpif);
> +    if (!dpif) {
> +        return;
> +    }
> +
> +    err = ct_dpif_timeout_policy_dump_start(dpif, &state);
> +    if (err) {
> +        return ;
> +    }
> +
> +    while (!(err = ct_dpif_timeout_policy_dump_next(dpif, state, &tp))) {
> +        ct_dpif_del_timeout_policy(dpif, tp->id);
> +        free(tp);
> +    }
> +
> +    ct_dpif_timeout_policy_dump_done(dpif, state);
> +    dpif_close(dpif);
> +}
> +
> +static struct datapath *
> +datapath_create(const struct ovsrec_datapath *dp_cfg, const char *type)
> +{
> +    struct datapath *dp;
> +
> +    ovs_assert(!datapath_lookup(type));
> +    dp = xzalloc(sizeof *dp);
> +
> +    dp->type = xstrdup(type);
> +    dp->dpif_backer_name = xasprintf("ovs-%s", type);
> +    dp->cfg = dp_cfg;
> +
> +    cmap_init(&dp->ct_zones);
> +    cmap_init(&dp->ct_tps);
> +
> +    datapath_clear_timeout_policy(dp);
> +    hmap_insert(&all_datapaths, &dp->node, hash_string(dp->type, 0));
> +    return dp;
> +}
> +
> +static void
> +datapath_destroy(struct datapath *dp)
> +{
> +    struct ct_zone *zone;
> +    struct ct_timeout_policy *tp;
> +    struct dpif *dpif;
> +
> +    if (dp) {
> +        CMAP_FOR_EACH (zone, node, &dp->ct_zones) {
> +            ct_zone_destroy(dp, zone);
> +        }
> +
> +        dpif_open(dp->dpif_backer_name, dp->type, &dpif);
> +
> +        CMAP_FOR_EACH (tp, node, &dp->ct_tps) {
> +            ct_timeout_policy_destroy(dp, tp, dpif);
> +        }
> +
> +        dpif_close(dpif);
> +        hmap_remove(&all_datapaths, &dp->node);
> +        cmap_destroy(&dp->ct_zones);
> +        cmap_destroy(&dp->ct_tps);
> +        free(dp->type);
> +        free(dp->dpif_backer_name);
> +        free(dp);
> +    }
> +}
> +
> +static void
> +add_del_datapaths(const struct ovsrec_open_vswitch *cfg)
> +{
> +    struct datapath *dp, *next;
> +    struct shash_node *node;
> +    struct shash new_dp;
> +    size_t i;
> +
> +    /* Collect new datapaths' type. */
> +    shash_init(&new_dp);
> +    for (i = 0; i < cfg->n_datapaths; i++) {
> +        const struct ovsrec_datapath *dp_cfg = cfg->value_datapaths[i];
> +        char *key = cfg->key_datapaths[i];
> +
> +        if (!strcmp(key, "system") || !strcmp(key, "netdev")) {
> +            shash_add(&new_dp, key, dp_cfg);
> +        } else {
> +            VLOG_WARN("Unsupported dpatapath type %s\n", key);
> +        }
> +    }
> +
> +    /* Get rid of deleted datapath. */
> +    HMAP_FOR_EACH_SAFE (dp, next, node, &all_datapaths) {
> +        dp->cfg = shash_find_data(&new_dp, dp->type);
> +        if (!dp->cfg) {
> +            datapath_destroy(dp);
> +        }
> +    }
> +
> +    /* Add new datapaths */
> +    SHASH_FOR_EACH (node, &new_dp) {
> +        const struct ovsrec_datapath *dp_cfg = node->data;
> +        if (!datapath_lookup(node->name)) {
> +            datapath_create(dp_cfg, node->name);
> +        }
> +    }
> +
> +    shash_destroy(&new_dp);
> +}
> +
> +static struct ct_zone *
> +ct_zone_lookup(struct cmap *ct_zones, uint16_t zone_id)
> +{
> +    struct ct_zone *zone;
> +
> +    CMAP_FOR_EACH_WITH_HASH (zone, node, hash_int(zone_id, 0), ct_zones) {
> +        if (zone->id == zone_id) {
> +            return zone;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct ct_zone *
> +ct_zone_alloc(uint16_t zone_id)
> +{
> +    struct ct_zone *zone;
> +
> +    zone = xzalloc(sizeof *zone);
> +    zone->id = zone_id;
> +
> +    return zone;
> +}
> +
> +static void
> +ct_zone_destroy(struct datapath *dp, struct ct_zone *zone)
> +{
> +    cmap_remove(&dp->ct_zones, &zone->node, hash_int(zone->id, 0));
> +    ovsrcu_postpone(free, zone);
> +}
> +
> +static struct ct_timeout_policy *
> +ct_timeout_policy_lookup(struct cmap *ct_tps, struct uuid *uuid)
> +{
> +    struct ct_timeout_policy *tp;
> +
> +    CMAP_FOR_EACH_WITH_HASH (tp, node, uuid_hash(uuid), ct_tps) {
> +        if (uuid_equals(&tp->uuid, uuid)) {
> +            return tp;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct ct_timeout_policy *
> +ct_timeout_policy_alloc(struct ovsrec_ct_timeout_policy *tp_cfg,
> +                        unsigned int idl_seqno)
> +{
> +    struct ct_timeout_policy *tp;
> +    size_t i;
> +
> +    tp = xzalloc(sizeof *tp);
> +    tp->uuid = tp_cfg->header_.uuid;
> +    for (i = 0; i < tp_cfg->n_timeouts; i++) {
> +        ct_dpif_set_timeout_policy_attr_by_name(&tp->cdtp,
> +            tp_cfg->key_timeouts[i], tp_cfg->value_timeouts[i]);
> +    }
> +    tp->cdtp.id = idl_seqno;
> +    tp->last_updated_seqno = idl_seqno;
> +
> +    return tp;
> +}
> +
> +static bool
> +ct_timeout_policy_update(struct ovsrec_ct_timeout_policy *tp_cfg,
> +                         struct ct_timeout_policy *tp,
> +                         unsigned int idl_seqno)
> +{
> +    size_t i;
> +    bool changed = false;
> +
> +    for (i = 0; i < tp_cfg->n_timeouts; i++) {
> +        changed |= ct_dpif_set_timeout_policy_attr_by_name(&tp->cdtp,
> +                        tp_cfg->key_timeouts[i],
> tp_cfg->value_timeouts[i]);
> +    }
> +    if (changed) {
> +        tp->last_updated_seqno = idl_seqno;
> +    }
> +    return changed;
> +}
> +
> +static void
> +ct_timeout_policy_destroy(struct datapath *dp, struct ct_timeout_policy
> *tp,
> +                          struct dpif *dpif)
> +{
> +    cmap_remove(&dp->ct_tps, &tp->node, uuid_hash(&tp->uuid));
> +    if (dpif) {
> +        ct_dpif_del_timeout_policy(dpif, tp->cdtp.id);
> +    }
> +    ovsrcu_postpone(free, tp);
> +}
> +
> +static void
> +datapath_update_ct_zone_config(struct datapath *dp, struct dpif *dpif,
> +                               unsigned int idl_seqno)
> +{
> +    const struct ovsrec_datapath *dp_cfg = dp->cfg;
> +    struct ovsrec_ct_timeout_policy *tp_cfg;
> +    struct ovsrec_ct_zone *zone_cfg;
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +    uint16_t zone_id;
> +    bool new_zone;
> +    size_t i;
> +
> +    for (i = 0; i < dp_cfg->n_ct_zones; i++) {
> +        /* Update ct_zone config */
> +        zone_cfg = dp_cfg->value_ct_zones[i];
> +        zone_id = dp_cfg->key_ct_zones[i];
> +        zone = ct_zone_lookup(&dp->ct_zones, zone_id);
> +        if (!zone) {
> +            new_zone = true;
> +            zone = ct_zone_alloc(zone_id);
> +        } else {
> +            new_zone = false;
> +        }
> +        zone->last_used_seqno = idl_seqno;
> +
> +        /* Update timeout policy */
> +        tp_cfg = zone_cfg->timeout_policy;
> +        tp = ct_timeout_policy_lookup(&dp->ct_tps, &tp_cfg->header_.uuid);
> +        if (!tp) {
> +            tp = ct_timeout_policy_alloc(tp_cfg, idl_seqno);
> +            cmap_insert(&dp->ct_tps, &tp->node, uuid_hash(&tp->uuid));
> +            if (dpif) {
> +                ct_dpif_add_timeout_policy(dpif, false, &tp->cdtp);
> +            }
> +        } else {
> +            if (ct_timeout_policy_update(tp_cfg, tp, idl_seqno)) {
> +                if (dpif) {
> +                    ct_dpif_add_timeout_policy(dpif, false, &tp->cdtp);
> +                }
> +            }
> +        }
> +        tp->last_used_seqno = idl_seqno;
> +
> +        /* Update default timeout policy */
> +        if (!zone_id && tp->last_updated_seqno == idl_seqno) {
>


If zone==0, we will configure a default timeout policy.
In Netfilter this will be nw_proto scope and shared across V4/V6
i.e. the global Netfilter values

I eluded to this in a previous patch.
We need to document it
Maybe we need to present it better.


+            ct_dpif_add_timeout_policy(dpif, true, &tp->cdtp);
> +        }
> +
> +        /* Link zone with new timeout policy */
> +        zone->tp_uuid = tp_cfg->header_.uuid;
> +        if (new_zone) {
> +            cmap_insert(&dp->ct_zones, &zone->node, hash_int(zone_id, 0));
> +        }
> +    }
> +}
> +
> +void
> +reconfigure_datapath(const struct ovsrec_open_vswitch *cfg,
> +                     unsigned int idl_seqno)
> +{
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +    struct datapath *dp;
> +    struct dpif *dpif;
> +
> +    add_del_datapaths(cfg);
> +    HMAP_FOR_EACH (dp, node, &all_datapaths) {
> +        dpif_open(dp->dpif_backer_name, dp->type, &dpif);
> +
> +        datapath_update_ct_zone_config(dp, dpif, idl_seqno);
> +
> +        /* Garbage colleciton */
> +        CMAP_FOR_EACH (zone, node, &dp->ct_zones) {
> +            if (zone->last_used_seqno != idl_seqno) {
> +                ct_zone_destroy(dp, zone);
> +            }
> +        }
> +        CMAP_FOR_EACH (tp, node, &dp->ct_tps) {
> +            if (tp->last_used_seqno != idl_seqno) {
> +                ct_timeout_policy_destroy(dp, tp, dpif);
> +            }
> +        }
> +
> +        dpif_close(dpif);
> +    }
> +}
> +
> +void
> +destroy_all_datapaths(void)
> +{
> +    struct datapath *dp, *next_dp;
> +
> +    HMAP_FOR_EACH_SAFE (dp, next_dp, node, &all_datapaths) {
> +        datapath_destroy(dp);
> +    }
> +}
> diff --git a/lib/datapath-config.h b/lib/datapath-config.h
> new file mode 100644
> index 000000000000..d9a90e4f8312
> --- /dev/null
> +++ b/lib/datapath-config.h
> @@ -0,0 +1,25 @@
> +/* Copyright (c) 2019 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef DATAPATH_CONFIG_H
> +#define DATAPATH_CONFIG_H 1
> +
> +#include "vswitch-idl.h"
> +
> +void reconfigure_datapath(const struct ovsrec_open_vswitch *,
> +                          unsigned int idl_seqno);
> +void destroy_all_datapaths(void);
> +
> +#endif /* datapath-config.h */
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 2976771aeaba..e8ac24a50ff2 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -26,6 +26,7 @@
>  #include "connectivity.h"
>  #include "coverage.h"
>  #include "daemon.h"
> +#include "datapath-config.h"
>  #include "dirs.h"
>  #include "dpif.h"
>  #include "dpdk.h"
> @@ -508,6 +509,7 @@ bridge_exit(bool delete_datapath)
>      HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) {
>          bridge_destroy(br, delete_datapath);
>      }
> +    destroy_all_datapaths();
>      ovsdb_idl_destroy(idl);
>  }
>
> @@ -669,6 +671,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
>      }
>
>      reconfigure_system_stats(ovs_cfg);
> +    reconfigure_datapath(ovs_cfg, idl_seqno);
>
>      /* Complete the configuration. */
>      sflow_bridge_number = 0;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list