diff mbox series

[ovs-dev,v5,1/2] northd: Make `vxlan_mode` a global variable.

Message ID 20240503081348.189608-2-odivlad@gmail.com
State Changes Requested
Headers show
Series Add support to disable VXLAN mode. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Vladislav Odintsov May 3, 2024, 8:13 a.m. UTC 
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 <odivlad@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(-)

Comments

Mark Michelson June 3, 2024, 8:44 p.m. UTC | #1 
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.

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.

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 <odivlad@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>
Vladislav Odintsov June 5, 2024, 12:51 p.m. UTC | #2 
Hi Mark,

Thanks for the review!
Please, see below.

regards,
 
Vladislav Odintsov

-----Original Message-----
From: Mark Michelson <mmichels@redhat.com>
Date: Tuesday, 4 June 2024 at 03:45
To: Vladislav Odintsov <odivlad@gmail.com>, <dev@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?

    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().

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.

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.


    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 <odivlad@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>
Mark Michelson June 5, 2024, 8:13 p.m. UTC | #3 
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 <mmichels@redhat.com>
> Date: Tuesday, 4 June 2024 at 03:45
> To: Vladislav Odintsov <odivlad@gmail.com>, <dev@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. 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 <odivlad@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>
> 
> 
>
Vladislav Odintsov June 6, 2024, 5:26 a.m. UTC | #4 
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.

regards,
Vladislav Odintsov

> On 5 Jun 2024, at 23:13, Mark Michelson <mmichels@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 <mmichels@redhat.com>
>> Date: Tuesday, 4 June 2024 at 03:45
>> To: Vladislav Odintsov <odivlad@gmail.com>, <dev@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 <odivlad@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>
>
Ihar Hrachyshka June 6, 2024, 7:40 p.m. UTC | #5 
On Thu, Jun 6, 2024 at 1:27 AM Vladislav Odintsov <odivlad@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 <mmichels@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 <mmichels@redhat.com>
> >> Date: Tuesday, 4 June 2024 at 03:45
> >> To: Vladislav Odintsov <odivlad@gmail.com>, <dev@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 <odivlad@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Vladislav Odintsov June 7, 2024, 2:03 p.m. UTC | #6 
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 <ihrachys@redhat.com> wrote:
> 
> On Thu, Jun 6, 2024 at 1:27 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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 <mmichels@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 <mmichels@redhat.com>
>>>> Date: Tuesday, 4 June 2024 at 03:45
>>>> To: Vladislav Odintsov <odivlad@gmail.com>, <dev@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 <odivlad@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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org <mailto:dev@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Regards,
Vladislav Odintsov
diff mbox series

Patch

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>