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

Darrell Ball dlu998 at gmail.com
Mon Jul 29 05:48:44 UTC 2019


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


More information about the dev mailing list