BFD hardware offload for BGP sessions - #4253
Conversation
Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Baorong Liu <baorliu@cisco.com>
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Baorong Liu <baorliu@cisco.com>
a09e3a4 to
7eac609
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| std::string BfdLink::exec(const char* cmd) { | ||
| std::array<char, 128> buffer; | ||
| std::string result; | ||
| std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(cmd, "r"), pclose); |
There was a problem hiding this comment.
This line breaks the swss .deb build on Debian Trixie / GCC 14.2 with -Werror=ignored-attributes:
bfdsyncd/bfdlink.cpp:128:44: error: ignoring attributes on template argument 'int (*)(FILE*)' [-Werror=ignored-attributes]
pclose is declared in glibc with __attribute__((nonnull)), and that attribute is stripped when decltype(&pclose) is used as the Deleter template parameter to std::unique_ptr. GCC 13 (Bookworm) was silent about it, anyone moving to a Trixie-based slave image (sonic-slave-trixie) hits a hard build failure since swss builds -Werror.
| /* Remote discriminator. */ | ||
| if (field == "remote_discriminator") | ||
| { | ||
| uint32_t rid = (uint32_t)strtoll(string(value).c_str(), NULL, 10); |
There was a problem hiding this comment.
All four strtoll(value.c_str(), NULL, 10) calls in handleBfdStateUpdate silently return 0 on garbage input. A malformed remote_discriminator / remote_min_rx / remote_min_tx / remote_multiplier field in STATE_DB will be turned into a zero, packed into msg.data.state.*, and sent over the BFDDP socket to bfdd — the operator gets no signal that the state notification is corrupt.
Worth replacing with swss::to_uint<uint32_t>(value) inside a try/catch (std::exception&) so a bad row logs the offending field name and the function returns false before sending a partial frame.
| m_bfdTable.del(bfdkey); | ||
| m_key2bfd.erase(bfdkey_map); | ||
| /* the symptom observed that redis eliminates consecutive del and add transaction sometime, get wrong result. need to wait to make sure deletion done */ | ||
| usleep(100000); |
There was a problem hiding this comment.
Do the timer fields actually need a delete/recreate? Per saibfd.h, MIN_TX, MIN_RX, and MULTIPLIER are CREATE_AND_SET, so they should be modifiable in place via set_bfd_session_attribute. A delete/recreate bounces the session to Down momentarily — which can flap the BGP sessions BFD is protecting, defeating the point.
For the fields that are CREATE_ONLY (MULTIHOP, CBIT, SRC_IP_ADDRESS, DST_IP_ADDRESS, LOCAL_DISCRIMINATOR), delete/recreate is unavoidable
There was a problem hiding this comment.
The reason to use delete/recreate approach is to make it compatible with existing systems/vendors:
1, current orchagent/bfdorch does not support attribute update:
In create_bfd_session:
if (bfd_session_map.find(key) != bfd_session_map.end())
{
SWSS_LOG_ERROR("BFD session for %s already exists", key.c_str());
return true;
}
2, with the delete/recreate approach, parameter update from frr/bfdd still works even some vendor does not support BFD attribute update in SAI. so it is safe for all the vendors who already support BFD HW offload, even that vendor does not support BFD (HW offload) attribute update.
Comment from Baorong Liu
There was a problem hiding this comment.
Pull request overview
This PR adds support infrastructure for using hardware-offloaded BFD sessions (created/managed via SAI in orchagent) to feed BFD session updates to BGP/bfdd via a new bfdsyncd daemon, enabling hardware BFD to replace software BFD for BGP neighbor reachability detection.
Changes:
- Add new
bfdsyncddaemon that accepts FRR/bfdd dataplane protocol messages, programsAPP_BFD_SESSION_TABLE, and forwardsSTATE_DBBFD state updates back to the client. - Extend
BfdOrchstate-change handling to persist additional remote session parameters inSTATE_DB, and allowsrc_macinput when creating sessions withHW_LOOKUP_VALID=false. - Add a new mock unit test target for
bfdsyncdand wirebfdsyncdinto the build system.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile.am |
Adds bfdsyncd to the top-level build subdirectories. |
configure.ac |
Registers bfdsyncd/Makefile for autotools generation. |
orchagent/bfdorch.cpp |
Persists BFD remote parameters on state change; supports optional src_mac during session creation when HW lookup is invalid. |
bfdsyncd/Makefile.am |
Adds build rules for the new bfdsyncd binary. |
bfdsyncd/bfdsyncd.cpp |
Implements the main loop for the new daemon (accept client + select loop with STATE_DB subscription). |
bfdsyncd/bfdlink.h |
Declares the BfdLink selectable handling the bfdd dataplane protocol and DB interactions. |
bfdsyncd/bfdlink.cpp |
Implements bfdd dataplane message parsing, DB programming, and state/counter message responses. |
bfdsyncd/bfdd/bfddp_packet.h |
Adds the bfdd dataplane protocol structures/constants used by bfdsyncd. |
tests/mock_tests/Makefile.am |
Adds tests_bfdsyncd target to the mock test suite build. |
tests/mock_tests/bfdsyncd/test_bfdlink.cpp |
Adds unit tests covering basic session add/state-update/counter-request/delete flows. |
| /* Message header. */ | ||
| msg.header.version = BFD_DP_VERSION; | ||
| msg.header.length = ntohs(msglen); | ||
| msg.header.type = ntohs(BFD_STATE_CHANGE); | ||
|
|
| BfdLink::~BfdLink() | ||
| { | ||
| delete[] m_messageBuffer; | ||
| if (m_connected) | ||
| close(m_connection_socket); | ||
| if (m_server_up) | ||
| close(m_server_socket); | ||
| } |
| m_connection_socket = ::accept(m_server_socket, (struct sockaddr *)&client_addr, | ||
| &client_len); | ||
| if (m_connection_socket < 0) | ||
| throw system_error(errno, system_category()); | ||
|
|
||
| SWSS_LOG_WARN("New connection accepted from: %s\n", inet_ntoa(client_addr.sin_addr)); | ||
| } |
| /* check link local ip address 169.254.0.0/16 0xa9fe0000 */ | ||
| if ((inet_pton(AF_INET, dst_addr, &v4) == 1) && ((v4.s_addr & 0x0000ffff) == 0x0000fea9)) { | ||
| is_linklocal = true; | ||
| SWSS_LOG_INFO("dst_addr %s is a link local ip address", dst_addr); | ||
| } |
| while ((c = getopt (argc, argv, "hdp:")) != -1) | ||
| switch (c) | ||
| { | ||
| case 'h': | ||
| cout << "Usage: bfdsyncd -d -p <tcp port number>" << endl; | ||
| break; | ||
| case 'd': |
| #ifndef __BFDLINK__ | ||
| #define __BFDLINK__ |
| * Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| * of this software and associated documentation files (the ?Software?), to | ||
| * deal in the Software without restriction, including without limitation the |
| * THE SOFTWARE IS PROVIDED ?AS IS?, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, |
|
@baorliu , please address copilot comments and resolve conflict. |
Merge PR sonic-net#4253 and address review feedback: fix GCC 14 pclose deleter build failure, validate STATE_DB numeric fields, correct BFDDP header byte order, link-local detection, resource cleanup, and master merge conflicts in mock_tests Makefile. Use ephemeral port 0 in unit tests to avoid CI bind conflicts on the default BFDDP port. Signed-off-by: Sridhar talari <stalarir@cisco.com>
|
Closing in favor of #4676 |
Original PR 3267 was messed up when trying to fix DCO error. this one is the replacement for that PR for code change reference.
For detailed description and commit history, please refer the original PR:
#3267
What I did
Provide an option to use hardware offloaded BFD to replace software BFD (frr/bfdd) for BGP neighbor reachability detection.
Why I did it
Hardware offloaded BFD usually provide more bfd sessions support, and has shorter detection time comparing to software bfd. and also lower CPU load because that BFD sessions are offloaded to hardware/ASIC.
How I verified it
Details if related
This program is built under sonic-swss, but running in bgp container.
HLD in PR review:
sonic-net/SONiC#1599
[notes] some commit history are lost after a force push after fixing a DCO error.
git log before the force push: