I’ve posted new version (v6) of patch set: https://patchwork.ozlabs.org/project/ovn/list/?series=410010
> On 6 Jun 2024, at 22:40, Ihar Hrachyshka <ihrac...@redhat.com> wrote: > > On Thu, Jun 6, 2024 at 1:27 AM Vladislav Odintsov <odiv...@gmail.com > <mailto:odiv...@gmail.com>> wrote: > >> Thanks Mark for such a detailed answer! >> >> I agree with your points, and also was thinking about them, but could not >> value their importance in terms of I-P logic. You helped with that. >> >> I’d prefer to apply my proposal to revert back to “bool is_vxlan_mode()” >> to make the “global” a true global. Will submit v6. >> >> > Happy we are doing it. Mark is more eloquent than me. :) > > >> regards, >> Vladislav Odintsov >> >>> On 5 Jun 2024, at 23:13, Mark Michelson <mmich...@redhat.com> wrote: >>> >>> On 6/5/24 08:51, Vladislav Odintsov wrote: >>>> Hi Mark, >>>> Thanks for the review! >>>> Please, see below. >>>> regards, >>>> Vladislav Odintsov >>>> -----Original Message----- >>>> From: Mark Michelson <mmich...@redhat.com> >>>> Date: Tuesday, 4 June 2024 at 03:45 >>>> To: Vladislav Odintsov <odiv...@gmail.com>, <d...@openvswitch.org> >>>> Subject: Re: [ovs-dev] [PATCH ovn v5 1/2] northd: Make `vxlan_mode` a >> global variable. >>>> Hi Vladislav, >>>> Generally speaking, I agree with this change. However, I think that >>>> setting a global variable from an incremental processing engine node >>>> runner feels wrong. >>>> The init_vxlan_mode() is called inside the en_global_config_run() only >> to >>>> initialize global value, which is then read by >> get_ovn_max_dp_key_local() to >>>> fill the "max_tunid" variable inside incremental processing engine node. >>>> Which drawbacks do you see of such variable initialization? >>> >>> The biggest drawbacks are: >>> * Reasoning about "ownership" of the vxlan_mode global variable >>> * Maintenance of the en_global_config I-P engine node. >>> >>> On the first point, since vxlan_mode is a global variable in northd.c, >> it's not obvious that the owner of this data is the en_global_config engine >> node. It's an easy mistake for someone to reference the variable before it >> has been initialized, for instance. However, if the boolean is on the >> ed_type_global_config struct, then it's clear to see that this data is >> scoped to the en_global_config engine node. >>> >>> On the second point, if someone were to overhaul the en_global_config >> engine node, it would be an easy mistake to make to not notice that >> vxlan_mode is being set by the engine node. I could see a developer >> splitting the node into separate nodes, for instance. In doing so, the >> developer could easily miss that the global vxlan_mode is being set by the >> engine node, since it's hidden behind an init_ function call. However, >> placing vxlan_mode on the ed_type_global_config makes it more clear that >> the en_global_config engine node is responsible for setting the value. >> >>> >>>> I think that instead, the "vxlan_mode" variable you have introduced >>>> should be a field on struct ed_type_global_config. This way, the >> engine >>>> node is only modifying data local to itself. >>>> I guess, that moving this to the struct ed_type_global_config will make >> the code >>>> a bit more complex: we have to pass this variable through all function >> calls to >>>> be able to read vxlan_mode value inside >> ovn_datapath_assign_requested_tnl_id(), >>>> ovn_port_assign_requested_tnl_id() and ovn_port_allocate_key(). >>> >>> I think dependency injection makes the code easier to read, understand, >> and maintain rather than making it more complex. It's clearer that the data >> from the en_global_config engine node is needed in all of the functions you >> listed if those functions require an ed_type_global_config argument. >>> >>>> Apart of this, the "vxlan_mode" variable has the same "global" meaning >> as >>>> "use_ct_inv_match", "check_lsp_is_up", "use_common_zone" and other >> global >>>> variables, which configure the global OVN behaviour. The difference is >> that it >>>> is required to read its value inside the en_global_config_run() to >> reflect the >>>> max_tunid back to NB_Global. >>> >>> Personally, I don't like those global variables either :) >>> >>> But those globals are also set within northd.c, and are initialized at >> the begining of a DB run. From the perspective of northd processing, they >> are truly "global" in their scope. The engine nodes form a dependency tree, >> and so it's important that data that is scoped to a particular node is >> housed in that engine node's data. This way, when nodes are being created, >> it's clear to know which other engine nodes they depend on. If engine nodes >> are setting global variables, then it becomes harder to understand the >> dependencies. >>>> If the global variable setting is totally not acceptable from engine >> node, I >>>> can propose another approach here. Revert init_vxlan_mode() back to >>>> `bool is_vxlan_mode()` and assign global variable outside of global >> engine node: >>>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >>>> index 873649a89..df0f8e58c 100644 >>>> --- a/northd/en-global-config.c >>>> +++ b/northd/en-global-config.c >>>> @@ -115,8 +115,9 @@ en_global_config_run(struct engine_node *node , >> void *data) >>>> config_data->svc_monitor_mac); >>>> } >>>> - init_vxlan_mode(sbrec_chassis_table); >>>> - char *max_tunid = xasprintf("%d", get_ovn_max_dp_key_local()); >>>> + char *max_tunid = xasprintf("%d", >>>> + get_ovn_max_dp_key_local( >>>> + >> is_vxlan_mode(sbrec_chassis_table))); >>>> smap_replace(options, "max_tunid", max_tunid); >>>> free(max_tunid); >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index 0e0ae24db..9ac608f03 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -885,25 +885,24 @@ join_datapaths(const struct >> nbrec_logical_switch_table *nbrec_ls_table, >>>> } >>>> } >>>> -void >>>> -init_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table) >>>> +bool >>>> +is_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table) >>>> { >>>> const struct sbrec_chassis *chassis; >>>> SBREC_CHASSIS_TABLE_FOR_EACH (chassis, sbrec_chassis_table) { >>>> for (int i = 0; i < chassis->n_encaps; i++) { >>>> if (!strcmp(chassis->encaps[i]->type, "vxlan")) { >>>> - vxlan_mode = true; >>>> - return; >>>> + return true; >>>> } >>>> } >>>> } >>>> - vxlan_mode = false; >>>> + return false; >>>> } >>>> uint32_t >>>> -get_ovn_max_dp_key_local(void) >>>> +get_ovn_max_dp_key_local(bool _vxlan_mode) >>>> { >>>> - if (vxlan_mode) { >>>> + if (_vxlan_mode) { >>>> /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for VXLAN mode. */ >>>> return OVN_MAX_DP_VXLAN_KEY; >>>> } >>>> @@ -916,9 +915,7 @@ ovn_datapath_allocate_key(struct hmap *datapaths, >> struct hmap *dp_tnlids, >>>> { >>>> if (!od->tunnel_key) { >>>> od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", >>>> - OVN_MIN_DP_KEY_LOCAL, >>>> - get_ovn_max_dp_key_local(), >>>> - hint); >>>> + OVN_MIN_DP_KEY_LOCAL, >> get_ovn_max_dp_key_local(vxlan_mode), hint); >>>> if (!od->tunnel_key) { >>>> if (od->sb) { >>>> sbrec_datapath_binding_delete(od->sb); >>>> @@ -17596,7 +17593,7 @@ ovnnb_db_run(struct northd_input *input_data, >>>> use_common_zone = smap_get_bool(input_data->nb_options, >> "use_common_zone", >>>> false); >>>> - init_vxlan_mode(input_data->sbrec_chassis_table); >>>> + vxlan_mode = is_vxlan_mode(input_data->sbrec_chassis_table); >>>> build_datapaths(ovnsb_txn, >>>> input_data->nbrec_logical_switch_table, >>>> diff --git a/northd/northd.h b/northd/northd.h >>>> index be480003e..c613652e9 100644 >>>> --- a/northd/northd.h >>>> +++ b/northd/northd.h >>>> @@ -791,9 +791,9 @@ lr_has_multiple_gw_ports(const struct ovn_datapath >> *od) >>>> return od->n_l3dgw_ports > 1 && !od->is_gw_router; >>>> } >>>> -void >>>> -init_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table); >>>> +bool >>>> +is_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table); >>>> -uint32_t get_ovn_max_dp_key_local(void); >>>> +uint32_t get_ovn_max_dp_key_local(bool); >>>> #endif /* NORTHD_H */ >>>> What do you think? >>>> Or, maybe I totally misunderstood your idea here, please correct me if >> yes. >>> >>> I think this idea is better than what is in the current patch. It also >> allows for you to add the NB global data as an argument to is_vxlan_mode() >> in patch 2 so that you can factor that into the decision about whether >> vxlan mode is enabled. >>> >>> The potential upside to caching a boolean is that you don't have to >> repeatedly walk the SB chassis table to determine if vxlan mode is enabled. >> >> This change was an intended potential performance enhancement by not only >> to avoid iterations over all chassises, but also to avoid doing this >> multiple times from mentioned functions. >> Though I didn’t do any performance testing, because it was not the concern. >> >>> I have no idea if this is actually a performance concern, though. Since >> northd performance tests have not yet shown this to be a bottleneck, I >> think your alternate proposal of keeping is_vxlan_mode() is the best >> approach. >>> >>>> On 5/3/24 04:13, Vladislav Odintsov wrote: >>>>> This simplifies code and subsequent commit to explicitely disable >> VXLAN >>>>> mode is based on these changes. >>>>> >>>>> Also "VXLAN mode" term is introduced in ovn-architecture man page. >>>>> >>>>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >>>>> --- >>>>> northd/en-global-config.c | 4 +- >>>>> northd/northd.c | 85 >> +++++++++++++++++---------------------- >>>>> northd/northd.h | 5 ++- >>>>> ovn-architecture.7.xml | 10 ++--- >>>>> 4 files changed, 47 insertions(+), 57 deletions(-) >>>>> >>>>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >>>>> index 28c78a12c..873649a89 100644 >>>>> --- a/northd/en-global-config.c >>>>> +++ b/northd/en-global-config.c >>>>> @@ -115,8 +115,8 @@ en_global_config_run(struct engine_node *node >> , void *data) >>>>> config_data->svc_monitor_mac); >>>>> } >>>>> >>>>> - char *max_tunid = xasprintf("%d", >>>>> - get_ovn_max_dp_key_local(sbrec_chassis_table)); >>>>> + init_vxlan_mode(sbrec_chassis_table); >>>>> + char *max_tunid = xasprintf("%d", >> get_ovn_max_dp_key_local()); >>>>> smap_replace(options, "max_tunid", max_tunid); >>>>> free(max_tunid); >>>>> >>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>> index 133cddb69..0e0ae24db 100644 >>>>> --- a/northd/northd.c >>>>> +++ b/northd/northd.c >>>>> @@ -90,6 +90,10 @@ static bool use_ct_inv_match = true; >>>>> */ >>>>> static bool default_acl_drop; >>>>> >>>>> +/* If this option is 'true' northd will use limited 24-bit space >> for datapath >>>>> + * and ports tunnel key allocation (12 bits for each instead of >> default 16). */ >>>>> +static bool vxlan_mode; >>>>> + >>>>> #define MAX_OVN_TAGS 4096 >>>>> >>>>> >>>>> @@ -881,40 +885,40 @@ join_datapaths(const struct >> nbrec_logical_switch_table *nbrec_ls_table, >>>>> } >>>>> } >>>>> >>>>> -static bool >>>>> -is_vxlan_mode(const struct sbrec_chassis_table >> *sbrec_chassis_table) >>>>> +void >>>>> +init_vxlan_mode(const struct sbrec_chassis_table >> *sbrec_chassis_table) >>>>> { >>>>> const struct sbrec_chassis *chassis; >>>>> SBREC_CHASSIS_TABLE_FOR_EACH (chassis, sbrec_chassis_table) >> { >>>>> for (int i = 0; i < chassis->n_encaps; i++) { >>>>> if (!strcmp(chassis->encaps[i]->type, "vxlan")) { >>>>> - return true; >>>>> + vxlan_mode = true; >>>>> + return; >>>>> } >>>>> } >>>>> } >>>>> - return false; >>>>> + vxlan_mode = false; >>>>> } >>>>> >>>>> uint32_t >>>>> -get_ovn_max_dp_key_local(const struct sbrec_chassis_table >> *sbrec_chassis_table) >>>>> +get_ovn_max_dp_key_local(void) >>>>> { >>>>> - if (is_vxlan_mode(sbrec_chassis_table)) { >>>>> - /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */ >>>>> + if (vxlan_mode) { >>>>> + /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for VXLAN mode. */ >>>>> return OVN_MAX_DP_VXLAN_KEY; >>>>> } >>>>> return OVN_MAX_DP_KEY - OVN_MAX_DP_GLOBAL_NUM; >>>>> } >>>>> >>>>> static void >>>>> -ovn_datapath_allocate_key(const struct sbrec_chassis_table >> *sbrec_ch_table, >>>>> - struct hmap *datapaths, struct hmap >> *dp_tnlids, >>>>> +ovn_datapath_allocate_key(struct hmap *datapaths, struct hmap >> *dp_tnlids, >>>>> struct ovn_datapath *od, uint32_t >> *hint) >>>>> { >>>>> if (!od->tunnel_key) { >>>>> od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, >> "datapath", >>>>> - OVN_MIN_DP_KEY_LOCAL, >>>>> - >> get_ovn_max_dp_key_local(sbrec_ch_table), >>>>> - hint); >>>>> + OVN_MIN_DP_KEY_LOCAL, >>>>> + >> get_ovn_max_dp_key_local(), >>>>> + hint); >>>>> if (!od->tunnel_key) { >>>>> if (od->sb) { >>>>> sbrec_datapath_binding_delete(od->sb); >>>>> @@ -927,7 +931,6 @@ ovn_datapath_allocate_key(const struct >> sbrec_chassis_table *sbrec_ch_table, >>>>> >>>>> static void >>>>> ovn_datapath_assign_requested_tnl_id( >>>>> - const struct sbrec_chassis_table *sbrec_chassis_table, >>>>> struct hmap *dp_tnlids, struct ovn_datapath *od) >>>>> { >>>>> const struct smap *other_config = (od->nbs >>>>> @@ -936,8 +939,7 @@ ovn_datapath_assign_requested_tnl_id( >>>>> uint32_t tunnel_key = smap_get_int(other_config, >> "requested-tnl-key", 0); >>>>> if (tunnel_key) { >>>>> const char *interconn_ts = smap_get(other_config, >> "interconn-ts"); >>>>> - if (!interconn_ts && is_vxlan_mode(sbrec_chassis_table) >> && >>>>> - tunnel_key >= 1 << 12) { >>>>> + if (!interconn_ts && vxlan_mode && tunnel_key >= 1 << >> 12) { >>>>> static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 1); >>>>> VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for >> datapath %s is " >>>>> "incompatible with VXLAN", tunnel_key, >>>>> @@ -985,7 +987,6 @@ build_datapaths(struct ovsdb_idl_txn >> *ovnsb_txn, >>>>> const struct nbrec_logical_switch_table >> *nbrec_ls_table, >>>>> const struct nbrec_logical_router_table >> *nbrec_lr_table, >>>>> const struct sbrec_datapath_binding_table >> *sbrec_dp_table, >>>>> - const struct sbrec_chassis_table >> *sbrec_chassis_table, >>>>> struct ovn_datapaths *ls_datapaths, >>>>> struct ovn_datapaths *lr_datapaths, >>>>> struct ovs_list *lr_list) >>>>> @@ -1000,12 +1001,11 @@ build_datapaths(struct ovsdb_idl_txn >> *ovnsb_txn, >>>>> struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); >>>>> struct ovn_datapath *od; >>>>> LIST_FOR_EACH (od, list, &both) { >>>>> - >> ovn_datapath_assign_requested_tnl_id(sbrec_chassis_table, &dp_tnlids, >>>>> - od); >>>>> + ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); >>>>> } >>>>> LIST_FOR_EACH (od, list, &nb_only) { >>>>> - >> ovn_datapath_assign_requested_tnl_id(sbrec_chassis_table, &dp_tnlids, >>>>> - od); } >>>>> + ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); >>>>> + } >>>>> >>>>> /* Keep nonconflicting tunnel IDs that are already >> assigned. */ >>>>> LIST_FOR_EACH (od, list, &both) { >>>>> @@ -1017,12 +1017,10 @@ build_datapaths(struct ovsdb_idl_txn >> *ovnsb_txn, >>>>> /* Assign new tunnel ids where needed. */ >>>>> uint32_t hint = 0; >>>>> LIST_FOR_EACH_SAFE (od, list, &both) { >>>>> - ovn_datapath_allocate_key(sbrec_chassis_table, >>>>> - datapaths, &dp_tnlids, od, >> &hint); >>>>> + ovn_datapath_allocate_key(datapaths, &dp_tnlids, od, >> &hint); >>>>> } >>>>> LIST_FOR_EACH_SAFE (od, list, &nb_only) { >>>>> - ovn_datapath_allocate_key(sbrec_chassis_table, >>>>> - datapaths, &dp_tnlids, od, >> &hint); >>>>> + ovn_datapath_allocate_key(datapaths, &dp_tnlids, od, >> &hint); >>>>> } >>>>> >>>>> /* Sync tunnel ids from nb to sb. */ >>>>> @@ -3982,16 +3980,14 @@ ovn_port_add_tnlid(struct ovn_port *op, >> uint32_t tunnel_key) >>>>> * that the I-P engine can fallback to recompute if needed; >> otherwise return >>>>> * true (even if the key is not allocated). */ >>>>> static bool >>>>> -ovn_port_assign_requested_tnl_id( >>>>> - const struct sbrec_chassis_table *sbrec_chassis_table, >> struct ovn_port *op) >>>>> +ovn_port_assign_requested_tnl_id(struct ovn_port *op) >>>>> { >>>>> const struct smap *options = (op->nbsp >>>>> ? &op->nbsp->options >>>>> : &op->nbrp->options); >>>>> uint32_t tunnel_key = smap_get_int(options, >> "requested-tnl-key", 0); >>>>> if (tunnel_key) { >>>>> - if (is_vxlan_mode(sbrec_chassis_table) && >>>>> - tunnel_key >= OVN_VXLAN_MIN_MULTICAST) { >>>>> + if (vxlan_mode && tunnel_key >= OVN_VXLAN_MIN_MULTICAST) >> { >>>>> static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 1); >>>>> VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s >> " >>>>> "is incompatible with VXLAN", >>>>> @@ -4011,11 +4007,10 @@ ovn_port_assign_requested_tnl_id( >>>>> } >>>>> >>>>> static bool >>>>> -ovn_port_allocate_key(const struct sbrec_chassis_table >> *sbrec_chassis_table, >>>>> - struct ovn_port *op) >>>>> +ovn_port_allocate_key(struct ovn_port *op) >>>>> { >>>>> if (!op->tunnel_key) { >>>>> - uint8_t key_bits = is_vxlan_mode(sbrec_chassis_table)? >> 12 : 16; >>>>> + uint8_t key_bits = vxlan_mode ? 12 : 16; >>>>> op->tunnel_key = >> ovn_allocate_tnlid(&op->od->port_tnlids, "port", >>>>> 1, (1u << (key_bits >> - 1)) - 1, >>>>> >> &op->od->port_key_hint); >>>>> @@ -4035,7 +4030,6 @@ ovn_port_allocate_key(const struct >> sbrec_chassis_table *sbrec_chassis_table, >>>>> static void >>>>> build_ports(struct ovsdb_idl_txn *ovnsb_txn, >>>>> const struct sbrec_port_binding_table >> *sbrec_port_binding_table, >>>>> - const struct sbrec_chassis_table *sbrec_chassis_table, >>>>> const struct sbrec_mirror_table *sbrec_mirror_table, >>>>> const struct sbrec_mac_binding_table >> *sbrec_mac_binding_table, >>>>> const struct sbrec_ha_chassis_group_table >> *sbrec_ha_chassis_group_table, >>>>> @@ -4069,10 +4063,10 @@ build_ports(struct ovsdb_idl_txn >> *ovnsb_txn, >>>>> /* Assign explicitly requested tunnel ids first. */ >>>>> struct ovn_port *op; >>>>> LIST_FOR_EACH (op, list, &both) { >>>>> - ovn_port_assign_requested_tnl_id(sbrec_chassis_table, >> op); >>>>> + ovn_port_assign_requested_tnl_id(op); >>>>> } >>>>> LIST_FOR_EACH (op, list, &nb_only) { >>>>> - ovn_port_assign_requested_tnl_id(sbrec_chassis_table, >> op); >>>>> + ovn_port_assign_requested_tnl_id(op); >>>>> } >>>>> >>>>> /* Keep nonconflicting tunnel IDs that are already >> assigned. */ >>>>> @@ -4084,14 +4078,14 @@ build_ports(struct ovsdb_idl_txn >> *ovnsb_txn, >>>>> >>>>> /* Assign new tunnel ids where needed. */ >>>>> LIST_FOR_EACH_SAFE (op, list, &both) { >>>>> - if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { >>>>> + if (!ovn_port_allocate_key(op)) { >>>>> sbrec_port_binding_delete(op->sb); >>>>> ovs_list_remove(&op->list); >>>>> ovn_port_destroy(ports, op); >>>>> } >>>>> } >>>>> LIST_FOR_EACH_SAFE (op, list, &nb_only) { >>>>> - if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { >>>>> + if (!ovn_port_allocate_key(op)) { >>>>> ovs_list_remove(&op->list); >>>>> ovn_port_destroy(ports, op); >>>>> } >>>>> @@ -4303,14 +4297,13 @@ ls_port_init(struct ovn_port *op, struct >> ovsdb_idl_txn *ovnsb_txn, >>>>> struct ovn_datapath *od, >>>>> const struct sbrec_port_binding *sb, >>>>> const struct sbrec_mirror_table >> *sbrec_mirror_table, >>>>> - const struct sbrec_chassis_table >> *sbrec_chassis_table, >>>>> struct ovsdb_idl_index *sbrec_chassis_by_name, >>>>> struct ovsdb_idl_index *sbrec_chassis_by_hostname) >>>>> { >>>>> op->od = od; >>>>> parse_lsp_addrs(op); >>>>> /* Assign explicitly requested tunnel ids first. */ >>>>> - if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, >> op)) { >>>>> + if (!ovn_port_assign_requested_tnl_id(op)) { >>>>> return false; >>>>> } >>>>> /* Keep nonconflicting tunnel IDs that are already >> assigned. */ >>>>> @@ -4320,7 +4313,7 @@ ls_port_init(struct ovn_port *op, struct >> ovsdb_idl_txn *ovnsb_txn, >>>>> } >>>>> } >>>>> /* Assign new tunnel ids where needed. */ >>>>> - if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { >>>>> + if (!ovn_port_allocate_key(op)) { >>>>> return false; >>>>> } >>>>> /* Create new binding, if needed. */ >>>>> @@ -4344,15 +4337,13 @@ ls_port_create(struct ovsdb_idl_txn >> *ovnsb_txn, struct hmap *ls_ports, >>>>> const char *key, const struct >> nbrec_logical_switch_port *nbsp, >>>>> struct ovn_datapath *od, >>>>> const struct sbrec_mirror_table >> *sbrec_mirror_table, >>>>> - const struct sbrec_chassis_table >> *sbrec_chassis_table, >>>>> struct ovsdb_idl_index *sbrec_chassis_by_name, >>>>> struct ovsdb_idl_index >> *sbrec_chassis_by_hostname) >>>>> { >>>>> struct ovn_port *op = ovn_port_create(ls_ports, key, nbsp, >> NULL, >>>>> NULL); >>>>> hmap_insert(&od->ports, &op->dp_node, >> hmap_node_hash(&op->key_node)); >>>>> - if (!ls_port_init(op, ovnsb_txn, od, NULL, >>>>> - sbrec_mirror_table, sbrec_chassis_table, >>>>> + if (!ls_port_init(op, ovnsb_txn, od, NULL, >> sbrec_mirror_table, >>>>> sbrec_chassis_by_name, >> sbrec_chassis_by_hostname)) { >>>>> ovn_port_destroy(ls_ports, op); >>>>> return NULL; >>>>> @@ -4367,7 +4358,6 @@ ls_port_reinit(struct ovn_port *op, struct >> ovsdb_idl_txn *ovnsb_txn, >>>>> struct ovn_datapath *od, >>>>> const struct sbrec_port_binding *sb, >>>>> const struct sbrec_mirror_table >> *sbrec_mirror_table, >>>>> - const struct sbrec_chassis_table >> *sbrec_chassis_table, >>>>> struct ovsdb_idl_index *sbrec_chassis_by_name, >>>>> struct ovsdb_idl_index >> *sbrec_chassis_by_hostname) >>>>> { >>>>> @@ -4375,8 +4365,7 @@ ls_port_reinit(struct ovn_port *op, struct >> ovsdb_idl_txn *ovnsb_txn, >>>>> op->sb = sb; >>>>> ovn_port_set_nb(op, nbsp, NULL); >>>>> op->l3dgw_port = op->cr_port = NULL; >>>>> - return ls_port_init(op, ovnsb_txn, od, sb, >>>>> - sbrec_mirror_table, sbrec_chassis_table, >>>>> + return ls_port_init(op, ovnsb_txn, od, sb, >> sbrec_mirror_table, >>>>> sbrec_chassis_by_name, >> sbrec_chassis_by_hostname); >>>>> } >>>>> >>>>> @@ -4521,7 +4510,6 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >>>>> op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, >>>>> new_nbsp->name, new_nbsp, od, >>>>> ni->sbrec_mirror_table, >>>>> - ni->sbrec_chassis_table, >>>>> ni->sbrec_chassis_by_name, >>>>> ni->sbrec_chassis_by_hostname); >>>>> if (!op) { >>>>> @@ -4553,7 +4541,6 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >>>>> if (!ls_port_reinit(op, ovnsb_idl_txn, >>>>> new_nbsp, >>>>> od, sb, ni->sbrec_mirror_table, >>>>> - ni->sbrec_chassis_table, >>>>> ni->sbrec_chassis_by_name, >>>>> ni->sbrec_chassis_by_hostname)) >> { >>>>> if (sb) { >>>>> @@ -17609,11 +17596,12 @@ ovnnb_db_run(struct northd_input >> *input_data, >>>>> use_common_zone = smap_get_bool(input_data->nb_options, >> "use_common_zone", >>>>> false); >>>>> >>>>> + init_vxlan_mode(input_data->sbrec_chassis_table); >>>>> + >>>>> build_datapaths(ovnsb_txn, >>>>> input_data->nbrec_logical_switch_table, >>>>> input_data->nbrec_logical_router_table, >>>>> input_data->sbrec_datapath_binding_table, >>>>> - input_data->sbrec_chassis_table, >>>>> &data->ls_datapaths, >>>>> &data->lr_datapaths, &data->lr_list); >>>>> build_lb_datapaths(input_data->lbs, input_data->lbgrps, >>>>> @@ -17621,7 +17609,6 @@ ovnnb_db_run(struct northd_input >> *input_data, >>>>> &data->lb_datapaths_map, >> &data->lb_group_datapaths_map); >>>>> build_ports(ovnsb_txn, >>>>> input_data->sbrec_port_binding_table, >>>>> - input_data->sbrec_chassis_table, >>>>> input_data->sbrec_mirror_table, >>>>> input_data->sbrec_mac_binding_table, >>>>> input_data->sbrec_ha_chassis_group_table, >>>>> diff --git a/northd/northd.h b/northd/northd.h >>>>> index 940926945..be480003e 100644 >>>>> --- a/northd/northd.h >>>>> +++ b/northd/northd.h >>>>> @@ -791,6 +791,9 @@ lr_has_multiple_gw_ports(const struct >> ovn_datapath *od) >>>>> return od->n_l3dgw_ports > 1 && !od->is_gw_router; >>>>> } >>>>> >>>>> -uint32_t get_ovn_max_dp_key_local(const struct >> sbrec_chassis_table *); >>>>> +void >>>>> +init_vxlan_mode(const struct sbrec_chassis_table >> *sbrec_chassis_table); >>>>> + >>>>> +uint32_t get_ovn_max_dp_key_local(void); >>>>> >>>>> #endif /* NORTHD_H */ >>>>> diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml >>>>> index bfd8680ce..3ecb58933 100644 >>>>> --- a/ovn-architecture.7.xml >>>>> +++ b/ovn-architecture.7.xml >>>>> @@ -2809,11 +2809,11 @@ >>>>> </ul> >>>>> >>>>> <p> >>>>> - When VXLAN is enabled on any hypervisor in a cluster, >> datapath and egress >>>>> - port identifier ranges are reduced to 12-bits. This is >> done because only >>>>> - STT and Geneve provide the large space for metadata (over >> 32 bits per >>>>> - packet). To accommodate for VXLAN, 24 bits available are >> split as >>>>> - follows: >>>>> + When VXLAN is enabled on any hypervisor in a cluster, >> datapath and egress >>>>> + port identifier ranges are reduced to 12-bits. This is done >> because only >>>>> + STT and Geneve provide the large space for metadata (over 32 >> bits per >>>>> + packet). The mode with reduced ranges is called <code>VXLAN >> mode</code>. >>>>> + To accommodate for VXLAN, 24 bits available are split as >> follows: >>>>> </p> >>>>> >>>>> <ul> >>> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > dev mailing list > d...@openvswitch.org <mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
