[ovs-dev] [PATCH ovn v3 1/2] controller-vtep: Fix MMR and physical locators create/update

Vladislav Odintsov odivlad at gmail.com
Tue Jun 15 16:30:41 UTC 2021


Hi Dumitru,

Thanks for the review, new patch is submitted to the list.

Regards,
Vladislav Odintsov

> On 15 Jun 2021, at 18:50, Dumitru Ceara <dceara at redhat.com> wrote:
> 
> On 6/11/21 1:44 PM, Vladislav Odintsov wrote:
>> Before this patch ovn-controller-vtep created Mcast_Macs_Remote
>> record for each Port Binding of the OVN Logical Switch, to which
>> VTEP Logical Switch was attached.
>> With this patch there is only one Mcast_Macs_Remote record per datapath.
>> Physical Locator Set is created every time when physical locators for
>> associated datapath are changed. Next, this newly-created physical
>> locator set is updated in the MMR record.
>> 
>> Signed-off-by: Vladislav Odintsov <odivlad at gmail.com <mailto:odivlad at gmail.com>>
>> ---
> 
> Thanks for this new revision!  I think the changes are OK, I just have a
> few minor style comments.  If you address them, feel free to add my ack
> in v4.
> 
> Acked-by: Dumitru Ceara <dceara at redhat.com <mailto:dceara at redhat.com>>
> 
>> controller-vtep/vtep.c       | 60 ++++++++++++++++++-------------
>> tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 105 insertions(+), 25 deletions(-)
>> 
>> diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
>> index f3b02f63f..49723b39d 100644
>> --- a/controller-vtep/vtep.c
>> +++ b/controller-vtep/vtep.c
>> @@ -48,7 +48,7 @@ struct mmr_hash_node_data {
>>  * database.
>>  *
>>  */
>> -
>> +
> 
> These are actually part of the coding guidelines [0], we should probably
> leave them as they are:
> 
> "Use form feeds (control+L) to divide long source files into logical
> pieces. A form feed should appear as the only character on a line."
> 
> [0]
> https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#basics <https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#basics>
> 
>> /* Searches the 'chassis_rec->encaps' for the first vtep tunnel
>>  * configuration, returns the 'ip'.  Unless duplicated, the returned
>>  * pointer cannot live past current vtep_run() execution. */
>> @@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
>> 
>>     return new_pl;
>> }
>> -
>> +
> 
> Same here.
> 
>> /* Creates a new 'Mcast_Macs_Remote'. */
>> static void
>> vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>> @@ -104,6 +104,7 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>>     struct vteprec_mcast_macs_remote *new_mmr =
>>        vteprec_mcast_macs_remote_insert(vtep_idl_txn);
>> 
>> +    VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name);
>>     vteprec_mcast_macs_remote_set_MAC(new_mmr, mac);
>>     vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls);
>>     vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
>> @@ -140,7 +141,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
>>                            ploc_entry->vteprec_ploc;
>>             if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
>>                                             locators[i]->dst_ip)) {
>> -                    locator_lists_differ = true;
>> +                locator_lists_differ = true;
>>             }
>>             i++;
>>         }
>> @@ -149,8 +150,8 @@ vtep_process_pls(const struct ovs_list *locators_list,
>>     return locator_lists_differ;
>> }
>> 
>> -/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
>> - * out-dated remote mcast mac entries as needed. */
>> +/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed
>> + * and also cleans up out-dated remote mcast mac entries as needed. */
>> static void
>> vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>>                 struct ovs_list *locators_list,
>> @@ -162,22 +163,29 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>>     bool mmr_changed;
>> 
>>     locators = xmalloc(n_locators_new * sizeof *locators);
>> -
>>     mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
>> 
>> -    if (mmr_ext && !n_locators_new) {
>> -        vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
>> -    } else if ((mmr_ext && mmr_changed) ||
>> -               (!mmr_ext && n_locators_new)) {
>> +    if (mmr_changed) {
>> +        if (n_locators_new) {
>> +            const struct vteprec_physical_locator_set *ploc_set =
>> +                vteprec_physical_locator_set_insert(vtep_idl_txn);
>> 
>> -        const struct vteprec_physical_locator_set *ploc_set =
>> -            vteprec_physical_locator_set_insert(vtep_idl_txn);
>> +            vteprec_physical_locator_set_set_locators(ploc_set, locators,
>> +                                                      n_locators_new);
>> 
>> -        vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
>> +            if (!mmr_ext) {  /* create new mmr */
>> +                vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls,
>> +                                ploc_set);
>> +            } else {  /* update existing mmr */
>> +                vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr,
>> +                                                          ploc_set);
>> +            }
>> 
>> -        vteprec_physical_locator_set_set_locators(ploc_set, locators,
>> -                                                  n_locators_new);
>> +        } else if (mmr_ext) {  /* remove old mmr */
>> +            vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
>> +        }
>>     }
>> +
>>     free(locators);
>> }
>> 
>> @@ -353,17 +361,19 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts,
>>             shash_add(&ls_node->physical_locators, chassis_ip, pl);
>>         }
>> 
>> -        char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
>> -        ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
>> +        if (!ls_node->mmr_ext) {
>> +            char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
>> +            ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
>> 
>> -        if (ls_node->mmr_ext &&
>> -            ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
>> +            if (ls_node->mmr_ext &&
>> +                ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
>> 
>> -            /* Delete the entry from the hash table so the mmr does not get
>> -             * removed from the DB later on during stale checking. */
>> -            shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
>> +                /* Delete the entry from the hash table so the mmr does not get
>> +                * removed from the DB later on during stale checking. */
>> +                shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
>> +            }
>> +            free(mac_tnlkey);
>>         }
>> -        free(mac_tnlkey);
>> 
>>         for (i = 0; i < port_binding_rec->n_mac; i++) {
>>             const struct vteprec_ucast_macs_remote *umr;
>> @@ -481,7 +491,7 @@ vtep_mcast_macs_cleanup(struct ovsdb_idl *vtep_idl)
>> 
>>     return true;
>> }
>> -
>> +
> 
> Form feed can stay.
> 
> Regards,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>


More information about the dev mailing list