Hi Praveen I think it might be nicer to reduce the 2 else branches (ie. count > si->saAmfSINumCurrActiveAssignments and count < si->saAmfSINumCurrActiveAssignments) to a single else block, with a comment saying adjustments are required.
Thanks Gary -----Original Message----- From: praveen malviya <[email protected]> Organization: Oracle Corporation Date: Tuesday, 21 March 2017 at 4:49 pm To: gary <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]> Cc: <[email protected]> Subject: Re: [PATCH 1 of 1] amfd: remove assignments from lower ranked SU while adjusting SI assignments [#2268] Hi Gary, Thanks for the reviewing the patch. For readability I had kept separate handling for new definition in the if block. Now I will push patch with below refactoring : /* Check if we need to readjust the SI assignments as PrefActiveAssignments got changed */ uint32_t count = mod_pref_assignments; if (mod_pref_assignments == 0) { //Zero is set for using PrefAssignedSus as default arguments. count = si->sg_of_si->pref_assigned_sus(); } if (count == si->saAmfSINumCurrActiveAssignments ) { TRACE("Assignments are equal updating the SI status "); si->saAmfSIPrefActiveAssignments = mod_pref_assignments; } else if (count > si->saAmfSINumCurrActiveAssignments) { si->saAmfSIPrefActiveAssignments = mod_pref_assignments; si->adjust_si_assignments(count); } else if (count < si->saAmfSINumCurrActiveAssignments) { si->adjust_si_assignments(count); si->saAmfSIPrefActiveAssignments = mod_pref_assignments; } TRACE("Modified saAmfSIPrefActiveAssignments is '%u'", si->saAmfSIPrefActiveAssignments); si->update_ass_state(); Thanks, Praveen On 21-Mar-17 7:50 AM, Gary Lee wrote: > Hi Praveen > > Ack (review only + regression tests run) with minor comment below. > > Thanks > Gary > > -----Original Message----- > From: <[email protected]> > Date: Friday, 17 March 2017 at 8:22 pm > To: <[email protected]>, <[email protected]>, gary <[email protected]>, <[email protected]> > Cc: <[email protected]> > Subject: [PATCH 1 of 1] amfd: remove assignments from lower ranked SU while adjusting SI assignments [#2268] > > src/amf/amfd/si.cc | 92 +++++++++++++++++++++++++++--------------------------- > 1 files changed, 46 insertions(+), 46 deletions(-) > > > In N-Way Active mode, when saAmfSIPrefActiveAssignments is reduced, > AMFD removes assignments from higher ranked SU when siranked su is not configured and lower > ranked SU have assignments. > Similar issue in N-Way model when SiPrefStandbyAssignment is reduced. Also AMFD is not > checking HA state of susi and tries to delete active susi and crashes. > > Patch fixes the problem by removing assignments from lower ranked SU. > > diff --git a/src/amf/amfd/si.cc b/src/amf/amfd/si.cc > --- a/src/amf/amfd/si.cc > +++ b/src/amf/amfd/si.cc > @@ -1052,8 +1052,7 @@ done: > */ > void AVD_SI::adjust_si_assignments(const uint32_t mod_pref_assignments) > { > - AVD_SU_SI_REL *sisu, *tmp_sisu; > - uint32_t no_of_sisus_to_delete; > + AVD_SU_SI_REL *sisu; > uint32_t i = 0; > > TRACE_ENTER2("for SI:%s ", name.c_str()); > @@ -1073,31 +1072,24 @@ void AVD_SI::adjust_si_assignments(const > TRACE("No New assignments are been done SI:%s", name.c_str()); > } > } else { > - no_of_sisus_to_delete = saAmfSINumCurrActiveAssignments - > - mod_pref_assignments; > - > - /* Get the sisu pointer from the si->list_of_sisu list from which > - no of sisus need to be deleted based on SI ranked SU */ > - sisu = tmp_sisu = list_of_sisu; > - for( i = 0; i < no_of_sisus_to_delete && nullptr != tmp_sisu; i++ ) { > - tmp_sisu = tmp_sisu->si_next; > + if (list_of_sisu == nullptr) > + return; > + /* > + avd_susi_create() keeps sisus in list_of_sisu in order from highest > + ranked to lowest ranked. > + Keep mod_pref_assignments in list_of_sisu from beginning and delete others. > + */ > + sisu = list_of_sisu; > + for( i = 0; ((i < mod_pref_assignments) && (sisu != nullptr)); i++ ) { > + sisu = sisu->si_next; > } > - while( tmp_sisu && (tmp_sisu->si_next != nullptr) ) { > - sisu = sisu->si_next; > - tmp_sisu = tmp_sisu->si_next; > - } > - > - for( i = 0; i < no_of_sisus_to_delete && (nullptr != sisu); i++ ) { > - /* Send quiesced request for the sisu that needs tobe deleted */ > + for( ; sisu != nullptr; sisu = sisu->si_next) { > if (avd_susi_mod_send(sisu, SA_AMF_HA_QUIESCED) == NCSCC_RC_SUCCESS) { > - /* Add SU to su_opr_list */ > avd_sg_su_oper_list_add(avd_cb, sisu->su, false); > } > - sisu = sisu->si_next; > } > - /* Change the SG FSM to AVD_SG_FSM_SG_REALIGN if assignment is sent.*/ > - if (i > 0) > - sg_of_si->set_fsm_state(AVD_SG_FSM_SG_REALIGN); > + /* Change the SG FSM to AVD_SG_FSM_SG_REALIGN as assignment is sent.*/ > + sg_of_si->set_fsm_state(AVD_SG_FSM_SG_REALIGN); > } > } > if( sg_of_si->sg_redundancy_model == SA_AMF_N_WAY_REDUNDANCY_MODEL ) { > @@ -1107,30 +1099,28 @@ void AVD_SI::adjust_si_assignments(const > LOG_ER("SI new assignmemts failed SI:%s", name.c_str()); > } > } else { > - no_of_sisus_to_delete = 0; > - no_of_sisus_to_delete = saAmfSINumCurrStandbyAssignments - > - mod_pref_assignments; > - > - /* Get the sisu pointer from the si->list_of_sisu list from which > - no of sisus need to be deleted based on SI ranked SU */ > - sisu = tmp_sisu = list_of_sisu; > - for(i = 0; i < no_of_sisus_to_delete && (nullptr != tmp_sisu); i++) { > - tmp_sisu = tmp_sisu->si_next; > + if (list_of_sisu == nullptr) > + return; > + /* > + avd_susi_create() keeps sisus in list_of_sisu in order from highest > + ranked to lowest ranked. > + Keep mod_pref_assignments + active in list_of_sisu from beginning and delete others. > + */ > + for (sisu = list_of_sisu; sisu != nullptr; sisu = sisu->si_next) { > + if (sisu->state == SA_AMF_HA_ACTIVE) > + continue; > + if (i == mod_pref_assignments) > + break; > + i++; > } > - while( tmp_sisu && (tmp_sisu->si_next != nullptr) ) { > - sisu = sisu->si_next; > - tmp_sisu = tmp_sisu->si_next; > + for(; sisu != nullptr; sisu = sisu->si_next) { > + if (sisu->state == SA_AMF_HA_ACTIVE) > + continue; > + if (avd_susi_del_send(sisu) == NCSCC_RC_SUCCESS) { > + avd_sg_su_oper_list_add(avd_cb, sisu->su, false); > + } > } > - > - for( i = 0; i < no_of_sisus_to_delete && (nullptr != sisu); i++ ) { > - /* Delete Standby SI assignment & move it to Realign state */ > - if (avd_susi_del_send(sisu) == NCSCC_RC_SUCCESS) { > - /* Add SU to su_opr_list */ > - avd_sg_su_oper_list_add(avd_cb, sisu->su, false); > - } > - sisu = sisu->si_next; > - } > - /* Change the SG FSM to AVD_SG_FSM_SG_REALIGN */ > + /* Change the SG FSM to AVD_SG_FSM_SG_REALIGN as assignment is sent.*/ > sg_of_si->set_fsm_state(AVD_SG_FSM_SG_REALIGN); > } > } > @@ -1171,10 +1161,20 @@ static void si_ccb_apply_modify_hdlr(Ccb > si->saAmfSIPrefActiveAssignments = mod_pref_assignments; > continue; > } > - > /* Check if we need to readjust the SI assignments as PrefActiveAssignments > got changed */ > - if ( mod_pref_assignments == si->saAmfSINumCurrActiveAssignments ) { > + if (mod_pref_assignments == 0) { > + //Zero is set for using PrefAssignedSus as default arguments. > + uint32_t count = si->sg_of_si->pref_assigned_sus(); > + if (count > si->saAmfSINumCurrActiveAssignments) { > [GL] below statement looks redundant, as it’s done a few lines later anyway. > + si->saAmfSIPrefActiveAssignments = mod_pref_assignments; > + si->adjust_si_assignments(count); > + } else if (count < si->saAmfSINumCurrActiveAssignments) { > + si->adjust_si_assignments(count); > + } else > + TRACE("Assignments are equal updating the SI status "); > + si->saAmfSIPrefActiveAssignments = mod_pref_assignments; > + } else if ( mod_pref_assignments == si->saAmfSINumCurrActiveAssignments ) { > TRACE("Assignments are equal updating the SI status "); > si->saAmfSIPrefActiveAssignments = mod_pref_assignments; > } else if (mod_pref_assignments > si->saAmfSINumCurrActiveAssignments) { > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
