From 8bda5141b733d4491d413b263e2e607178df27f8 Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Mon, 15 Jun 2026 07:06:48 -0700 Subject: [PATCH 1/7] feat: dedicated event socket and thread for VPP async events Carry the asynchronous VPP event stream on a dedicated binary-API socket (event_socket_client_main / vat_event_main) and background thread, separate from the synchronous request/reply command path. Adds event-connection M_EV/ PING_EV/S_EV/WR_EV macros (using the explicit per-connection socket via vl_socket_client_*2), a dedicated lock for the event connection, and makes __plugin_msg_base thread-local so the command and event threads do not race. Signed-off-by: Nicholas Ching --- vslib/vpp/vppxlate/SaiVppXlate.c | 262 ++++++++++++++++++++++++++----- 1 file changed, 227 insertions(+), 35 deletions(-) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 3cd77edf28..b97e0fb736 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -414,12 +414,18 @@ do { \ */ #define WR(ret) \ do { \ - f64 timeout = vat_time_now (vam) + 1.0; \ + f64 start_time = vat_time_now (vam); \ + f64 hard_deadline = start_time + 10.0; \ + f64 idle_deadline = start_time + 1.0; \ socket_client_main_t *scm = vam->socket_client_main; \ ret = -99; \ - while (vat_time_now (vam) < timeout) { \ - if (scm && scm->socket_enable) \ - vl_socket_client_read (5); \ + while (1) { \ + f64 now = vat_time_now (vam); \ + if (now >= hard_deadline || now >= idle_deadline) \ + break; \ + if (scm && scm->socket_enable) { \ + vl_socket_client_read (1); \ + } \ if (vam->result_ready == 1) { \ ret = vam->retval; \ break; \ @@ -428,6 +434,74 @@ do { \ } \ } while(0); +/* + * Event-connection variants of M / S / PING / WR. + * + * The asynchronous VPP event stream is carried on a dedicated binary-API + * socket (event_socket_client_main / vat_event_main) that is independent of + * the synchronous request/reply socket used by the command path. These macros + * therefore operate on the explicit "scm" passed via vam->socket_client_main + * using the vl_socket_client_*2() APIs instead of the global-socket helpers. + */ +#define M_EV(T, mp) \ +do { \ + socket_client_main_t *scm = vam->socket_client_main; \ + vam->result_ready = 0; \ + if (scm && scm->socket_enable) \ + mp = vl_socket_client_msg_alloc2 (scm, (int)sizeof(*mp)); \ + else \ + mp = vl_msg_api_alloc_as_if_client((int)sizeof(*mp)); \ + clib_memset (mp, 0, sizeof (*mp)); \ + mp->_vl_msg_id = ntohs (VL_API_##T+__plugin_msg_base); \ + mp->client_index = vam->my_client_index; \ +} while(0); + +#define PING_EV(mp_ping) \ +do { \ + socket_client_main_t *scm = vam->socket_client_main; \ + if (scm && scm->socket_enable) \ + mp_ping = vl_socket_client_msg_alloc2 (scm, (int)sizeof (*mp_ping)); \ + else \ + mp_ping = vl_msg_api_alloc_as_if_client ((int)sizeof (*mp_ping)); \ + clib_memset (mp_ping, 0, sizeof (*mp_ping)); \ + mp_ping->_vl_msg_id = htons (VL_API_CONTROL_PING + 1); \ + mp_ping->client_index = vam->my_client_index; \ + vam->result_ready = 0; \ + if (scm) \ + scm->control_pings_outstanding++; \ +} while(0); + +#define S_EV(mp) \ +do { \ + socket_client_main_t *scm = vam->socket_client_main; \ + if (scm && scm->socket_enable) \ + vl_socket_client_write2 (scm); \ + else \ + vl_msg_api_send_shmem (vam->vl_input_queue, (u8 *)&mp); \ +} while (0); + +#define WR_EV(ret) \ +do { \ + f64 start_time = vat_time_now (vam); \ + f64 hard_deadline = start_time + 10.0; \ + f64 idle_deadline = start_time + 1.0; \ + socket_client_main_t *scm = vam->socket_client_main; \ + ret = -99; \ + while (1) { \ + f64 now = vat_time_now (vam); \ + if (now >= hard_deadline || now >= idle_deadline) \ + break; \ + if (scm && scm->socket_enable) { \ + vl_socket_client_read2 (scm, 1); \ + } \ + if (vam->result_ready == 1) { \ + ret = vam->retval; \ + break; \ + } \ + vat_suspend (vam->vlib_main, 1e-5); \ + } \ +} while(0); + #define VPP_MAX_CTX 16 #define VPP_CTX_INDEX_MASK 0xff #define VPP_CTX_GENERATION_SHIFT 8 @@ -505,6 +579,31 @@ void vpp_mutex_unlock () #define VPP_LOCK() vpp_mutex_lock() #define VPP_UNLOCK() vpp_mutex_unlock() +/* + * Dedicated lock for the asynchronous event connection (vat_event_main / + * event_socket_client_main). Kept separate from vpp_mutex so the background + * event-polling thread never serializes against the synchronous command path. + */ +static pthread_mutex_t vpp_event_mutex; + +void vpp_event_mutex_lock_init () +{ + pthread_mutex_init(&vpp_event_mutex, NULL); +} + +void vpp_event_mutex_lock () +{ + pthread_mutex_lock(&vpp_event_mutex); +} + +void vpp_event_mutex_unlock () +{ + pthread_mutex_unlock(&vpp_event_mutex); +} + +#define EVENT_LOCK() vpp_event_mutex_lock() +#define EVENT_UNLOCK() vpp_event_mutex_unlock() + /* * Right now configuration is done synchronously in a single thread. * When the need arises for multiple requests in pipeline we can move to a pool to @@ -570,6 +669,29 @@ vat_main_t vat_main; uword *interface_name_by_sw_index = NULL; uword *link_speed_by_sw_index = NULL; +/* + * Dedicated binary-API connection for the asynchronous VPP event stream + * (sw_interface_event / bfd events). Owned by the background event-polling + * thread and isolated from the synchronous request/reply connection so the + * unsolicited event flood cannot starve or deadlock the command path. + */ +static socket_client_main_t event_socket_client_main; +vat_main_t vat_event_main; + +/* + * Per-thread "current" vat_main. Reply handlers run in the context of whatever + * thread is reading its connection's socket; they use cur_vam() so that a reply + * read on a given connection updates only that connection's result state. + * Defaults to the command connection (&vat_main) when unset. + */ +static __thread vat_main_t *tl_cur_vam; + +static inline vat_main_t *cur_vam (void) +{ + return tl_cur_vam ? tl_cur_vam : &vat_main; +} + + f64 vat_time_now (vat_main_t * vam) { @@ -691,7 +813,7 @@ vl_noop_handler (void *mp) static void set_reply_status (int retval) { - vat_main_t *vam = &vat_main; + vat_main_t *vam = cur_vam(); if (vam->async_mode) { @@ -706,14 +828,14 @@ static void set_reply_status (int retval) static void set_reply_sw_if_index (vl_api_interface_index_t sw_if_index) { - vat_main_t *vam = &vat_main; + vat_main_t *vam = cur_vam(); vam->sw_if_index = sw_if_index; } static void vl_api_control_ping_reply_t_handler (vl_api_control_ping_reply_t *mp) { - vat_main_t *vam = &vat_main; + vat_main_t *vam = cur_vam(); set_reply_status((int)ntohl ((uint32_t)mp->retval)); @@ -734,7 +856,7 @@ vl_api_sw_interface_event_t_handler (vl_api_sw_interface_event_t *mp) uint32_t flags, sw_if_index; uword *ptr; - sw_if_index = htonl(mp->sw_if_index); + sw_if_index = ntohl(mp->sw_if_index); ptr = hash_get(interface_name_by_sw_index, sw_if_index); if (NULL == ptr) { SAIVPP_INFO("vpp cannot get interface name for sw index %u", sw_if_index); @@ -742,7 +864,7 @@ vl_api_sw_interface_event_t_handler (vl_api_sw_interface_event_t *mp) } const char *hw_ifname = (const char *) ptr[0]; - flags = htonl(mp->flags); + flags = ntohl(mp->flags); if (flags & IF_STATUS_API_FLAG_ADMIN_UP && !(flags & IF_STATUS_API_FLAG_LINK_UP)) { return; @@ -1371,7 +1493,14 @@ vl_api_sr_set_encap_source_reply_t_handler(vl_api_sr_set_encap_source_reply_t *m _(MEMCLNT_MSG_ID(GET_FIRST_MSG_ID_REPLY), get_first_msg_id_reply) \ _(MEMCLNT_MSG_ID(CONTROL_PING_REPLY), control_ping_reply) -static u16 interface_msg_id_base, memclnt_msg_id_base, __plugin_msg_base; +static u16 interface_msg_id_base, memclnt_msg_id_base; +/* + * __plugin_msg_base selects the message-id base for the message currently + * being constructed. The command path and the dedicated event-connection path + * run on different threads under different locks, so this must be thread-local + * to avoid a data race between "set base" and the immediately following M()/PING(). + */ +static __thread u16 __plugin_msg_base; static u16 l2_msg_id_base, vxlan_msg_id_base, ipip_msg_id_base; static u16 tunterm_msg_id_base; static u16 bfd_msg_id_base; @@ -1673,6 +1802,53 @@ vsc_socket_connect (vat_main_t * vam, char *client_name) return 0; } +/* + * Establish the dedicated event connection. Uses the explicit-scm + * vl_socket_client_connect2() so it is a fully independent client connection + * (its own VPP client_index and socket fd). Deliberately does NOT touch the + * global api_main->my_client_index, which belongs to the command connection; + * messages on this connection carry vat_event_main.my_client_index via the + * M_EV/PING_EV macros. + */ +static int +vsc_event_socket_connect (vat_main_t * vam, char *client_name) +{ + int rv; + vam->socket_client_main = &event_socket_client_main; + if ((rv = vl_socket_client_connect2 (&event_socket_client_main, + (char *) vam->socket_name, + client_name, + 0 /* default socket rx, tx buffer */ ))) + return rv; + + /* vpp expects the client index in network order */ + vam->my_client_index = (u32)htonl (event_socket_client_main.client_index); + return 0; +} + +static int init_vpp_event_client () +{ + char client_name[] = "sonic_vpp_event_client"; + vat_main_t *vam = &vat_event_main; + + vpp_event_mutex_lock_init(); + + clib_time_init (&vam->clib_time); + vam->socket_name = format (0, "%s%c", API_SOCKET_FILE, 0); + /* vat_suspend()/vat_time_now() only need clib_time; reuse the command + * vlib_main pointer to keep the helper macros happy. */ + vam->vlib_main = vat_main.vlib_main; + + if (vsc_event_socket_connect(vam, client_name) == 0) { + SAIVPP_INFO("vpp event socket connect successful\n"); + return 0; + } + + SAIVPP_ERROR("vpp event socket connect failed\n"); + return -1; +} + + typedef struct { u8 *name; @@ -2006,14 +2182,24 @@ int init_vpp_client() vpp_lcp_ethertype_enable(0x0806); /* - * SONiC periodically polls the port status so currently there is no need for - * async notification. This also simplifies the synchronous design of saivpp. - * Revisit the async mechanism if there is greater reason. + * Bring up the dedicated event connection before subscribing to any + * VPP event source. The want_*_events registrations below are issued + * on this connection, so VPP delivers all unsolicited notifications to + * the event socket, keeping the command socket reply-only. */ - vpp_intf_events_enable_disable(true); - - /* Register with VPP for BFD notifications */ - vpp_bfd_events_enable_disable(true); + if (init_vpp_event_client() != 0) { + SAIVPP_ERROR("vpp event client init failed; async events disabled\n"); + } else { + /* + * SONiC periodically polls the port status so currently there is no need for + * async notification. This also simplifies the synchronous design of saivpp. + * Revisit the async mechanism if there is greater reason. + */ + vpp_intf_events_enable_disable(true); + + /* Register with VPP for BFD notifications */ + vpp_bfd_events_enable_disable(true); + } /* Enable BFD multihop support in VPP */ vpp_bfd_udp_enable_multihop(); @@ -2131,25 +2317,27 @@ int set_interface_vrf (const char *hwif_name, u32 sub_id, u32 vrf_id, bool is_ip static int vpp_intf_events_enable_disable (bool enable) { - vat_main_t *vam = &vat_main; + vat_main_t *vam = &vat_event_main; vl_api_want_interface_events_t *mp; int ret; - VPP_LOCK(); + EVENT_LOCK(); + tl_cur_vam = &vat_event_main; __plugin_msg_base = interface_msg_id_base; - M (WANT_INTERFACE_EVENTS, mp); + M_EV (WANT_INTERFACE_EVENTS, mp); mp->enable_disable = enable; mp->pid = htonl((uint32_t)getpid()); - S (mp); - WR (ret); + S_EV (mp); + WR_EV (ret); if (ret) { SAIVPP_ERROR("%s failed(%d) enable %d", __func__, ret, enable); } else { SAIVPP_INFO("%s enable %d", __func__, enable); } - VPP_UNLOCK(); + tl_cur_vam = NULL; + EVENT_UNLOCK(); return ret; } @@ -3162,21 +3350,23 @@ int vpp_get_interface_speed (const char *hwif_name, uint32_t *speed) int vpp_sync_for_events () { - vat_main_t *vam = &vat_main; + vat_main_t *vam = &vat_event_main; vl_api_control_ping_t *mp_ping; int ret; - VPP_LOCK(); + EVENT_LOCK(); + tl_cur_vam = &vat_event_main; /* Use a control ping for synchronization */ __plugin_msg_base = memclnt_msg_id_base; - PING (NULL, mp_ping); - S (mp_ping); + PING_EV (mp_ping); + S_EV (mp_ping); - WR (ret); + WR_EV (ret); - VPP_UNLOCK(); + tl_cur_vam = NULL; + EVENT_UNLOCK(); return ret; } @@ -4033,25 +4223,27 @@ int bfd_udp_del(bool multihop, const char *hwif_name, vpp_ip_addr_t *local_addr, static int vpp_bfd_events_enable_disable (bool enable) { - vat_main_t *vam = &vat_main; + vat_main_t *vam = &vat_event_main; vl_api_want_bfd_events_t *mp; int ret; - VPP_LOCK(); + EVENT_LOCK(); + tl_cur_vam = &vat_event_main; __plugin_msg_base = bfd_msg_id_base; - M (WANT_BFD_EVENTS, mp); + M_EV (WANT_BFD_EVENTS, mp); mp->enable_disable = enable; mp->pid = htonl((uint32_t)getpid()); - S (mp); - WR (ret); + S_EV (mp); + WR_EV (ret); if (ret) { SAIVPP_ERROR("%s failed(%d) enable %d", __func__, ret, enable); } else { SAIVPP_INFO("%s enable %d", __func__, enable); } - VPP_UNLOCK(); + tl_cur_vam = NULL; + EVENT_UNLOCK(); return ret; } From fa2d9ed56bc997b36bfc1a09abaef76aca84848a Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Thu, 2 Jul 2026 10:52:47 -0700 Subject: [PATCH 2/7] fix: extend idle deadline on successful socket read in WR macros Reset idle_deadline when vl_socket_client_read returns data so bursts of unsolicited events do not expire the wait before the reply arrives. Apply the same logic to WR_EV on the dedicated event socket. Signed-off-by: Nicholas Ching --- vslib/vpp/vppxlate/SaiVppXlate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index b97e0fb736..0c82597bb5 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -424,7 +424,12 @@ do { \ if (now >= hard_deadline || now >= idle_deadline) \ break; \ if (scm && scm->socket_enable) { \ - vl_socket_client_read (1); \ + int _wr_rv = vl_socket_client_read (1); \ + if (_wr_rv == 0) { \ + idle_deadline = vat_time_now (vam) + 1.0; \ + if (idle_deadline > hard_deadline) \ + idle_deadline = hard_deadline; \ + } \ } \ if (vam->result_ready == 1) { \ ret = vam->retval; \ @@ -492,7 +497,12 @@ do { \ if (now >= hard_deadline || now >= idle_deadline) \ break; \ if (scm && scm->socket_enable) { \ - vl_socket_client_read2 (scm, 1); \ + int _wr_rv = vl_socket_client_read2 (scm, 1); \ + if (_wr_rv == 0) { \ + idle_deadline = vat_time_now (vam) + 1.0; \ + if (idle_deadline > hard_deadline) \ + idle_deadline = hard_deadline; \ + } \ } \ if (vam->result_ready == 1) { \ ret = vam->retval; \ From fce3891de74f8a321ca8f5260e6cfa9706e93fc0 Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Thu, 2 Jul 2026 10:52:57 -0700 Subject: [PATCH 3/7] docs: document EVENT_LOCK and VPP_LOCK ordering rule Event-connection paths must not acquire VPP_LOCK while holding EVENT_LOCK to avoid deadlock with the synchronous command path. Signed-off-by: Nicholas Ching --- vslib/vpp/vppxlate/SaiVppXlate.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 0c82597bb5..43093e9bc2 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -593,6 +593,10 @@ void vpp_mutex_unlock () * Dedicated lock for the asynchronous event connection (vat_event_main / * event_socket_client_main). Kept separate from vpp_mutex so the background * event-polling thread never serializes against the synchronous command path. + * + * Lock ordering: code running under EVENT_LOCK must never acquire VPP_LOCK. + * The command path holds VPP_LOCK and may wait on the event connection; + * acquiring VPP_LOCK while holding EVENT_LOCK would deadlock. */ static pthread_mutex_t vpp_event_mutex; @@ -3358,6 +3362,10 @@ int vpp_get_interface_speed (const char *hwif_name, uint32_t *speed) return 0; } +/* + * Synchronize with VPP via the dedicated event socket. Runs under EVENT_LOCK; + * must not acquire VPP_LOCK (see EVENT_LOCK comment above). + */ int vpp_sync_for_events () { vat_main_t *vam = &vat_event_main; From a402c743c397f72df0ae0ed9ef0d43720e1c6ec3 Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Fri, 3 Jul 2026 07:18:19 -0700 Subject: [PATCH 4/7] fix: self-heal VPP port oper-status and reconnect event socket Add level-triggered oper-status resync each event-poll cycle so missed LINK_UP edges after reboot/config-reload cannot leave ports stuck DOWN in STATE_DB. Make the dedicated event socket detect read failures, reconnect, and re-subscribe to interface/BFD events. Signed-off-by: Nicholas Ching --- vslib/vpp/SwitchVpp.h | 4 ++ vslib/vpp/SwitchVppRif.cpp | 49 +++++++++++++++++ vslib/vpp/vppxlate/SaiVppXlate.c | 94 +++++++++++++++++++++++++++++--- 3 files changed, 138 insertions(+), 9 deletions(-) diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index 26eab9f10e..bf8758b9b2 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -1155,6 +1155,8 @@ namespace saivs void vppProcessEvents (); + void resyncPortOperStatus(); + void startVppEventsThread(); private: // VPP @@ -1251,6 +1253,8 @@ namespace saivs std::map> m_hostif_info_map; + std::map m_last_oper_up; + CRMTracker m_crmTracker; bool isIPv4Route(const std::string &serializedObjectId); diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 2a07d209b9..942bdfeca8 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -385,6 +385,50 @@ bool SwitchVpp::vpp_get_hwif_name ( return true; } +void SwitchVpp::resyncPortOperStatus() +{ + SWSS_LOG_ENTER(); + + for (auto& kvp: m_hostif_info_map) + { + const std::string& tapname = kvp.first; + auto& info = kvp.second; + + if (!info) + { + continue; + } + + const char* dev = tapname.c_str(); + const char* hwif_name = tap_to_hwif_name(dev); + + bool link_up = false; + + if (interface_get_state(hwif_name, &link_up) != 0) + { + continue; + } + + auto prev_it = m_last_oper_up.find(tapname); + bool have_prev = (prev_it != m_last_oper_up.end()); + + if (have_prev && prev_it->second == link_up) + { + continue; + } + + m_last_oper_up[tapname] = link_up; + + auto state = link_up ? SAI_PORT_OPER_STATUS_UP : SAI_PORT_OPER_STATUS_DOWN; + + send_port_oper_status_notification(info->m_portId, state, false); + + SWSS_LOG_NOTICE("resync oper-status %s(%s) %s", + hwif_name, dev, + link_up ? "UP" : "DOWN"); + } +} + void SwitchVpp::vppProcessEvents () { SWSS_LOG_ENTER(); @@ -397,6 +441,10 @@ void SwitchVpp::vppProcessEvents () nanosleep(&req, NULL); ret = vpp_sync_for_events(); SWSS_LOG_NOTICE("Checking for any VS events status %d", ret); + if (ret < 0) + { + SWSS_LOG_WARN("vpp_sync_for_events failed (%d); event socket reconnect attempted", ret); + } while ((evp = vpp_ev_dequeue())) { if (evp->type == VPP_INTF_LINK_STATUS) { asyncIntfStateUpdate(evp->data.intf_status.hwif_name, @@ -414,6 +462,7 @@ void SwitchVpp::vppProcessEvents () } vpp_ev_free(evp); } + resyncPortOperStatus(); } } diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 43093e9bc2..3a010ca11c 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -498,6 +498,10 @@ do { \ break; \ if (scm && scm->socket_enable) { \ int _wr_rv = vl_socket_client_read2 (scm, 1); \ + if (_wr_rv < 0) { \ + ret = _wr_rv; \ + break; \ + } \ if (_wr_rv == 0) { \ idle_deadline = vat_time_now (vam) + 1.0; \ if (idle_deadline > hard_deadline) \ @@ -566,6 +570,9 @@ static void vpp_evq_init () static int vpp_acl_counters_enable_disable(bool enable); static int vpp_intf_events_enable_disable(bool enable); static int vpp_bfd_events_enable_disable(bool enable); +static int vpp_event_client_setup(void); +static int vpp_event_connect(void); +static int vpp_event_reconnect(void); static int vpp_bfd_udp_enable_multihop(); static int vpp_lcp_ethertype_enable(u16 ethertype); @@ -692,6 +699,9 @@ uword *link_speed_by_sw_index = NULL; static socket_client_main_t event_socket_client_main; vat_main_t vat_event_main; +static int event_client_connected = 0; +static int event_mutex_initialized = 0; + /* * Per-thread "current" vat_main. Reply handlers run in the context of whatever * thread is reading its connection's socket; they use cur_vam() so that a reply @@ -1840,28 +1850,80 @@ vsc_event_socket_connect (vat_main_t * vam, char *client_name) return 0; } -static int init_vpp_event_client () +static int init_vpp_event_client (void) +{ + return vpp_event_connect(); +} + +static int +vpp_event_client_setup (void) { - char client_name[] = "sonic_vpp_event_client"; vat_main_t *vam = &vat_event_main; + static int vam_setup_done = 0; - vpp_event_mutex_lock_init(); + if (!event_mutex_initialized) + { + vpp_event_mutex_lock_init(); + event_mutex_initialized = 1; + } - clib_time_init (&vam->clib_time); - vam->socket_name = format (0, "%s%c", API_SOCKET_FILE, 0); - /* vat_suspend()/vat_time_now() only need clib_time; reuse the command - * vlib_main pointer to keep the helper macros happy. */ - vam->vlib_main = vat_main.vlib_main; + if (!vam_setup_done) + { + clib_time_init (&vam->clib_time); + vam->socket_name = format (0, "%s%c", API_SOCKET_FILE, 0); + vam->vlib_main = vat_main.vlib_main; + vam_setup_done = 1; + } + + return 0; +} + +static int +vpp_event_connect (void) +{ + char client_name[] = "sonic_vpp_event_client"; + vat_main_t *vam = &vat_event_main; - if (vsc_event_socket_connect(vam, client_name) == 0) { + vpp_event_client_setup(); + + if (vsc_event_socket_connect(vam, client_name) == 0) + { + event_client_connected = 1; SAIVPP_INFO("vpp event socket connect successful\n"); return 0; } + event_client_connected = 0; SAIVPP_ERROR("vpp event socket connect failed\n"); return -1; } +static int +vpp_event_reconnect (void) +{ + vat_main_t *vam = &vat_event_main; + + SAIVPP_INFO("attempting vpp event socket reconnect\n"); + + EVENT_LOCK(); + vl_socket_client_disconnect2 (&event_socket_client_main); + event_client_connected = 0; + vam->result_ready = 0; + EVENT_UNLOCK(); + + if (vpp_event_connect() != 0) + return -1; + + if (vpp_intf_events_enable_disable(true) != 0) + return -1; + + if (vpp_bfd_events_enable_disable(true) != 0) + return -1; + + SAIVPP_INFO("vpp event socket reconnect successful\n"); + return 0; +} + typedef struct { @@ -3372,9 +3434,17 @@ int vpp_sync_for_events () vl_api_control_ping_t *mp_ping; int ret; + if (!event_client_connected) + { + if (vpp_event_reconnect() != 0) + return -1; + } + EVENT_LOCK(); tl_cur_vam = &vat_event_main; + vam->result_ready = 0; + /* Use a control ping for synchronization */ __plugin_msg_base = memclnt_msg_id_base; @@ -3386,6 +3456,12 @@ int vpp_sync_for_events () tl_cur_vam = NULL; EVENT_UNLOCK(); + if (ret < 0) + { + SAIVPP_ERROR("vpp_sync_for_events failed (%d), reconnecting event socket\n", ret); + vpp_event_reconnect(); + } + return ret; } From 4310b46636bcab7c5de32779dd5862c845ccb6bc Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Tue, 7 Jul 2026 12:09:35 -0700 Subject: [PATCH 5/7] fix: serialize VPP interface table access between command and event threads Signed-off-by: Nicholas Ching --- vslib/vpp/vppxlate/SaiVppXlate.c | 71 +++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 6 deletions(-) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 3a010ca11c..ee3625d5ee 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -625,6 +625,27 @@ void vpp_event_mutex_unlock () #define EVENT_LOCK() vpp_event_mutex_lock() #define EVENT_UNLOCK() vpp_event_mutex_unlock() +/* + * Leaf lock protecting the shared interface lookup tables + * (vat_main.sw_if_index_by_interface_name, interface_name_by_sw_index and + * link_speed_by_sw_index). These tables are read and written from both the + * synchronous command path (under VPP_LOCK) and the asynchronous event + * handler (under EVENT_LOCK), so neither connection lock on its own provides + * mutual exclusion over them; a concurrent read during a write/rehash/free + * corrupts the hash and crashes syncd. + * + * Lock ordering: this is a leaf lock. It may be acquired while holding + * VPP_LOCK or EVENT_LOCK, but code holding it must never acquire VPP_LOCK, + * EVENT_LOCK, or re-acquire this lock (keep the critical sections to the raw + * hash operations only). This keeps the EVENT_LOCK-never-acquires-VPP_LOCK + * ordering rule above intact. Statically initialized so it is valid before + * any thread (including the background event thread) starts. + */ +static pthread_mutex_t vpp_intf_table_mutex = PTHREAD_MUTEX_INITIALIZER; + +#define INTF_TABLE_LOCK() pthread_mutex_lock(&vpp_intf_table_mutex) +#define INTF_TABLE_UNLOCK() pthread_mutex_unlock(&vpp_intf_table_mutex) + /* * Right now configuration is done synchronously in a single thread. * When the need arises for multiple requests in pipeline we can move to a pool to @@ -881,12 +902,22 @@ vl_api_sw_interface_event_t_handler (vl_api_sw_interface_event_t *mp) uword *ptr; sw_if_index = ntohl(mp->sw_if_index); + + /* + * Copy the interface name out while holding the table lock; the pointer + * returned by hash_get points into interface_name_by_sw_index, which the + * command path may concurrently rewrite/free under VPP_LOCK. + */ + char hw_ifname[64]; + INTF_TABLE_LOCK(); ptr = hash_get(interface_name_by_sw_index, sw_if_index); if (NULL == ptr) { + INTF_TABLE_UNLOCK(); SAIVPP_INFO("vpp cannot get interface name for sw index %u", sw_if_index); return; } - const char *hw_ifname = (const char *) ptr[0]; + snprintf(hw_ifname, sizeof(hw_ifname), "%s", (const char *) ptr[0]); + INTF_TABLE_UNLOCK(); flags = ntohl(mp->flags); if (flags & IF_STATUS_API_FLAG_ADMIN_UP && @@ -910,7 +941,7 @@ vl_api_sw_interface_event_t_handler (vl_api_sw_interface_event_t *mp) vpp_intf_status_t *stp = &evinfo->data.intf_status; stp->link_up = link_up; - strncpy(stp->hwif_name, hw_ifname, sizeof(stp->hwif_name) -1); + snprintf(stp->hwif_name, sizeof(stp->hwif_name), "%s", hw_ifname); vpp_ev_enqueue(evinfo); } @@ -931,12 +962,14 @@ vl_api_sw_interface_details_t_handler (vl_api_sw_interface_details_t *mp) vat_main_t *vam = &vat_main; u8 *s = format (0, "%s%c", mp->interface_name, 0); + INTF_TABLE_LOCK(); hash_set_mem (vam->sw_if_index_by_interface_name, s, ntohl (mp->sw_if_index)); hash_set (interface_name_by_sw_index, ntohl (mp->sw_if_index), s); /* Save link speed (in Kbps) per interface */ hash_set (link_speed_by_sw_index, ntohl (mp->sw_if_index), ntohl (mp->link_speed)); + INTF_TABLE_UNLOCK(); /* In sub interface case, fill the sub interface table entry */ if (mp->sw_if_index != mp->sup_sw_if_index) @@ -1943,6 +1976,14 @@ api_sw_interface_dump (vat_main_t *vam) VPP_LOCK(); + /* + * Toss and recreate the name tables atomically w.r.t. the event handler, + * which reads interface_name_by_sw_index under INTF_TABLE_LOCK. The lock is + * released before the dump below because WR() dispatches the + * sw_interface_details handler, which re-acquires INTF_TABLE_LOCK. + */ + INTF_TABLE_LOCK(); + /* Toss the old name table */ hash_foreach_pair (p, vam->sw_if_index_by_interface_name, ({ vec_add2 (nses, ns, 1); @@ -1969,6 +2010,9 @@ api_sw_interface_dump (vat_main_t *vam) /* recreate the interface name hash table */ vam->sw_if_index_by_interface_name = hash_create_string (0, sizeof (uword)); + + INTF_TABLE_UNLOCK(); + __plugin_msg_base = interface_msg_id_base; /* * Ask for all interface names. Otherwise, the epic catalog of @@ -2013,11 +2057,13 @@ dump_interface_table (vat_main_t *vam) return -99; } + INTF_TABLE_LOCK(); hash_foreach_pair (p, vam->sw_if_index_by_interface_name, ({ vec_add2 (nses, ns, 1); ns->name = (u8 *) (p->key); ns->value = (u32) p->value[0]; })); + INTF_TABLE_UNLOCK(); vec_sort_with_function (nses, name_sort_cmp); @@ -2035,13 +2081,16 @@ static u32 get_swif_idx (vat_main_t *vam, const char *ifname) hash_pair_t *p; u8 *name; u32 value; + u32 found = (u32) -1; + INTF_TABLE_LOCK(); hash_foreach_pair (p, vam->sw_if_index_by_interface_name, ({ name = (u8 *) (p->key); value = (u32) p->value[0]; - if (strcmp((char *) name, ifname) == 0) return value; + if (strcmp((char *) name, ifname) == 0) { found = value; } })); - return ((u32) -1); + INTF_TABLE_UNLOCK(); + return found; } static const char * get_swif_name (vat_main_t *vam, const u32 swif_idx) @@ -2049,13 +2098,16 @@ static const char * get_swif_name (vat_main_t *vam, const u32 swif_idx) hash_pair_t *p; u8 *name; u32 value; + const char *found = NULL; + INTF_TABLE_LOCK(); hash_foreach_pair (p, vam->sw_if_index_by_interface_name, ({ name = (u8 *) (p->key); value = (u32) p->value[0]; - if (value == swif_idx) return (const char * )name; + if (value == swif_idx) { found = (const char *) name; } })); - return NULL; + INTF_TABLE_UNLOCK(); + return found; } static int config_lcp_hostif (vat_main_t *vam, @@ -2224,9 +2276,11 @@ int init_vpp_client() vpp_base_vpe_init(); vam->socket_name = format (0, "%s%c", API_SOCKET_FILE, 0); + INTF_TABLE_LOCK(); vam->sw_if_index_by_interface_name = hash_create_string (0, sizeof (uword)); interface_name_by_sw_index = hash_create (0, sizeof (uword)); link_speed_by_sw_index = hash_create (0, sizeof (uword)); + INTF_TABLE_UNLOCK(); if (vsc_socket_connect(vam, client_name) == 0) { int rc; @@ -3406,13 +3460,16 @@ int vpp_get_interface_speed (const char *hwif_name, uint32_t *speed) return -EINVAL; } + INTF_TABLE_LOCK(); p = hash_get(link_speed_by_sw_index, idx); if (!p) { + INTF_TABLE_UNLOCK(); VPP_UNLOCK(); return -ENOENT; } *speed = (uint32_t) p[0]; + INTF_TABLE_UNLOCK(); if (*speed == 0 || *speed == UINT32_MAX) { VPP_UNLOCK(); @@ -5023,9 +5080,11 @@ int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, // Iterates all known sw intfs to collect addresses. u32 *sw_if_idxs = NULL; hash_pair_t *p; + INTF_TABLE_LOCK(); hash_foreach_pair(p, interface_name_by_sw_index, ({ vec_add1(sw_if_idxs, (u32) p->key); })); + INTF_TABLE_UNLOCK(); for (unsigned int i = 0; i < vec_len(sw_if_idxs); i++) { /* Pre-filter: skip interfaces not in the target VRF */ From f92ee3647a345cb25cc4da41c7c9ab3ac099bec9 Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Thu, 9 Jul 2026 07:49:26 -0700 Subject: [PATCH 6/7] fix: resync VPP port oper-status on event-socket reconnect instead of periodic polling Signed-off-by: Nicholas Ching --- vslib/vpp/SwitchVpp.cpp | 6 +++++ vslib/vpp/SwitchVpp.h | 11 +++++++++ vslib/vpp/SwitchVppRif.cpp | 32 +++++++++++++++++++++++++ vslib/vpp/vppxlate/SaiVppXlate.c | 41 ++++++++++++++++++++++++-------- 4 files changed, 80 insertions(+), 10 deletions(-) diff --git a/vslib/vpp/SwitchVpp.cpp b/vslib/vpp/SwitchVpp.cpp index 890c6e8b80..ffa3caa22f 100644 --- a/vslib/vpp/SwitchVpp.cpp +++ b/vslib/vpp/SwitchVpp.cpp @@ -1303,6 +1303,8 @@ sai_status_t SwitchVpp::create( { SWSS_LOG_ENTER(); + serviceDeferredOperStatusResync(); + if (object_type == SAI_OBJECT_TYPE_DEBUG_COUNTER) { sai_object_id_t object_id; @@ -1658,6 +1660,8 @@ sai_status_t SwitchVpp::remove( { SWSS_LOG_ENTER(); + serviceDeferredOperStatusResync(); + if (object_type == SAI_OBJECT_TYPE_DEBUG_COUNTER) { sai_object_id_t objectId; @@ -2017,6 +2021,8 @@ sai_status_t SwitchVpp::set( { SWSS_LOG_ENTER(); + serviceDeferredOperStatusResync(); + if (objectType == SAI_OBJECT_TYPE_PORT) { sai_object_id_t objectId; diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index bf8758b9b2..320642ec6d 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -1157,6 +1158,15 @@ namespace saivs void resyncPortOperStatus(); + // Run a deferred oper-status resync on the command thread if the + // event thread has requested one. resyncPortOperStatus() issues VPP + // binary-API calls (interface_get_state) which allocate on VPP's + // non-thread-safe clib heap; running them on the background event + // thread races the command thread's clib allocations and crashes + // (os_panic in clib_mem_heap_realloc_aligned). So the event thread + // only flags that a resync is due and the command thread performs it. + void serviceDeferredOperStatusResync(); + void startVppEventsThread(); private: // VPP @@ -1165,6 +1175,7 @@ namespace saivs std::map m_hwif_hostif_map; int mapping_init = 0; bool m_run_vpp_events_thread = true; + std::atomic m_operResyncDue { false }; bool VppEventsThreadStarted = false; std::shared_ptr m_vpp_thread; diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 942bdfeca8..d3bfdc2ed6 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -437,6 +437,11 @@ void SwitchVpp::vppProcessEvents () vpp_event_info_t *evp; int ret; + // One-shot level oper-status resync at startup: recover the current link + // state of any interface whose state was set before we subscribed to + // interface events (edge-only delivery would otherwise miss it). + m_operResyncDue.store(true, std::memory_order_relaxed); + while(m_run_vpp_events_thread) { nanosleep(&req, NULL); ret = vpp_sync_for_events(); @@ -444,6 +449,13 @@ void SwitchVpp::vppProcessEvents () if (ret < 0) { SWSS_LOG_WARN("vpp_sync_for_events failed (%d); event socket reconnect attempted", ret); + // The read failure triggers an event-socket reconnect inside + // vpp_sync_for_events. VPP only re-delivers *future* edges after + // re-subscribe, so schedule a one-shot level resync to recover any + // edge missed during the outage (e.g. reboot/config-reload) - the + // exact case resyncPortOperStatus was added for. This is event-driven + // (not a periodic poll), so it does not exhaust the VPP client clib heap. + m_operResyncDue.store(true, std::memory_order_relaxed); } while ((evp = vpp_ev_dequeue())) { if (evp->type == VPP_INTF_LINK_STATUS) { @@ -462,6 +474,26 @@ void SwitchVpp::vppProcessEvents () } vpp_ev_free(evp); } + // No periodic resync here. resyncPortOperStatus() issues per-interface + // SW_INTERFACE_DUMPs; doing that every cycle exhausted VPP's client clib + // heap (os_panic in clib_mem_heap_realloc_aligned). Resync is instead + // requested only at startup and after a reconnect (above), and executed + // on the command thread via serviceDeferredOperStatusResync(). + } +} + +void SwitchVpp::serviceDeferredOperStatusResync() +{ + SWSS_LOG_ENTER(); + + // Runs on the command thread (from the create/set/remove entry points). + // resyncPortOperStatus() issues VPP binary-API SW_INTERFACE_DUMPs; the event + // thread only *requests* a resync (m_operResyncDue) on startup and after an + // event-socket reconnect - never on a periodic timer - so this performs at + // most a handful of level reads over the switch lifetime, avoiding the VPP + // client clib-heap exhaustion that a continuous poll caused. + if (m_operResyncDue.exchange(false, std::memory_order_relaxed)) + { resyncPortOperStatus(); } } diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index ee3625d5ee..26108393c8 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -580,7 +580,19 @@ static pthread_mutex_t vpp_mutex; void vpp_mutex_lock_init () { - pthread_mutex_init(&vpp_mutex, NULL); + /* + * Recursive so the command path can nest EVENT_LOCK inside VPP_LOCK (e.g. + * event registration/reconnect issued while holding VPP_LOCK). EVENT_LOCK + * now maps onto this same mutex (see EVENT_LOCK below): the command path and + * the background event thread both allocate on VPP's process-global, + * non-thread-safe clib heap, so they must be mutually exclusive to avoid + * heap corruption (os_panic in clib_mem_heap_realloc_aligned). + */ + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&vpp_mutex, &attr); + pthread_mutexattr_destroy(&attr); } void vpp_mutex_lock () @@ -597,13 +609,11 @@ void vpp_mutex_unlock () #define VPP_UNLOCK() vpp_mutex_unlock() /* - * Dedicated lock for the asynchronous event connection (vat_event_main / - * event_socket_client_main). Kept separate from vpp_mutex so the background - * event-polling thread never serializes against the synchronous command path. - * - * Lock ordering: code running under EVENT_LOCK must never acquire VPP_LOCK. - * The command path holds VPP_LOCK and may wait on the event connection; - * acquiring VPP_LOCK while holding EVENT_LOCK would deadlock. + * Legacy dedicated event-connection mutex. No longer used for locking: EVENT_LOCK + * now maps onto the recursive vpp_mutex (see EVENT_LOCK below) so that command-path + * and event-thread clib allocations are mutually exclusive on VPP's shared, + * non-thread-safe clib heap. Retained only to keep the init/teardown surface + * unchanged; kept unused otherwise. */ static pthread_mutex_t vpp_event_mutex; @@ -622,8 +632,19 @@ void vpp_event_mutex_unlock () pthread_mutex_unlock(&vpp_event_mutex); } -#define EVENT_LOCK() vpp_event_mutex_lock() -#define EVENT_UNLOCK() vpp_event_mutex_unlock() +/* + * EVENT_LOCK maps onto the same recursive vpp_mutex as VPP_LOCK. The event + * connection (vat_event_main) and the command connection (vat_main) both + * allocate on VPP's process-global, non-thread-safe clib heap; a background + * event-thread clib allocation racing a command-thread clib allocation corrupts + * the heap and crashes (os_panic in clib_mem_heap_realloc_aligned). Serializing + * every VPP client/clib operation under one recursive mutex prevents that. The + * mutex is recursive so the command path may still nest EVENT_LOCK inside + * VPP_LOCK (event registration/reconnect). No command operation blocks waiting + * on an unsolicited event, so this single lock cannot deadlock. + */ +#define EVENT_LOCK() vpp_mutex_lock() +#define EVENT_UNLOCK() vpp_mutex_unlock() /* * Leaf lock protecting the shared interface lookup tables From 43895242d6acaf96fee2a82b5dce6f8db39caec0 Mon Sep 17 00:00:00 2001 From: Nicholas Ching Date: Wed, 15 Jul 2026 14:44:56 -0700 Subject: [PATCH 7/7] fix: resolve WR macro declaration after master update Signed-off-by: Nicholas Ching --- vslib/vpp/vppxlate/SaiVppXlate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 26108393c8..b8e235eedb 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -418,13 +418,14 @@ do { \ f64 hard_deadline = start_time + 10.0; \ f64 idle_deadline = start_time + 1.0; \ socket_client_main_t *scm = vam->socket_client_main; \ + int _wr_rv; \ ret = -99; \ while (1) { \ f64 now = vat_time_now (vam); \ if (now >= hard_deadline || now >= idle_deadline) \ break; \ if (scm && scm->socket_enable) { \ - int _wr_rv = vl_socket_client_read (1); \ + _wr_rv = vl_socket_client_read (1); \ if (_wr_rv == 0) { \ idle_deadline = vat_time_now (vam) + 1.0; \ if (idle_deadline > hard_deadline) \