[ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

Darrell Ball dlu998 at gmail.com
Tue Aug 6 03:07:28 UTC 2019


Thanks for the patch

comments inline

On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
> the zone-based timeout policy in the vswitchd. Whenever there is a
> database change, vswitchd will read the datapath, CT_Zone, and
> CT_Timeout_Policy tables from ovsdb to detect if the is any timeout
> policy changes.
>
> If a new timeout policy is added, it stores the information in
> per datapath type internal datapath structure in struct dpif-backer,
> and pushes down the conntrack timeout policy into the datapath via dpif
> interface.
>
> If a timeout policy is no longer used, vswitchd may not be able to
> remove it from datapath immediately since the datapath flow may still
> reference that. Instead, we keep an timeout policy kill list, that
> vswitchd will goes back to the list regularly and try to kill the
> unused timeout policies.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>  ofproto/ofproto-dpif.c     | 266
> +++++++++++++++++++++++++++++++++++++++++++++
>  ofproto/ofproto-dpif.h     |  10 ++
>  ofproto/ofproto-provider.h |   8 ++
>  ofproto/ofproto.c          |  20 ++++
>  ofproto/ofproto.h          |   4 +
>  vswitchd/bridge.c          |  41 +++++++
>  6 files changed, 349 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..6336494e0bc8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -156,6 +156,25 @@ struct ofport_dpif {
>      size_t n_qdscp;
>  };
>
> +struct ct_timeout_policy {
> +    struct uuid uuid;
> +    unsigned int last_used_seqno;
> +    struct ct_dpif_timeout_policy cdtp;
> +    struct cmap_node node;          /* Element in struct dpif_backer's
> +                                     * "ct_tps" cmap. */
>


This looks like a layering violation
should be in dpif-netlink or netlink-conntrack for kernel side


> +    struct ovs_list list_node;      /* Element in struct dpif_backer's
> +                                     * "ct_tp_kill_list" list. */





> +};
> +
> +struct ct_zone {
> +    uint16_t id;
> +    unsigned int last_used_seqno;
> +    struct uuid tp_uuid;            /* uuid that identifies a timeout
> policy in
> +                                     * struct dpif_backer's "ct_tps"
> cmap. */
> +    struct cmap_node node;          /* Element in struct dpif_backer's
> +                                     * "ct_zones" cmap. */
> +};
> +
>  static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
>                                         ofp_port_t);
>
> @@ -196,6 +215,8 @@ static struct hmap all_ofproto_dpifs_by_uuid =
>
>  static bool ofproto_use_tnl_push_pop = true;
>  static void ofproto_unixctl_init(void);
> +static void destroy_ct_zone_timeout_policy(struct dpif_backer *backer);
> +static void init_ct_zone_timeout_policy(struct dpif_backer *backer);
>
>  static inline struct ofproto_dpif *
>  ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -683,6 +704,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
>      }
>      dpif_close(backer->dpif);
>      id_pool_destroy(backer->meter_ids);
> +    destroy_ct_zone_timeout_policy(backer);
>      free(backer);
>  }
>
> @@ -694,6 +716,8 @@ struct odp_garbage {
>
>  static void check_support(struct dpif_backer *backer);
>
> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
> +
>  static int
>  open_dpif_backer(const char *type, struct dpif_backer **backerp)
>  {
> @@ -811,6 +835,8 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>          backer->meter_ids = NULL;
>      }
>
> +    init_ct_zone_timeout_policy(backer);
> +
>      /* Make a pristine snapshot of 'support' into 'boottime_support'.
>       * 'boottime_support' can be checked to prevent 'support' to be
> changed
>       * beyond the datapath capabilities. In case 'support' is changed by
> @@ -5086,6 +5112,244 @@ ct_flush(const struct ofproto *ofproto_, const
> uint16_t *zone)
>      ct_dpif_flush(ofproto->backer->dpif, zone, NULL);
>  }
>
> +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_remove_and_destroy(struct dpif_backer *backer, struct ct_zone
> *zone)
> +{
> +    cmap_remove(&backer->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,
> +                        struct id_pool *tp_ids)
> +{
> +    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]);
> +    }
> +    if (!id_pool_alloc_id(tp_ids, &tp->cdtp.id)) {
> +        VLOG_WARN_RL(&rl, "failed to allocate timeout policy id.");
> +        free(tp);
> +        return NULL;
> +    }
> +
> +    return tp;
> +}
> +
> +static void
> +ct_timeout_policy_destroy(struct dpif_backer *backer,
> +                          struct ct_timeout_policy *tp)
> +{
> +    id_pool_free_id(backer->timeout_policy_ids, tp->cdtp.id);
> +    ovsrcu_postpone(free, tp);
> +}
> +
> +static bool
> +ct_timeout_policy_update(struct ovsrec_ct_timeout_policy *tp_cfg,
> +                         struct ct_timeout_policy *tp)
> +{
> +    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]);
> +    }
> +    return changed;
> +}
> +
> +static void
> +init_ct_zone_timeout_policy(struct dpif_backer *backer)
> +{
> +    cmap_init(&backer->ct_zones);
> +    cmap_init(&backer->ct_tps);
> +    backer->timeout_policy_ids = id_pool_create(0, MAX_TIMEOUT_POLICY_ID);
> +    ovs_list_init(&backer->ct_tp_kill_list);
> +
> +    /* Remove existing timeout policy from datapath. */
> +    struct ct_dpif_timeout_policy cdtp;
> +    struct ct_timeout_policy *tp;
> +    void *state;
> +    int err;
>

don't need 'err'



> +
> +    err = ct_dpif_timeout_policy_dump_start(backer->dpif, &state);
> +    if (err) {
> +        return;
> +    }
> +
> +    while (!(err = ct_dpif_timeout_policy_dump_next(backer->dpif, state,
> +                                                    &cdtp))) {
> +        tp = xzalloc(sizeof *tp);
> +        tp->cdtp = cdtp;
> +        id_pool_add(backer->timeout_policy_ids, cdtp.id);
> +        ovs_list_insert(&backer->ct_tp_kill_list, &tp->list_node);
> +    }
> +
> +    ct_dpif_timeout_policy_dump_done(backer->dpif, state);
> +}
> +
> +static void
> +destroy_ct_zone_timeout_policy(struct dpif_backer *backer)
> +{
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +
> +    CMAP_FOR_EACH (zone, node, &backer->ct_zones) {
> +        ct_zone_remove_and_destroy(backer, zone);
> +    }
> +
> +    CMAP_FOR_EACH (tp, node, &backer->ct_tps) {
> +        cmap_remove(&backer->ct_tps, &tp->node, uuid_hash(&tp->uuid));
> +        ct_timeout_policy_destroy(backer, tp);
> +    }
> +
> +    cmap_destroy(&backer->ct_zones);
> +    cmap_destroy(&backer->ct_tps);
> +    id_pool_destroy(backer->timeout_policy_ids);
> +}
> +
> +static void
> +ct_zone_timeout_policy_revalidate(struct dpif_backer *backer,
> +                                  unsigned int idl_seqno)
>

Looks like layering violation
'implementation' should be in dpif-netlink or netlink-conntrack for kernel
side



> +{
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +
> +    CMAP_FOR_EACH (zone, node, &backer->ct_zones) {
> +        if (zone->last_used_seqno != idl_seqno) {
> +            ct_zone_remove_and_destroy(backer, zone);
> +        }
> +    }
> +
> +    CMAP_FOR_EACH (tp, node, &backer->ct_tps) {
> +        if (tp->last_used_seqno != idl_seqno) {
> +            /* Even timeout policy is not referenced by any ct_zone in the
> +             * database, vswitchd may not be able to delete it right away
> +             * since the datapath flow may still reference to the timeout
> +             * policy. Move the timeout policy to ct_tp_kill_list and try
> to
> +             * delete it when possible. */
> +            cmap_remove(&backer->ct_tps, &tp->node, uuid_hash(&tp->uuid));
> +            ovs_list_push_back(&backer->ct_tp_kill_list, &tp->list_node);
> +        }
> +    }
> +}
> +
> +static void
> +ct_zone_timeout_policy_sweep(const struct ofproto *ofproto)
>

Looks like layering violation
implementation should be in dpif-netlink or netlink-conntrack for kernel
side



> +{
> +    struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
> +    struct ct_timeout_policy *tp;
> +    int err;
> +
> +    if (!ovs_list_is_empty(&backer->ct_tp_kill_list)) {
> +        LIST_FOR_EACH (tp, list_node, &backer->ct_tp_kill_list) {
> +            err = ct_dpif_del_timeout_policy(backer->dpif, tp->cdtp.id);
> +            if (!err) {
> +                ovs_list_remove(&tp->list_node);
> +                ct_timeout_policy_destroy(backer, tp);
> +            } else {
> +                VLOG_INFO_RL(&rl, "Failed to delete timeout policy id = "
> +                        "%"PRIu32" %s", tp->cdtp.id, ovs_strerror(err));
> +            }
> +        }
> +    }
> +}
> +
> +static void
> +ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto,
> +                                const struct ovsrec_datapath *dp_cfg,
> +                                unsigned int idl_seqno)
> +{
> +    struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
> +    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(&backer->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(&backer->ct_tps,
> &tp_cfg->header_.uuid);
> +        if (!tp) {
> +            tp = ct_timeout_policy_alloc(tp_cfg,
> backer->timeout_policy_ids);
> +            if (tp) {
> +                cmap_insert(&backer->ct_tps, &tp->node,
> uuid_hash(&tp->uuid));
> +                ct_dpif_set_timeout_policy(backer->dpif, &tp->cdtp);
> +            }
> +        } else {
> +            if (ct_timeout_policy_update(tp_cfg, tp)) {
> +                ct_dpif_set_timeout_policy(backer->dpif, &tp->cdtp);
> +            }
> +        }
> +        tp->last_used_seqno = idl_seqno;
> +
> +        /* Link zone with new timeout policy. */
> +        zone->tp_uuid = tp_cfg->header_.uuid;
> +        if (new_zone) {
> +            cmap_insert(&backer->ct_zones, &zone->node, hash_int(zone_id,
> 0));
> +        }
> +    }
> +    ct_zone_timeout_policy_revalidate(backer, idl_seqno);
> +    ct_zone_timeout_policy_sweep(ofproto);
> +}
> +
>  static bool
>  set_frag_handling(struct ofproto *ofproto_,
>                    enum ofputil_frag_handling frag_handling)
> @@ -6189,4 +6453,6 @@ const struct ofproto_class ofproto_dpif_class = {
>      get_datapath_version,       /* get_datapath_version */
>      type_set_config,
>      ct_flush,                   /* ct_flush */
> +    ct_zone_timeout_policy_reconfig,
> +    ct_zone_timeout_policy_sweep,
>  };
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index cd5321eb942c..170edf0e0e70 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -245,6 +245,16 @@ struct dpif_backer {
>      /* Meter. */
>      struct id_pool *meter_ids;     /* Datapath meter allocation. */
>
> +    /* Connection tracking */
>




Looks like layering violation
should be in dpif-netlink or netlink-conntrack for kernel side
userspace datapath could use uuid directly


> +    struct id_pool  *timeout_policy_ids;    /* Datapath timeout policy id
> +                                             * allocation. */
>





> +    struct cmap ct_zones;                   /* "struct ct_zone"s indexed
> by
> +                                             * zone id. */
> +    struct cmap ct_tps;                     /* "struct ct_timeout_policy"s
> +                                             * indexed by uuid. */
>



Looks like layering violation
should be in dpif-netlink or netlink-conntrack for kernel side

+    struct ovs_list ct_tp_kill_list;        /* a list of timeout policy to
> be
> +                                             * deleted. */
>





> +
>      /* Version string of the datapath stored in OVSDB. */
>      char *dp_version_string;
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7907d4bfb416..41e07f0ee23e 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -58,6 +58,7 @@
>  #include "tun-metadata.h"
>  #include "versions.h"
>  #include "vl-mff-map.h"
> +#include "vswitch-idl.h"
>
>  struct match;
>  struct ofputil_flow_mod;
> @@ -1872,6 +1873,13 @@ struct ofproto_class {
>      /* Flushes the connection tracking tables. If 'zone' is not NULL,
>       * only deletes connections in '*zone'. */
>      void (*ct_flush)(const struct ofproto *, const uint16_t *zone);
> +
> +    /* Reconfigures the zone-based conntrack tiemout policy based on the
> +     * ovsdb configuration. */
> +    void (*ct_zone_timeout_policy_reconfig)(const struct ofproto
> *ofproto_,
> +            const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
> +    /* Cleans up the to be deleted timeout policy in the pending kill
> list. */
> +    void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_);
>  };
>
>  extern const struct ofproto_class ofproto_dpif_class;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 1d6fc00696f8..373b8a4eba0c 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -935,6 +935,26 @@ ofproto_get_flow_restore_wait(void)
>      return flow_restore_wait;
>  }
>
> +/* Connection tracking configuration. */
> +void
> +ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto,
> +                                        const struct ovsrec_datapath
> *dp_cfg,
> +                                        unsigned int idl_seqno)
> +{
> +    if (ofproto->ofproto_class->ct_zone_timeout_policy_reconfig) {
> +        ofproto->ofproto_class->ct_zone_timeout_policy_reconfig(
> +            ofproto, dp_cfg, idl_seqno);
> +    }
> +}
> +
> +void
> +ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto)
> +{
> +    if (ofproto->ofproto_class->ct_zone_timeout_policy_sweep) {
> +        ofproto->ofproto_class->ct_zone_timeout_policy_sweep(ofproto);
> +    }
> +}
> +
>
>  /* Spanning Tree Protocol (STP) configuration. */
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 6e4afffa17e0..2ae42374be36 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -52,6 +52,7 @@ struct ovs_list;
>  struct lldp_status;
>  struct aa_settings;
>  struct aa_mapping_settings;
> +struct ovsrec_datapath;
>
>  /* Needed for the lock annotations. */
>  extern struct ovs_mutex ofproto_mutex;
> @@ -362,6 +363,9 @@ int ofproto_get_stp_status(struct ofproto *, struct
> ofproto_stp_status *);
>  int ofproto_set_rstp(struct ofproto *, const struct ofproto_rstp_settings
> *);
>  int ofproto_get_rstp_status(struct ofproto *, struct ofproto_rstp_status
> *);
>  void ofproto_set_vlan_limit(int vlan_limit);
> +void ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto
> *ofproto,
> +    const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
> +void ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto);
>
>  /* Configuration of ports. */
>  void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 2976771aeaba..39b7de7d8182 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -220,6 +220,11 @@ static long long int stats_timer = LLONG_MIN;
>  #define AA_REFRESH_INTERVAL (1000) /* In milliseconds. */
>  static long long int aa_refresh_timer = LLONG_MIN;
>
> +/* Each time this timer expires, the bridge tries to delete the timeout
> + * policies in the kill list. */
> +#define CT_ZONE_TIMEOUT_POLICY_TIMER_INTERVAL (5000) /* In milliseconds.
> */
> +static long long int ct_zone_timeout_policy_timer = LLONG_MIN;
> +
>  /* Whenever system interfaces are added, removed or change state, the
> bridge
>   * will be reconfigured.
>   */
> @@ -588,6 +593,40 @@ config_ofproto_types(const struct smap *other_config)
>  }
>
>  static void
> +reconfigure_conntrack_timeout_policy(const struct ovsrec_open_vswitch
> *cfg)
> +{
> +    struct bridge *br;
> +    int i;
> +
> +    for (i = 0; i < cfg->n_datapaths; i++) {
> +        const struct ovsrec_datapath *dp_cfg = cfg->value_datapaths[i];
> +        char *key = cfg->key_datapaths[i];
> +
> +        HMAP_FOR_EACH (br, node, &all_bridges) {
> +            if (!strcmp(br->type, key)) {
> +                ofproto_ct_zone_timeout_policy_reconfig(br->ofproto,
> dp_cfg,
> +                                                        idl_seqno);
> +            }
> +            break;
> +        }
> +    }
> +}
> +
> +static void
> +run_ct_zone_timeout_policy_sweep(void)
> +{
> +    struct bridge *br;
> +
> +    if (time_msec() >= ct_zone_timeout_policy_timer) {
> +        HMAP_FOR_EACH (br, node, &all_bridges) {
> +            ofproto_ct_zone_timeout_policy_sweep(br->ofproto);
> +        }
> +        ct_zone_timeout_policy_timer = time_msec() +
> +
>  CT_ZONE_TIMEOUT_POLICY_TIMER_INTERVAL;
> +    }
> +}
> +
> +static void
>  bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>  {
>      struct sockaddr_in *managers;
> @@ -669,6 +708,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
>      }
>
>      reconfigure_system_stats(ovs_cfg);
> +    reconfigure_conntrack_timeout_policy(ovs_cfg);
>
>      /* Complete the configuration. */
>      sflow_bridge_number = 0;
> @@ -3082,6 +3122,7 @@ bridge_run(void)
>      run_stats_update();
>      run_status_update();
>      run_system_stats();
> +    run_ct_zone_timeout_policy_sweep();
>  }
>
>  void
> --
> 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