Hi Thang, Please check my comments inline [Thien]
Best Regards, Thien ________________________________ From: Thang Nguyen <[email protected]> Sent: Friday, July 3, 2026 4:16 PM To: Thien Huynh <[email protected]>; Dat Phan <[email protected]> Cc: [email protected] <[email protected]>; Thang Nguyen <[email protected]> Subject: [PATCH 1/1] amf: Remove CSIs at the same rank in parallel within an SI [#3401] When removing an SI with many CSIs, the CSI removal was done sequentially one at a time, each waiting for saAmfResponse() before proceeding to the next. This could cause the total removal time to exceed the default systemd TimeoutStopSec (90s) during opensafd stop. Remove all CSIs at the same rank in parallel, similar to how CSI assignment already handles same-rank CSIs via assign_all_csis_at_rank(). Add helper functions: - all_csis_at_rank_removed(): check if all CSIs at a rank are done. - remove_all_csis_at_rank(): issue removal for all CSIs at a rank. - find_highest_rank_to_remove(): find next rank to process. Modify avnd_su_si_remove() to call remove_all_csis_at_rank() and find_highest_rank_to_remove() for starting the initial removal batch, keeping a single source of truth for the rank-based removal logic. Modify avnd_comp_csi_remove_done() to trigger the next rank batch when the current rank batch completes using the same helpers. For the common case where all CSIs have no dependencies (rank=1), this reduces removal time from O(N * callback_time) to O(callback_time). --- src/amf/amfnd/avnd_comp.h | 3 + src/amf/amfnd/comp.cc | 163 ++++++++++++++++++++++++++++++-------- src/amf/amfnd/susm.cc | 19 ++--- 3 files changed, 139 insertions(+), 46 deletions(-) diff --git a/src/amf/amfnd/avnd_comp.h b/src/amf/amfnd/avnd_comp.h index 51bdc855e..941303b82 100644 --- a/src/amf/amfnd/avnd_comp.h +++ b/src/amf/amfnd/avnd_comp.h @@ -921,6 +921,9 @@ extern void avnd_comp_cbq_csi_rec_del(struct avnd_cb_tag *, AVND_COMP *, extern uint32_t avnd_comp_csi_remove(struct avnd_cb_tag *, AVND_COMP *, AVND_COMP_CSI_REC *); +extern uint32_t remove_all_csis_at_rank(struct avnd_cb_tag *cb, + struct avnd_su_si_rec *si, + uint32_t rank, bool target_all); extern uint32_t avnd_comp_csi_assign(struct avnd_cb_tag *, AVND_COMP *, AVND_COMP_CSI_REC *); diff --git a/src/amf/amfnd/comp.cc b/src/amf/amfnd/comp.cc index ad729eb67..6427ff78c 100644 --- a/src/amf/amfnd/comp.cc +++ b/src/amf/amfnd/comp.cc @@ -1764,6 +1764,108 @@ bool all_csis_removable_from_su(const AVND_SU *su) { TRACE_LEAVE2("%u", all_csi_removed); return all_csi_removed; } +/** + * Are all CSIs at the specified rank removed (or unassigned) for this SI? + * @param si + * @param rank + * + * @return bool + */ +static bool all_csis_at_rank_removed(struct avnd_su_si_rec *si, + uint32_t rank) { + AVND_COMP_CSI_REC *csi; + TRACE_ENTER2("'%s' rank=%u", si->name.c_str(), rank); + + for (csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_FIRST(&si->csi_list); + csi != nullptr; + csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_NEXT(&csi->si_dll_node)) { + if (csi->rank != rank) continue; + /* Unqualified components (failed/unregistered) are treated as already + * removed — their CSIs will not receive a removal callback. */ + if (!IsCompQualifiedAssignment(csi->comp)) continue; + if (!m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVED(csi) && + !m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_UNASSIGNED(csi)) { + TRACE_LEAVE2("false"); + return false; + } + } + + TRACE_LEAVE2("true"); + return true; +} + +/** + * Remove all CSIs at the specified rank for the SI in parallel. + * @param cb + * @param si + * @param rank + * @param target_all true when removing with TARGET_ALL (pass nullptr to + * avnd_comp_csi_remove), false for specific CSI removal. + * + * @return uint32_t NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE + */ +uint32_t remove_all_csis_at_rank(AVND_CB *cb, struct avnd_su_si_rec *si, + uint32_t rank, bool target_all) { + AVND_COMP_CSI_REC *csi, *next_csi; + uint32_t rc = NCSCC_RC_SUCCESS; + + TRACE_ENTER2("'%s' rank=%u", si->name.c_str(), rank); + + /* Save next_csi before calling avnd_comp_csi_remove() because a synchronous + * remove_done path (unqualified comp, unassigned PI CSI, non-instantiated NPI + * comp) can cascade all the way to avnd_su_si_rec_del(), which frees the SI + * and every CSI in it. Dereferencing csi->si_dll_node after that call is a + * use-after-free. */ [Thien] When avnd_comp_csi_remove() triggers a synchronous cascade (unqualified comp, unassigned PI CSI, or non-instantiated NPI comp), the entire SI and all its CSIs can be freed within the same call. After this returns, next_csi points to freed memory. The loop assignment csi = next_csi is a use-after-free. Proposal detects SI freed. AVND_SU *su = si->su; for (...) { uint32_t si_count_before = su->si_list.n_nodes; avnd_comp_csi_remove(cb, csi->comp, ...); if (NCSCC_RC_SUCCESS != rc) break; if (su->si_list.n_nodes < si_count_before) break; // SI freed, stop } The same for avnd_comp_csi_remove_done (if-csi branch) uint32_t si_count_before = comp->su->si_list.n_nodes; rc = remove_all_csis_at_rank(cb, csi->si, next_rank, target_all); if (comp->su->si_list.n_nodes < si_count_before) goto done; // SI freed, skip + for (csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_FIRST(&si->csi_list); + csi != nullptr; csi = next_csi) { + next_csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_NEXT(&csi->si_dll_node); + if (csi->rank != rank) continue; + if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVED(csi) || + m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVING(csi)) + continue; + if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_ASSIGNING(csi)) { + LOG_WA("'%s' is getting assigned, remove it after assignment", + csi->name.c_str()); + csi->pending_removal = true; + continue; + } + if (target_all) { + if (m_AVND_COMP_IS_ALL_CSI(csi->comp)) continue; + rc = avnd_comp_csi_remove(cb, csi->comp, 0); + } else { + rc = avnd_comp_csi_remove(cb, csi->comp, csi); + } + if (NCSCC_RC_SUCCESS != rc) break; + } + + TRACE_LEAVE2("%u", rc); + return rc; +} + +/** + * Find the highest rank among CSIs that still need removal in this SI. + * Returns 0 if no CSI needs removal. + * @param si + * + * @return uint32_t highest rank needing removal, or 0 + */ +static uint32_t find_highest_rank_to_remove(struct avnd_su_si_rec *si) { + AVND_COMP_CSI_REC *csi; + uint32_t highest = 0; + + for (csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_FIRST(&si->csi_list); + csi != nullptr; + csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_NEXT(&csi->si_dll_node)) { + if (!m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVED(csi) && + !m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVING(csi) && + !m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_UNASSIGNED(csi)) { + if (csi->rank > highest) highest = csi->rank; + } + } + + return highest; +} + /**************************************************************************** Name : avnd_comp_csi_remove_done @@ -1788,6 +1890,7 @@ uint32_t avnd_comp_csi_remove_done(AVND_CB *cb, AVND_COMP *comp, AVND_COMP_CSI_REC *curr_csi = 0; uint32_t rc = NCSCC_RC_SUCCESS; const std::string csiname = csi ? csi->name : "all CSIs"; + bool target_all = csi ? false : true; TRACE_ENTER2("'%s' '%s'", comp->name.c_str(), csi ? csi->name.c_str() : nullptr); @@ -1839,23 +1942,16 @@ uint32_t avnd_comp_csi_remove_done(AVND_CB *cb, AVND_COMP *comp, avnd_comp_cbq_csi_rec_del(cb, comp, csi->name); } - for (curr_csi = - (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_LAST(&csi->si->csi_list); - curr_csi; curr_csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_PREV( - &curr_csi->si_dll_node)) { - if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVED(curr_csi) || - m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_UNASSIGNED(curr_csi)) - continue; - else if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVING(curr_csi)) - break; - else if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_ASSIGNING(curr_csi)) { - LOG_WA("'%s' is getting assigned, remove it after assignment", - curr_csi->name.c_str()); - curr_csi->pending_removal = true; - break; - } else { - rc = avnd_comp_csi_remove(cb, curr_csi->comp, curr_csi); - break; + /* Check if all CSIs at the current rank are removed. If so, proceed + * to remove all CSIs at the next lower rank in parallel. + * Also guard against pending_removal CSIs at this rank that are still + * in ASSIGNING state — they will trigger their own removal via + * avnd_comp_csi_assign_done() once assignment completes. */ + if (all_csis_at_rank_removed(csi->si, csi->rank) && + !csi->pending_removal) { + uint32_t next_rank = find_highest_rank_to_remove(csi->si); + if (next_rank > 0) { + rc = remove_all_csis_at_rank(cb, csi->si, next_rank, target_all); } } @@ -1867,30 +1963,27 @@ uint32_t avnd_comp_csi_remove_done(AVND_CB *cb, AVND_COMP *comp, } } else { avnd_comp_cbq_csi_rec_del(cb, comp, ""); - /* Issue remove callback with TARGET_ALL for CSIs belonging to prv rank.*/ + /* Check if all components at the current rank are done. + * If so, issue TARGET_ALL for all components at the next lower rank + * in parallel. */ + /* Track which SIs have already been processed in this pass to avoid + * calling remove_all_csis_at_rank() twice for the same (si, rank) pair + * when a component holds multiple CSIs in the same SI at the same rank. */ + avnd_su_si_rec *last_processed_si = nullptr; for (curr_csi = m_AVND_CSI_REC_FROM_COMP_DLL_NODE_GET( m_NCS_DBLIST_FIND_FIRST(&comp->csi_list)); curr_csi; curr_csi = m_AVND_CSI_REC_FROM_COMP_DLL_NODE_GET( m_NCS_DBLIST_FIND_NEXT(&curr_csi->comp_dll_node))) { - for (AVND_COMP_CSI_REC *csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_PREV( - &curr_csi->si_dll_node); - csi; csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_PREV( - &csi->si_dll_node)) { - if (m_AVND_COMP_CSI_CURR_ASSIGN_STATE_IS_REMOVED(csi)) - continue; - else { - /* remove all the csis belonging to this comp */ - rc = avnd_comp_csi_remove(cb, csi->comp, 0); - break; + if (curr_csi->si == last_processed_si) continue; + if (all_csis_at_rank_removed(curr_csi->si, curr_csi->rank)) { + uint32_t next_rank = find_highest_rank_to_remove(curr_csi->si); + if (next_rank > 0) { + last_processed_si = curr_csi->si; + rc = remove_all_csis_at_rank(cb, curr_csi->si, next_rank, target_all); + if (NCSCC_RC_SUCCESS != rc || comp->csi_list.n_nodes == 0) + goto done; } } - - /* When AMFND responds to AMFD for removal of assignments for the SIs in - any SU, it also deletes all the SUSI and COMPCSI records. In such a - case component will not have any CSI in its csi_list. If removal is - completed break the loop. - */ - if (comp->csi_list.n_nodes == 0) break; } /* This is removal with TARGET_ALL. So if all CSIs in all SIs of SU are diff --git a/src/amf/amfnd/susm.cc b/src/amf/amfnd/susm.cc index 066980ad3..b2aafa4fd 100644 --- a/src/amf/amfnd/susm.cc +++ b/src/amf/amfnd/susm.cc @@ -835,20 +835,18 @@ uint32_t avnd_su_si_remove(AVND_CB *cb, AVND_SU *su, AVND_SU_SI_REC *si) { curr_csi = next_csi; } } else { - /* pick up the last csi */ + /* remove all CSIs at the highest rank in parallel */ curr_csi = (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_LAST(&curr_si->csi_list); - - /* remove the csi */ if (curr_csi) { - rc = avnd_comp_csi_remove(cb, curr_csi->comp, (si) ? curr_csi : 0); + rc = remove_all_csis_at_rank(cb, curr_si, curr_csi->rank, false); if (NCSCC_RC_SUCCESS != rc) goto done; } } } else { AVND_SU_SI_REC *next_si; - /* removal for all SIs, do all in paralell */ + /* removal for all SIs, do all in parallel */ for (curr_si = (AVND_SU_SI_REC *)m_NCS_DBLIST_FIND_FIRST(&su->si_list); curr_si;) { /* Save next ptr here since it might get deleted deep down in @@ -856,16 +854,15 @@ uint32_t avnd_su_si_remove(AVND_CB *cb, AVND_SU *su, AVND_SU_SI_REC *si) { next_si = (AVND_SU_SI_REC *)m_NCS_DBLIST_FIND_NEXT(&curr_si->su_dll_node); - /* pick up the last csi */ - curr_csi = - (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_LAST(&curr_si->csi_list); - LOG_NO("Removing '%s' from '%s'", curr_si->name.c_str(), su->name.c_str()); - /* remove all the CSIs from this comp */ + /* Send TARGET_ALL to all components at the highest rank in + * this SI in parallel */ + curr_csi = + (AVND_COMP_CSI_REC *)m_NCS_DBLIST_FIND_LAST(&curr_si->csi_list); if (curr_csi) { - rc = avnd_comp_csi_remove(cb, curr_csi->comp, nullptr); + rc = remove_all_csis_at_rank(cb, curr_si, curr_csi->rank, true); if (NCSCC_RC_SUCCESS != rc || !su->si_list.n_nodes) goto done; } curr_si = next_si; -- 2.34.1 The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection. Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions. _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
