Hi Praveen, Ack I have used your ntf.c test before and after I applied your fix but could not reproduce the problem in any test. The fix seems to be Ok. It is hard to see if the removed mutex locks will case any data races but I assume you have checked that. I did use helgrind together with ntf.c after applying the fix. I got 786 possible data race detections! but all of them seems to be related to ncs and mds (and are probably false) but could not see anything that I could directly relate to the fix.
Thanks Lennart -----Original Message----- From: [email protected] [mailto:[email protected]] Sent: den 19 mars 2015 06:45 To: Lennart Lund; [email protected]; Minh Chau H Cc: [email protected] Subject: [PATCH 1 of 1] ntfa: fix mutiple mutex lock/unlock on ntfa_cb in saNtfNotificationSend [#1263] 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
