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

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@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

I only have some minor comments inline below.

On Thu, Jul 25, 2019 at 04:24:10PM -0700, Yi-Hung Wei 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@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;
> +    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 */
comment should end wit dot

> +    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 */
missing .
> +        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 */

missing . and some places below

> +        if (!zone_id && tp->last_updated_seqno == idl_seqno) {
> +            ct_dpif_add_timeout_policy(dpif, true, &tp->cdtp);

Do we need to check 'if (dpif)' here?
You have the checking above, but not here. Or should we just return
error when dpif is NULL.

> +        }
> +
> +        /* 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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks for the patch; not a full review

On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei <yihung.wei@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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

One more comment

Not a full review; just focusing on the more important parts for now.


On Fri, Jul 26, 2019 at 5:44 PM Darrell Ball <dlu998@gmail.com> wrote:

> Thanks for the patch; not a full review
>
> On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei <yihung.wei@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@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;
>>
>
I am not sure this datapath timeout profile ID allocation scheme is going
to work.

1/ I think one change in the idL_seqno can be associated with multiple
changes in the DB;
for example we can add multiple timeout profiles with one seqno change, in
which
case all these profiles would get the same datapath timeout profile ID.

2/ What happens when vswitchd restarts.
If you want to delete a profile from the kernel, how do you know the
datapath profile ID ?
When you want to add a profile, I think you could end with trying to use
the same ID as an
existing different profile in the kernel.

I think you want to use an ID pool (see id-pool.c/.h) to allocate datapath
profile IDs
and then store the ID in the external ID column of the timeout profile in
the DB
When vswitchd restarts, you need to re-populate your datastructures,
including the
datapath profile IDs.

+
>> +    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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>

On Fri, Jul 26, 2019 at 1:22 PM William Tu <u9012063@gmail.com> wrote:
> > +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 */
> missing .
> > +        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 */
>
> missing . and some places below
>
> > +        if (!zone_id && tp->last_updated_seqno == idl_seqno) {
> > +            ct_dpif_add_timeout_policy(dpif, true, &tp->cdtp);
>
> Do we need to check 'if (dpif)' here?
> You have the checking above, but not here. Or should we just return
> error when dpif is NULL.
Thanks for review. I will check if dpif is NULL and address the
comment issue in v2.

Thanks,

-Yi-Hung