The *_hdl() functions are also good candidates for removal. :-)
The timer destruction utilizes multiple mechanisms for synchronization:
a flag called "tmr_destroying", and a selection object called
tmr_destroy_syn_obj. Presumably, these mechanisms are enough for
synchronizing the destruction of the timer data structures.
regards,
Anders Widell
On 01/25/2017 11:56 AM, ramesh betham wrote:
> Hi Anders,
>
> Across OpenSAF code, ncshm_<...>_hdl() functions are serving the
> similar purpose.
>
> The reason for not noticing inconsistency in handling ncslpg_<>
> functions in sysf_tmr can be due to
>
> 1. Threads that are using timer calls are gracefully exited before
> the main thread exits (or)
> 2. The protection taken by applications through ncshm_<>_hdl()
> functions had made threads to exit in graceful manner. (or)
> 3. Main thread exiting and subsequently the respective process
> exiting might have hided the situation of thread status that is
> still live and processing sysf_tmr calls.
>
> Regards,
> Ramesh.
>
>
> On 1/25/2017 2:35 PM, Anders Widell wrote:
>>
>> Let't not become sentimental about old broke code. As noted, this
>> mechanism is currently broken and doesn't do anything useful in its
>> current form. I doubt that it has ever worked during the history of
>> the OpenSAF project. And if it was really needed, it is doubtful why
>> it would only be needed in the timer implementation and not anywhere
>> else.
>>
>> regards,
>>
>> Anders Widell
>>
>>
>> On 01/25/2017 05:22 AM, ramesh betham wrote:
>>> Hi Anders,
>>>
>>> Quick comments.
>>>
>>> The purpose of having ncslpg_<...>() functions in sysf_tmr is to
>>> make sure to go through gracefull shutdown process.
>>> sysfTmrDestroy() is suppose to wait till all threads using
>>> sysfTmr to complete their tasks. Observed that ncslpg_destroy()
>>> should have been called in sysfTmrDestroy() to accomplish the
>>> defined purpose.
>>>
>>> Regards,
>>> Ramesh.
>>>
>>> On 1/24/2017 7:24 PM, Anders Widell wrote:
>>>> src/base/hj_hdl.c | 96
>>>> +-------------------------------------------------
>>>> src/base/ncs_hdl_pub.h | 22 +----------
>>>> src/base/sysf_tmr.c | 34 ++---------------
>>>> 3 files changed, 6 insertions(+), 146 deletions(-)
>>>>
>>>>
>>>> Remove the "local persistence guard" API since it is only referenced by the
>>>> OpenSAF timer implementation, and in there it servces no purpose since the
>>>> function ncslpg_destroy() is never called.
>>>>
>>>> diff --git a/src/base/hj_hdl.c b/src/base/hj_hdl.c
>>>> --- a/src/base/hj_hdl.c
>>>> +++ b/src/base/hj_hdl.c
>>>> @@ -1,6 +1,7 @@
>>>> /* -*- OpenSAF -*-
>>>> *
>>>> * (C) Copyright 2008 The OpenSAF Foundation
>>>> + * Copyright Ericsson AB 2012, 2017 - All Rights Reserved.
>>>> *
>>>> * This program is distributed in the hope that it will be useful, but
>>>> * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY
>>>> @@ -25,15 +26,6 @@
>>>> See ncs_hdl.h for brief write-up of issues/capabilities. See the
>>>> ncshm_ calls.
>>>>
>>>> - This file also contains an implementation for a 'Local Persistence
>>>> Guard',
>>>> - which is a cheap (in CPU cycles) persistence guard scheme (for read
>>>> - only object access, like the handle manager) that can be used when an
>>>> object
>>>> - that is known to exist needs a guard for some object that hangs off of
>>>> this
>>>> - object. For example, the LEAP sysfpool has a global struct that always
>>>> exists.
>>>> - What we need to know and be assured of is that when we access memory,
>>>> that
>>>> - the structures managed will persist (destroy() could be called after
>>>> all)
>>>> - while we are in the middle of an operation. See the ncslpg_ calls.
>>>> -
>>>>
>>>> ******************************************************************************
>>>> */
>>>>
>>>> @@ -718,89 +710,3 @@ void hm_unblock_him(HM_CELL *cell)
>>>> int rc = sem_post((sem_t*)cell->data); /* unblock that destroy thread
>>>> */
>>>> osafassert(rc == 0);
>>>> }
>>>> -
>>>> -/***************************************************************************
>>>> - *
>>>> - *
>>>> - *
>>>> - * P u b l i c L o c a l P e r s i s t e n c e G u a r d
>>>> - *
>>>> - * A P I s (prefix 'ncslpg_')
>>>> - *
>>>> - *
>>>> - *
>>>> -
>>>> ***************************************************************************/
>>>> -
>>>> -/*****************************************************************************
>>>> -
>>>> - PROCEDURE NAME: ncslpg_take
>>>> -
>>>> - DESCRIPTION: If all validation stuff is in order return true,
>>>> which
>>>> - means this thread can enter this object.
>>>> -
>>>> -*****************************************************************************/
>>>> -
>>>> -bool ncslpg_take(NCSLPG_OBJ *pg)
>>>> -{
>>>> - m_NCS_OS_ATOMIC_INC(&(pg->inhere)); /* set first, ask later.. to
>>>> beat 'closing' */
>>>> - if (pg->open == true)
>>>> - return true; /* its open, lets go in */
>>>> - else
>>>> - m_NCS_OS_ATOMIC_DEC(&(pg->inhere));
>>>> - return false; /* its closed */
>>>> -}
>>>> -
>>>> -/*****************************************************************************
>>>> -
>>>> - PROCEDURE NAME: ncslpg_give
>>>> -
>>>> - DESCRIPTION: decriment the refcount, as this thread is leaving
>>>> now.
>>>> -
>>>> -*****************************************************************************/
>>>> -
>>>> -uint32_t ncslpg_give(NCSLPG_OBJ *pg, uint32_t ret)
>>>> -{
>>>> - m_NCS_OS_ATOMIC_DEC(&(pg->inhere));
>>>> - return ret;
>>>> -}
>>>> -
>>>> -/*****************************************************************************
>>>> -
>>>> - PROCEDURE NAME: ncslpg_create
>>>> -
>>>> - DESCRIPTION: Put the passed NCSLPG_OBJ in start state. If its
>>>> already
>>>> - in start state, return FAILURE.
>>>> -
>>>> -*****************************************************************************/
>>>> -
>>>> -uint32_t ncslpg_create(NCSLPG_OBJ *pg)
>>>> -{
>>>> - if (pg->open == true)
>>>> - m_LEAP_DBG_SINK_VOID;
>>>> - pg->open = true;
>>>> - pg->inhere = 0;
>>>> - return NCSCC_RC_SUCCESS;
>>>> -}
>>>> -
>>>> -/*****************************************************************************
>>>> -
>>>> - PROCEDURE NAME: ncslpg_destroy
>>>> -
>>>> - DESCRIPTION: Close this LPG. Wait for all other threads to leave
>>>> before
>>>> - returning to the invoker, allowing her to proceed.
>>>> Note
>>>> - that if this object is already closed, this function
>>>> - returns false (invoker should not proceed, as the
>>>> object is
>>>> - already destroyed or being destoyed.
>>>> -
>>>> -*****************************************************************************/
>>>> -
>>>> -bool ncslpg_destroy(NCSLPG_OBJ *pg)
>>>> -{
>>>> - if (pg->open == false)
>>>> - return false; /* already closed */
>>>> - pg->open = false; /* stop others from entering */
>>>> - while (pg->inhere != 0) /* Anybody inhere?? */
>>>> - m_NCS_TASK_SLEEP(1); /* OK, I'll wait; could do semaphore I
>>>> suppose */
>>>> -
>>>> - return true; /* Invoker can proceed to get rid of protected
>>>> thing */
>>>> -}
>>>> diff --git a/src/base/ncs_hdl_pub.h b/src/base/ncs_hdl_pub.h
>>>> --- a/src/base/ncs_hdl_pub.h
>>>> +++ b/src/base/ncs_hdl_pub.h
>>>> @@ -1,6 +1,7 @@
>>>> /* -*- OpenSAF -*-
>>>> *
>>>> * (C) Copyright 2008 The OpenSAF Foundation
>>>> + * Copyright Ericsson AB 2016, 2017 - All Rights Reserved.
>>>> *
>>>> * This program is distributed in the hope that it will be useful, but
>>>> * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY
>>>> @@ -94,27 +95,6 @@ NCSCONTEXT ncshm_take_hdl(NCS_SERVICE_ID
>>>>
>>>> void ncshm_give_hdl(uint32_t hdl);
>>>>
>>>> -/************************************************************************/
>>>> -/* NCSLPG_OBJ - this structure is embedded in known, persistent thing
>>>> */
>>>> -/************************************************************************/
>>>> -
>>>> -typedef struct ncslpg_obj {
>>>> - bool open; /* Is the object (still) open/available */
>>>> - uint32_t inhere; /* use-count of clients 'inside' object now */
>>>> -
>>>> -} NCSLPG_OBJ; /* Local Persistence Guard */
>>>> -
>>>> -/***************************************************************************
>>>> - *
>>>> - * P u b l i c L o c a l P e r s i s t e n c e G u a r d A P I s
>>>> - *
>>>> -
>>>> ***************************************************************************/
>>>> -
>>>> -bool ncslpg_take(NCSLPG_OBJ *pg);
>>>> -uint32_t ncslpg_give(NCSLPG_OBJ *pg, uint32_t ret);
>>>> -uint32_t ncslpg_create(NCSLPG_OBJ *pg);
>>>> -bool ncslpg_destroy(NCSLPG_OBJ *pg);
>>>> -
>>>> #ifdef __cplusplus
>>>> }
>>>> #endif
>>>> diff --git a/src/base/sysf_tmr.c b/src/base/sysf_tmr.c
>>>> --- a/src/base/sysf_tmr.c
>>>> +++ b/src/base/sysf_tmr.c
>>>> @@ -1,6 +1,7 @@
>>>> /* -*- OpenSAF -*-
>>>> *
>>>> * (C) Copyright 2008 The OpenSAF Foundation
>>>> + * Copyright Ericsson AB 2009, 2017 - All Rights Reserved.
>>>> *
>>>> * This program is distributed in the hope that it will be useful, but
>>>> * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY
>>>> @@ -160,7 +161,6 @@ typedef struct tmr_safe {
>>>> typedef struct sysf_tmr_cb {
>>>> uint32_t tick; /* Times TmrExpiry has been called */
>>>>
>>>> - NCSLPG_OBJ persist; /* guard against fleeting destruction */
>>>> TMR_SAFE safe; /* critical region stuff */
>>>> NCS_PATRICIA_TREE tmr_pat_tree;
>>>> TMR_STATS stats;
>>>> @@ -239,17 +239,10 @@ static bool sysfTmrExpiry(SYSF_TMR_PAT_N
>>>> /* get these guys one behind to start as well */
>>>> dead_tmr->next = NULL;
>>>>
>>>> - /* Confirm and secure tmr service is/will persist */
>>>> - if (ncslpg_take(&gl_tcb.persist) == false)
>>>> - return false; /* going or gone away.. Lets leave */
>>>> -
>>>> if (tmr_destroying == true) {
>>>> /* Raise An indication */
>>>> m_NCS_SEL_OBJ_IND(&tmr_destroy_syn_obj);
>>>>
>>>> - /*If thread canceled here, It had no effect on timer thread
>>>> destroy */
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> -
>>>> /* returns true if thread is going to be destroyed otherwise
>>>> return false(normal flow) */
>>>> return true;
>>>> }
>>>> @@ -308,7 +301,6 @@ static bool sysfTmrExpiry(SYSF_TMR_PAT_N
>>>> ncs_patricia_tree_del(&gl_tcb.tmr_pat_tree, (NCS_PATRICIA_NODE *)tmp);
>>>> m_NCS_MEM_FREE(tmp, NCS_MEM_REGION_PERSISTENT, NCS_SERVICE_ID_LEAP_TMR,
>>>> 0);
>>>>
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> return false;
>>>> }
>>>>
>>>> @@ -485,9 +477,6 @@ bool sysfTmrCreate(void)
>>>> /* Empty Timer Service control block. */
>>>> memset(&gl_tcb, '\0', sizeof(SYSF_TMR_CB));
>>>>
>>>> - /* put local persistent guard in start state */
>>>> - ncslpg_create(&gl_tcb.persist);
>>>> -
>>>> /* Initialize the locks */
>>>> m_NCS_LOCK_INIT(&gl_tcb.safe.enter_lock);
>>>> m_NCS_LOCK_INIT(&gl_tcb.safe.free_lock);
>>>> @@ -641,9 +630,6 @@ tmr_t ncs_tmr_alloc(char *file, uint32_t
>>>> if (tmr_destroying == true)
>>>> return NULL;
>>>>
>>>> - if (ncslpg_take(&gl_tcb.persist) == false) /* guarentee
>>>> persistence */
>>>> - return NULL;
>>>> -
>>>> m_NCS_LOCK(&gl_tcb.safe.free_lock, NCS_LOCK_WRITE);
>>>>
>>>> back = &gl_tcb.safe.dmy_free; /* see if we have a free one */
>>>> @@ -681,7 +667,6 @@ tmr_t ncs_tmr_alloc(char *file, uint32_t
>>>> TMR_DBG_SET(tmr->dbg, file, line);
>>>> }
>>>>
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> return (tmr_t)tmr;
>>>> }
>>>>
>>>> @@ -707,13 +692,9 @@ tmr_t ncs_tmr_start(tmr_t tid, int64_t t
>>>>
>>>> TMR_DBG_ASSERT_STATE(tmr, (TMR_STATE_DORMANT | TMR_STATE_CREATE));
>>>>
>>>> - if (ncslpg_take(&gl_tcb.persist) == false) /* guarentee
>>>> persistence */
>>>> - return NULL;
>>>> -
>>>> if (TMR_TEST_STATE(tmr, TMR_STATE_DORMANT)) { /* If client is
>>>> re-using timer */
>>>> m_NCS_TMR_CREATE(new_tmr, tmrDelay, tmrCB, tmrUarg); /* get
>>>> a new one */
>>>> if (new_tmr == NULL) {
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> return NULL;
>>>> }
>>>>
>>>> @@ -738,7 +719,6 @@ tmr_t ncs_tmr_start(tmr_t tid, int64_t t
>>>> if (rc == NCSCC_RC_FAILURE) {
>>>> /* Free the timer created */
>>>> m_NCS_UNLOCK(&gl_tcb.safe.enter_lock, NCS_LOCK_WRITE);
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> return NULL;
>>>> }
>>>> #if ENABLE_SYSLOG_TMR_STATS
>>>> @@ -779,8 +759,6 @@ tmr_t ncs_tmr_start(tmr_t tid, int64_t t
>>>> m_NCS_UNLOCK(&gl_tcb.safe.enter_lock, NCS_LOCK_WRITE);
>>>> TMR_DBG_SET(tmr->dbg, file, line);
>>>>
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> -
>>>> return tmr;
>>>> }
>>>>
>>>> @@ -915,13 +893,9 @@ int64_t ncs_tmr_remaining(tmr_t tmrID, i
>>>> *p_tleft = 0;
>>>> TMR_DBG_ASSERT_ISA(tmr->dbg); /* confirm that its timer memory */
>>>>
>>>> - if (ncslpg_take(&gl_tcb.persist) == false) /* guarentee
>>>> persistence */
>>>> - return NCSCC_RC_FAILURE;
>>>> -
>>>> m_NCS_LOCK(&gl_tcb.safe.enter_lock, NCS_LOCK_WRITE); /* critical
>>>> region START */
>>>> if (!TMR_TEST_STATE(tmr, TMR_STATE_START)) {
>>>> m_NCS_UNLOCK(&gl_tcb.safe.enter_lock, NCS_LOCK_WRITE); /*
>>>> critical region START */
>>>> - ncslpg_give(&gl_tcb.persist, 0);
>>>> return NCSCC_RC_FAILURE;
>>>> }
>>>> m_NCS_UNLOCK(&gl_tcb.safe.enter_lock, NCS_LOCK_WRITE); /* critical
>>>> region START */
>>>> @@ -931,7 +905,7 @@ int64_t ncs_tmr_remaining(tmr_t tmrID, i
>>>>
>>>> *p_tleft = total_ticks_left * NCS_MILLISECONDS_PER_TICK;
>>>>
>>>> - return ncslpg_give(&gl_tcb.persist, NCSCC_RC_SUCCESS);
>>>> + return NCSCC_RC_SUCCESS;
>>>> }
>>>>
>>>>
>>>> /****************************************************************************
>>>> @@ -969,7 +943,7 @@ uint32_t ncs_tmr_whatsout(void)
>>>> printf("| #| | line| | | |\n");
>>>> printf("|---|----+-----+-----------+------------+----------------|\n");
>>>>
>>>> - if ((ncslpg_take(&gl_tcb.persist) == false) || (tmr_destroying ==
>>>> true)) {
>>>> + if (tmr_destroying == true) {
>>>> printf("< . . . TMR SVC DESTROYED: .CLEANUP ALREADY DONE..>\n");
>>>> return NCSCC_RC_FAILURE; /* going or gone away.. Lets
>>>> leave */
>>>> }
>>>> @@ -990,7 +964,7 @@ uint32_t ncs_tmr_whatsout(void)
>>>> free = free->keep;
>>>> }
>>>> m_NCS_UNLOCK(&gl_tcb.safe.free_lock, NCS_LOCK_WRITE); /* critical
>>>> region END */
>>>> - return ncslpg_give(&gl_tcb.persist, NCSCC_RC_SUCCESS);
>>>> + return NCSCC_RC_SUCCESS;
>>>> }
>>>>
>>>>
>>>> /****************************************************************************
>>>
>>
>
------------------------------------------------------------------------------
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
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
