Ack.
Thanks,
Ramesh.
On 1/25/2017 5:19 PM, Anders Widell wrote:
> 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
