Hi Ihar,

thanks for your review!

> On 2 May 2024, at 18:11, Ihar Hrachyshka <ihrac...@redhat.com> wrote:
> 
> On Thu, May 2, 2024 at 5:51 AM Vladislav Odintsov <odiv...@gmail.com 
> <mailto: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.
> 

Here I wanted to mention that in many pieces of code argument which
was passed just to find VXLAN encaps was removed and with less
code/arguments it looks more simple.

> 
>> mode is based on these changes.
>> 
>> Also `vxlan mode` term is introduced in ovn-architecture man page.
>> 
> 
> Should the mode name keep VXLAN capitalized?
> 

Dunno.
This was inspired by writing in initial commit [1].
I’m fine with both writings.


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

Oops. This is a result of a wrong rebase, sorry.
Will be fixed in v5.

> 
>>     }
>>     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.

Also, it’s caused by mistakes in rebase. Will be fixed.

> 
> 
>>     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
> ```
> 

Ok, this completely removes all possible wrong readings.

> 
>> +    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 <mailto:d...@openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> 
>> 
> _______________________________________________
> dev mailing list
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


1: 
https://github.com/ovn-org/ovn/commit/b07f1bc3d068#diff-935a3771045d93c92d2ffc5adb052cc0ed59bfd965aaf064c7c3c257f2ff332aR1211

Regards,
Vladislav Odintsov

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to