On Thu, May 2, 2024 at 5:51 AM Vladislav Odintsov <odiv...@gmail.com> wrote:

> This simplifies code and subsequent commit to explicitely disable vxlan
>

I personally find it debatable that moving from explicit dependency through
a function argument to implicit dependency through a global variable is a
simplification. But I will leave others to chime in.


> mode is based on these changes.
>
> Also `vxlan mode` term is introduced in ovn-architecture man page.
>

Should the mode name keep VXLAN capitalized?


>
> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
> ---
>  northd/en-global-config.c |  4 +-
>  northd/northd.c           | 94 ++++++++++++++++-----------------------
>  northd/northd.h           |  5 ++-
>  ovn-architecture.7.xml    | 11 +++--
>  4 files changed, 50 insertions(+), 64 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 5e12fd1e8..b54219a85 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,24 +885,25 @@ 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)) {
> +    if (vxlan_mode) {
>          /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */
>          return OVN_MAX_DP_VXLAN_KEY;
>      }
> @@ -906,15 +911,14 @@ get_ovn_max_dp_key_local(const struct
> sbrec_chassis_table *sbrec_chassis_table)
>  }
>
>  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,17 +4078,10 @@ 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)) {
> -            sbrec_port_binding_delete(op->sb);
> -            ovs_list_remove(&op->list);
> -            ovn_port_destroy(ports, op);
> -        }
> +        ovn_port_allocate_key(op);
>

Shouldn't you still check the result of `ovn_port_allocate_key`? (Below
too.)


>      }
>      LIST_FOR_EACH_SAFE (op, list, &nb_only) {
> -        if (!ovn_port_allocate_key(sbrec_chassis_table, op)) {
> -            ovs_list_remove(&op->list);
> -            ovn_port_destroy(ports, op);
> -        }
> +        ovn_port_allocate_key(op);
>      }
>
>      /* For logical ports that are in both databases, update the southbound
> @@ -4303,14 +4290,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 +4306,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. */
> @@ -4333,6 +4319,10 @@ ls_port_init(struct ovn_port *op, struct
> ovsdb_idl_txn *ovnsb_txn,
>          op->sb = sbrec_port_binding_insert(ovnsb_txn);
>          sbrec_port_binding_set_logical_port(op->sb, op->key);
>      }
> +    /* Assign new tunnel ids where needed. */
> +    if (!ovn_port_allocate_key(op)) {
> +        return false;
> +    }
>

Was this ^ intended? I believe recent patches structured the code here so
that any potential failures happen before a new binding is created.


>      ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name,
>                            sbrec_chassis_by_hostname, NULL,
> sbrec_mirror_table,
>                            op, NULL, NULL);
> @@ -4344,15 +4334,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 +4355,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 +4362,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 +4507,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 +4538,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 +17593,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 +17606,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..7abb1fa83 100644
> --- a/ovn-architecture.7.xml
> +++ b/ovn-architecture.7.xml
> @@ -2809,13 +2809,12 @@
>    </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).  Such mode is a <code>vxlan mode</code>.  To accommodate for
>

The text is slightly ambiguous (one could interpret it as: vxlan mode is
the mode where the metadata space is large).

Consider:

```
+    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>
>      <li>
>        12-bit logical datapath identifier, derived from the
> --
> 2.44.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to