Hi Nagu,
a question, it looks like e.g. avnd_comp_clc_cmd_execute() may run in
parallel wrt the main thread
and the new ImmReader, not sure but have you checked for data races in
this function?
/Thanks HansN
On 04/11/2016 01:49 PM, nagendr...@oracle.com wrote:
> osaf/services/saf/amf/amfnd/amfnd.cc | 3 -
> osaf/services/saf/amf/amfnd/comp.cc | 3 -
> osaf/services/saf/amf/amfnd/di.cc | 24 ---------
> osaf/services/saf/amf/amfnd/mds.cc | 89
> ++++++++++++++++++++++++++++++++++++
> osaf/services/saf/amf/amfnd/su.cc | 12 ----
> osaf/services/saf/amf/amfnd/susm.cc | 9 ---
> osaf/services/saf/amf/amfnd/term.cc | 4 -
> 7 files changed, 89 insertions(+), 55 deletions(-)
>
>
> Since, the message id counter is updated by main thread and
> imm reader thread simultaneouspy, so there is chance that
> the counter may be updated in any order and make Amfd and
> Amfnd counters Out of Order.
> So, such msg id counter update should be removed
> from both and should be kep at a common location.
> This patch fixes the same.
>
> diff --git a/osaf/services/saf/amf/amfnd/amfnd.cc
> b/osaf/services/saf/amf/amfnd/amfnd.cc
> --- a/osaf/services/saf/amf/amfnd/amfnd.cc
> +++ b/osaf/services/saf/amf/amfnd/amfnd.cc
> @@ -386,9 +386,6 @@ uint32_t avnd_evt_avd_reboot_evh(AVND_CB
>
> osafassert(AVSV_D2N_REBOOT_MSG == evt->info.avd->msg_type);
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> /* Clear error report related alarms before reboot.
> TODO: This for loop can be removed if AVD remembers and checkpoints
> alarms sent due to error report.
> diff --git a/osaf/services/saf/amf/amfnd/comp.cc
> b/osaf/services/saf/amf/amfnd/comp.cc
> --- a/osaf/services/saf/amf/amfnd/comp.cc
> +++ b/osaf/services/saf/amf/amfnd/comp.cc
> @@ -2691,9 +2691,6 @@ uint32_t avnd_evt_comp_admin_op_req(AVND
>
> TRACE_ENTER2("'%s' op=%u", info->dn.value, info->oper_id);
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> comp = m_AVND_COMPDB_REC_GET(cb->compdb, info->dn);
> osafassert( comp != nullptr);
>
> diff --git a/osaf/services/saf/amf/amfnd/di.cc
> b/osaf/services/saf/amf/amfnd/di.cc
> --- a/osaf/services/saf/amf/amfnd/di.cc
> +++ b/osaf/services/saf/amf/amfnd/di.cc
> @@ -108,9 +108,6 @@ static uint32_t avnd_evt_node_admin_op_r
>
> TRACE_ENTER2("%s op=%u", info->dn.value, info->oper_id);
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> switch(info->oper_id) {
> default:
> LOG_NO("%s: unsupported adm op %u", __FUNCTION__,
> info->oper_id);
> @@ -200,17 +197,6 @@ uint32_t avnd_evt_avd_operation_request_
>
> TRACE_ENTER2("Class=%u, action=%u", param->class_id, param->act);
>
> - /* dont process unless AvD is up */
> - if (!m_AVND_CB_IS_AVD_UP(cb))
> - goto done;
> -
> - // TODO() hide the below code in a "set_msg_id()" function
> - // If message was not broadcasted, (msg_id == 0)
> - if (info->msg_id != 0) {
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> - }
> -
> switch (param->class_id) {
> case AVSV_SA_AMF_NODE:
> rc = avnd_node_oper_req(cb, param);
> @@ -260,7 +246,6 @@ uint32_t avnd_evt_avd_operation_request_
> avnd_msg_content_free(cb, &msg);
> }
>
> -done:
> TRACE_LEAVE();
> return rc;
> }
> @@ -1410,20 +1395,11 @@ uint32_t avnd_evt_avd_role_change_evh(AV
>
> TRACE_ENTER();
>
> - /* dont process unless AvD is up */
> - if (!m_AVND_CB_IS_AVD_UP(cb)){
> - LOG_IN("AVD is not up yet");
> - return NCSCC_RC_FAILURE;
> - }
> -
> info = &evt->info.avd->msg_info.d2n_role_change_info;
>
> TRACE("MsgId: %u,NodeId:%u, role rcvd:%u role present:%u",\
> info->msg_id, info->node_id, info->role,
> cb->avail_state_avnd);
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> prev_ha_state = cb->avail_state_avnd;
>
> /* Ignore the duplicate roles. */
> diff --git a/osaf/services/saf/amf/amfnd/mds.cc
> b/osaf/services/saf/amf/amfnd/mds.cc
> --- a/osaf/services/saf/amf/amfnd/mds.cc
> +++ b/osaf/services/saf/amf/amfnd/mds.cc
> @@ -300,6 +300,93 @@ done:
> return rc;
> }
>
> +/**
> + * @brief This function update the receive msg id with the msg id sent
> + * from Amfd.
> + * @param ptr to avnd_cb.
> + * @param ptr to evt.
> + * @return Nothing.
> + */
> +static void update_rcv_msg_id(AVND_CB *cb, AVND_EVT *evt)
> +{
> + uint32_t msg_id;
> + switch (evt->type) {
> + case AVND_EVT_AVD_OPERATION_REQUEST_MSG:
> + {
> + AVSV_D2N_OPERATION_REQUEST_MSG_INFO *op_info =
> &evt->info.avd->msg_info.d2n_op_req;
> + msg_id = op_info->msg_id;
> + /* dont update the counter unless AND is up. */
> + if (!m_AVND_CB_IS_AVD_UP(cb))
> + goto done;
> +
> + // TODO() hide the below code in a
> "set_msg_id()" function
> + // If message was not broadcasted, (msg_id == 0)
> + if (msg_id == 0)
> + goto done;
> + }
> + break;
> + case AVND_EVT_AVD_ROLE_CHANGE_MSG:
> + {
> + AVSV_D2N_ROLE_CHANGE_INFO *rc_info =
> &evt->info.avd->msg_info.d2n_role_change_info;
> + msg_id = rc_info->msg_id;
> + /* dont update the counter unless AND is up. */
> + if (!m_AVND_CB_IS_AVD_UP(cb))
> + goto done;
> + }
> + break;
> + case AVND_EVT_AVD_SU_PRES_MSG:
> + {
> + AVSV_D2N_PRESENCE_SU_MSG_INFO *pres_info =
> &evt->info.avd->msg_info.d2n_prsc_su;
> + msg_id = pres_info->msg_id;
> + /* dont update the counter unless AND is up. */
> + if (!m_AVND_CB_IS_AVD_UP(cb))
> + return;
> + }
> + break;
> + case AVND_EVT_AVD_REG_SU_MSG:
> + {
> + AVSV_D2N_REG_SU_MSG_INFO *reg_info =
> &evt->info.avd->msg_info.d2n_reg_su;
> + msg_id = reg_info->msg_id;
> + /* dont update the counter unless AND is up. */
> + if (!m_AVND_CB_IS_AVD_UP(cb))
> + return;
> + }
> + break;
> + case AVND_EVT_AVD_INFO_SU_SI_ASSIGN_MSG:
> + {
> + AVSV_D2N_INFO_SU_SI_ASSIGN_MSG_INFO *susi_info
> = &evt->info.avd->msg_info.d2n_su_si_assign;
> + msg_id = susi_info->msg_id;
> + }
> + break;
> + case AVND_EVT_AVD_SET_LEDS_MSG:
> + {
> + AVSV_D2N_SET_LEDS_MSG_INFO *led_info =
> &evt->info.avd->msg_info.d2n_set_leds;
> + msg_id = led_info->msg_id;
> + }
> + break;
> + case AVND_EVT_AVD_ADMIN_OP_REQ_MSG:
> + {
> + AVSV_D2N_ADMIN_OP_REQ_MSG_INFO *adm_info =
> &evt->info.avd->msg_info.d2n_admin_op_req_info;
> + msg_id = adm_info->msg_id;
> + }
> + break;
> + case AVND_EVT_AVD_REBOOT_MSG:
> + {
> + AVSV_D2N_REBOOT_MSG_INFO *reboot_info =
> &evt->info.avd->msg_info.d2n_reboot_info;
> + msg_id = reboot_info->msg_id;
> + break;
> + }
> + default:
> + goto done;
> + }
> +
> +
> + avnd_msgid_assert(msg_id);
> + cb->rcv_msg_id = msg_id;
> +done:
> + return;
> +}
> +
>
> /****************************************************************************
> Name : avnd_mds_rcv
>
> @@ -432,6 +519,8 @@ uint32_t avnd_mds_rcv(AVND_CB *cb, MDS_C
> /* nullify the msg as it is used in the event */
> memset(&msg, 0, sizeof(AVND_MSG));
>
> + /* Update the receive msg id counter. */
> + update_rcv_msg_id(cb, evt);
> /* send the event */
> rc = avnd_evt_send(cb, evt);
>
> diff --git a/osaf/services/saf/amf/amfnd/su.cc
> b/osaf/services/saf/amf/amfnd/su.cc
> --- a/osaf/services/saf/amf/amfnd/su.cc
> +++ b/osaf/services/saf/amf/amfnd/su.cc
> @@ -125,15 +125,8 @@ uint32_t avnd_evt_avd_reg_su_evh(AVND_CB
>
> TRACE_ENTER();
>
> - /* dont process unless AvD is up */
> - if (!m_AVND_CB_IS_AVD_UP(cb))
> - goto done;
> -
> info = &evt->info.avd->msg_info.d2n_reg_su;
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> /*
> * Check whether SU updates are received after fail-over then
> * call a separate processing function.
> @@ -396,8 +389,6 @@ uint32_t avnd_evt_avd_info_su_si_assign_
> }
> }
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
>
> if (info->msg_act == AVSV_SUSI_ACT_ASGN) {
> /* SI rank and CSI capability (originally from SaAmfCtCsType)
> @@ -622,9 +613,6 @@ uint32_t avnd_evt_su_admin_op_req(AVND_C
>
> TRACE_ENTER2("%s op=%u", info->dn.value, info->oper_id);
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> su = m_AVND_SUDB_REC_GET(cb->sudb, info->dn);
> osafassert(su != nullptr);
>
> diff --git a/osaf/services/saf/amf/amfnd/susm.cc
> b/osaf/services/saf/amf/amfnd/susm.cc
> --- a/osaf/services/saf/amf/amfnd/susm.cc
> +++ b/osaf/services/saf/amf/amfnd/susm.cc
> @@ -1277,12 +1277,6 @@ uint32_t avnd_evt_avd_su_pres_evh(AVND_C
>
> TRACE_ENTER();
>
> - /* dont process unless AvD is up */
> - if (!m_AVND_CB_IS_AVD_UP(cb)) {
> - TRACE("AVD is not yet up");
> - goto done;
> - }
> -
> /* since AMFND is going down no need to process SU
> instantiate/terminate msg
> * because SU instantiate will cause component information to be read
> from IMMND
> * and IMMND might have been already terminated and in that case AMFND
> will osafassert */
> @@ -1293,9 +1287,6 @@ uint32_t avnd_evt_avd_su_pres_evh(AVND_C
>
> info = &evt->info.avd->msg_info.d2n_prsc_su;
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> /* get the su */
> su = m_AVND_SUDB_REC_GET(cb->sudb, info->su_name);
> if (!su) {
> diff --git a/osaf/services/saf/amf/amfnd/term.cc
> b/osaf/services/saf/amf/amfnd/term.cc
> --- a/osaf/services/saf/amf/amfnd/term.cc
> +++ b/osaf/services/saf/amf/amfnd/term.cc
> @@ -196,14 +196,10 @@ done:
>
> ******************************************************************************/
> uint32_t avnd_evt_avd_set_leds_evh(AVND_CB *cb, AVND_EVT *evt)
> {
> - AVSV_D2N_SET_LEDS_MSG_INFO *info =
> &evt->info.avd->msg_info.d2n_set_leds;
> uint32_t rc = NCSCC_RC_SUCCESS;
>
> TRACE_ENTER();
>
> - avnd_msgid_assert(info->msg_id);
> - cb->rcv_msg_id = info->msg_id;
> -
> if (cb->led_state == AVND_LED_STATE_GREEN) {
> /* Nothing to be done we have already got this msg */
> goto done;
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
