Initial support of VxLAN BGP EVPN #99
Conversation
oholubnychyy
commented
Jul 5, 2024
oholubnychyy
commented
- Implemented create/remove VxLAN tunnel offloading to VPP
- Enabled VPP's vxlan_plugin.so by default
- Implemented but not used create/remove VxLAN-GRE tunnel offloading
- Implemented create/remove VxLAN tunnel offloading to VPP - Enabled VPP's vxlan_plugin.so by default - Implemented but not used create/remove VxLAN-GRE tunnel offloading Signed-off-by: Oleksiy Holubnychyy <[email protected]>
|
It looks like we both are working on vxlan but from different angle. There are certainly some overlapping our PRs (mine is #97). Let's work together to resolve the conflicts. |
|
|
||
| S(mp); | ||
|
|
||
| W(ret); |
There was a problem hiding this comment.
Does it reliably return if_ind? I found W macro returns whenever there is a message received. If there is an unsolicited event like interface up/down, it can make W return before the callback is invoked. If this happens, if_ind will have invalid data because the tunnel_add_del callback has been called until the next time W is called. (I see you call refresh_interfaces_list in the caller before accessing if_ind, which calls W multiple times. You may not see the issue)
I am thinking to introduce a resource pool in PR #97 to manager vxlan instance id and pass it in to vpp. Then you don't need to query interface name from sw_if_index. What do you think?
There was a problem hiding this comment.
I believe if_ind is returned correctly.
W() busy-waits on vam->result_ready, it is not used in vl_api_sw_interface_event_t_handler().
So I see no problem with my implementation of vpp_vxlan_tunnel_add_del().
In fact I followed already existing pattern. For example, have a look at implementation of vpp_acl_add_replace()
If I'm still missing your point please be more specific and point exact lines in the code which you think lead to race conditions.
There was a problem hiding this comment.
Please check W implementation: https://github.com/FDio/vpp/blob/master/src/vlibapi/vat_helper_macros.h#L99. At line 99, vl_socket_client_read returns whenever it receives a message. If it is an unsolicited event, the callback of you API won't be invoked and result_ready won't be set. line 100 in W will time out. ret will be -99, which will be returned from the tunnel_add_del API. The check on return status in the caller will fail.
| } | ||
|
|
||
|
|
||
| int vpp_vxlan_gpe_tunnel_add_del(vpp_ip_addr_t *sip, vpp_ip_addr_t *dip, uint32_t vni, bool is_add, uint32_t *if_ind) |
There was a problem hiding this comment.
Is this function used? I didn't find any callers
There was a problem hiding this comment.
Indeed, vpp_vxlan_gpe_tunnel_add_del() is not used in this PR.
I started with this API but due to its limitations had to use API without GPE.
I just didn't want to throw out the API which works, it doesn't cause any harm now and may be used later.
| _In_ const sai_attribute_t *decap_attr, | ||
| _Out_ const sai_attribute_t **vni_attr) | ||
| { | ||
| for (const auto &obj_it : m_objectHash.at(SAI_OBJECT_TYPE_TUNNEL_MAP_ENTRY)) { |
There was a problem hiding this comment.
This is very inefficient to go through all tunnel_map_entries. In #97, I have the same need to find tunnel_map_entries from tunnel_map. To efficiently process that, I introduced SaiObjectDB, which dynamically build the relationship on demand. Please take a look.
There was a problem hiding this comment.
m_objectHash is the DB for SAI objects. If you are going to create another DB for SAI objects then I would say this is not a great idea.
For me the rule of thumb is to avoid at all costs keeping same data in more then one place. Otherwise you are responsible for keeping the data in sync between different places which significantly complicates the implementation.
The other reason not to create another DB is the current implementation. If you have a look at the code you'll not find an example of using another DB for particular feature.
There was a problem hiding this comment.
It's trade off. The more code you add the more complex. But this is necessary.
- SAI objects are basically a graph. Using a flat hash table is not appropriate. With proper graph data structure, we can improve navigation efficiency for the situation you have.
- You can see how many customized map to maintain the relationship: m_acl_tbl_rules_map, m_acl_tbl_grp_mbr_map, m_nxthop_grp_mbr_map .... The goal of SaiObjectDB is to eliminate those maps with common and standard implementation. So we don't need to repeat adding such code that will increase the risk of adding more bugs.
- Using proper OO methodology can improve code readability and modularity.
Adding some infra has the risk of short term instability but the benefit pays off in the long run.
