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
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
