Ack from me. Thanks, Minh
On 3/19/2015 4:45 PM, [email protected] wrote: > osaf/libs/agents/saf/ntfa/ntfa_api.c | 10 ++++------ > 1 files changed, 4 insertions(+), 6 deletions(-) > > > In a multithreaded application,saNtfFinalize() may hang forever > leading to deadlock, if one thread tries to call finalize the handle, > when other thread is trying to send a notification. > > The reason is saNtfNotificationSend() API takes mutex_lock() on ntfa_cb three > times: > -In the first lock/unlock,it retrieves client record. > -In the second lock/unlock, it verifies notification parameters. > -In the third lock/unlock, it sets notificationId that it has got > from ntfs. > In saNtfFinalize() API also after receving response from the NTFS, it > tries to free all records for this client and also it releases allocated > memory for the notifications. > If saNtfFinalize() is called for the same client from one thread for which > Send() is in > progress in another thread, saNtfFinalize() may get lock on ntfa_cb and it > will try to > destroy all the records for this client. But here it will not be able to > destroy > the record as Send() API is still using records in the lock free region. > Thus after taking a lock on ntfa_cb, saNtfFinalize() blocks itself for the > client > record to be released by Send(). Since Send() is in progress, in between it > will also > try to again take lock on ntfa_cb, but it will not get as it already owned by > saNtfFinalize() > which is already blocked because of client resources owned by Send(). > > As a solution, multiple locks should be avoided in the Send() API. Only while > retrieving > client record and notificationHanndle record it should use lock on ntfa_cb. > The reason is saNtfFinalize() will anyway wait for the resoureces taken by > Send() to > be released and thus it will not free the memory. In the worst case, > saNtfFinalize() > will complete in NTFS_WAIT_TIME, but there will not be any deadlock situation. > > diff --git a/osaf/libs/agents/saf/ntfa/ntfa_api.c > b/osaf/libs/agents/saf/ntfa/ntfa_api.c > --- a/osaf/libs/agents/saf/ntfa/ntfa_api.c > +++ b/osaf/libs/agents/saf/ntfa/ntfa_api.c > @@ -1398,23 +1398,25 @@ SaAisErrorT saNtfNotificationSend(SaNtfN > goto done; > } > > + osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0); > notification_hdl_rec = ncshm_take_hdl(NCS_SERVICE_ID_NTFA, > notificationHandle); > if (notification_hdl_rec == NULL) { > + osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > TRACE("ncshm_take_hdl notificationHandle failed"); > rc = SA_AIS_ERR_BAD_HANDLE; > goto err_free; > } > notification_hdl_rec->is_longdn_agent_owner = false; > - osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0); > client_handle = notification_hdl_rec->parent_hdl->local_hdl; > - osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > /* retrieve client hdl rec */ > client_rec = ncshm_take_hdl(NCS_SERVICE_ID_NTFA, client_handle); > if (client_rec == NULL) { > + osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > TRACE("ncshm_take_hdl client_handle failed"); > rc = SA_AIS_ERR_BAD_HANDLE; > goto done_give_hdl; > } > + osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > > /** > ** Populate a sync MDS message > @@ -1427,7 +1429,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN > send_param->notificationType = > notification_hdl_rec->ntfNotificationType; > ntfsv_v_data_cp(&send_param->variable_data, > ¬ification_hdl_rec->variable_data); > > - osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0); > /* Check parameters, depending on type */ > switch (notification_hdl_rec->ntfNotificationType) { > case SA_NTF_TYPE_ALARM: > @@ -1486,7 +1487,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN > goto done_give_hdls; > } > > - osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > /* Send a sync MDS message to obtain a notification id */ > mds_rc = ntfa_mds_msg_sync_send(&ntfa_cb, &msg, &o_msg, timeout); > switch (mds_rc) { > @@ -1506,7 +1506,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN > TRACE("ntfa_mds_msg_sync_send FAILED: %u", rc); > rc = SA_AIS_ERR_TRY_AGAIN; > } > - osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0); > if (mds_rc != NCSCC_RC_SUCCESS) { > goto done_give_hdls; > } > @@ -1556,7 +1555,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN > done_give_hdls: > if (o_msg) > ntfa_msg_destroy(o_msg); > - osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0); > ncshm_give_hdl(client_handle); > done_give_hdl: > ncshm_give_hdl(notificationHandle); > ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
