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 <dlu...@gmail.com> wrote: > Thanks for the patch; not a full review > > On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei <yihung....@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....@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 >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
