Re: [devel] [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT [#2036]
Hi Gary/Minh,
I think, comments can be taken up as a part of refactoring later.
Ack from me, for both the patches.
Thanks,
Praveen
On 23-Sep-16 5:33 AM, Minh Hon Chau wrote:
> osaf/services/saf/amf/amfd/include/timer.h | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
>
> Since the AVD_TMR_CL_INIT can be started/restarted to wait for all SU
> presence state
> synchronization, so m_AVD_CLINIT_TMR_START could not always be called as the
> first start. As
> a result, due to @is_active is set to false before start timer, therefore the
> timer can be
> restarted without stop in advance. It appears a warning as below
>
> Sep 22 22:08:22.640710 osafamfd [477:timer.cc:0066] TEST >> avd_start_tmr: 1
> Sep 22 22:08:22.640717 osafamfd [477:sysf_tmr.c:0690] TR IN LEAP_DBG_SINK
> Sep 22 22:08:22.640758 osafamfd [477:timer.cc:0096] << avd_start_tmr
>
> Patch removes setting @is_active to false in m_AVD_CLINIT_TMR_START
>
> diff --git a/osaf/services/saf/amf/amfd/include/timer.h
> b/osaf/services/saf/amf/amfd/include/timer.h
> --- a/osaf/services/saf/amf/amfd/include/timer.h
> +++ b/osaf/services/saf/amf/amfd/include/timer.h
> @@ -65,7 +65,6 @@ typedef struct avd_tmr_tag {
> #define m_AVD_CLINIT_TMR_START(cb) \
> {\
> saflog(LOG_NOTICE, amfSvcUsrName, "Starting cluster startup timer"); \
> - cb->amf_init_tmr.is_active = false; \
> cb->amf_init_tmr.type = AVD_TMR_CL_INIT; \
> avd_start_tmr(cb,&(cb->amf_init_tmr),
> avd_cluster->saAmfClusterStartupTimeout); \
> }
>
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT [#2036]
Hi Nagu,
Please let me know what particular thing that concerns you with (1), or
do you agree if I do like (2)?
Thanks,
Minh
On 23/09/16 16:27, minh chau wrote:
> Hi Nagu,
>
> The macro m_AVD_CLINIT_TMR_START has the setting *is_active = false*,
> so if this macro is called again without checking timer status, then
> amf_init_tmr will be started again while the same instance of this
> timer could be running.
> In the fix of #1988, I could not reuse this macro and had to manually
> start/stop this timer. But #1988 is in context of headless feature
> only, so it would not cause any problem because m_AVD_CLINIT_TMR_START
> is always called first.
> Now in #2063 which has roaming feature enabled, m_AVD_CLINIT_TMR_START
> is not always called first. The first instance of amf_init_tmr is
> started in avd_node_up_evh(), because leds state of amfnd in active
> promoted SC is green (true). Then, m_AVD_CLINIT_TMR_START get hit
> after 2N Opensaf SU (in promoted SC) is assigned ACTIVE.
> So, to fix the problem in #2063, I do either (1) or (2):
> (1). As in this patch, remove *is_active = false* in
> m_AVD_CLINIT_TMR_START
> (2). Before call m_AVD_CLINIT_TMR_START, need to stop this timer if
> it's running, likely:
> diff --git a/osaf/services/saf/amf/amfd/ndfsm.cc
> b/osaf/services/saf/amf/amfd/ndfsm.cc
> --- a/osaf/services/saf/amf/amfd/ndfsm.cc
> +++ b/osaf/services/saf/amf/amfd/ndfsm.cc
> @@ -570,6 +570,8 @@ void avd_nd_ncs_su_assigned(AVD_CL_CB *c
> cb->init_state = AVD_INIT_DONE;
>
> /* start the cluster init timer. */
> + if (cb->amf_init_tmr.is_active == true)
> + avd_stop_tmr(cb, &cb->amf_init_tmr);
> m_AVD_CLINIT_TMR_START(cb);
> }
>
> I think (1) is reasonable, the is_active could not be forced to be
> false before starting timer regardless actual status of timer. The
> current *is_active = false* in macro, I think it was meant as an
> initial setting, so it'd better be done somewhere in initialize(void),
> similarly as
> cb->heartbeat_tmr.is_active = false;
>
> Do you agree if I also add *cb->amf_init_tmr.is_active = false* in
> initialize(void)?
>
> Thanks,
> Minh
>
> On 23/09/16 15:46, Nagendra Kumar wrote:
>> Please don't change anything in this function. Please stop and start
>> the timer or write a function if needed.
>>
>> Thanks
>> -Nagu
>>> -Original Message-
>>> From: Minh Hon Chau [mailto:minh.c...@dektech.com.au]
>>> Sent: 23 September 2016 05:33
>>> To: hans.nordeb...@ericsson.com; Nagendra Kumar; Praveen Malviya;
>>> gary@dektech.com.au; long.hb.ngu...@dektech.com.au;
>>> minh.c...@dektech.com.au
>>> Cc: opensaf-devel@lists.sourceforge.net
>>> Subject: [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT
>>> [#2036]
>>>
>>> osaf/services/saf/amf/amfd/include/timer.h | 1 -
>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>>
>>>
>>> Since the AVD_TMR_CL_INIT can be started/restarted to wait for all SU
>>> presence state
>>> synchronization, so m_AVD_CLINIT_TMR_START could not always be called
>>> as the first start. As
>>> a result, due to @is_active is set to false before start timer,
>>> therefore the
>>> timer can be
>>> restarted without stop in advance. It appears a warning as below
>>>
>>> Sep 22 22:08:22.640710 osafamfd [477:timer.cc:0066] TEST >>
>>> avd_start_tmr: 1
>>> Sep 22 22:08:22.640717 osafamfd [477:sysf_tmr.c:0690] TR IN
>>> LEAP_DBG_SINK
>>> Sep 22 22:08:22.640758 osafamfd [477:timer.cc:0096] << avd_start_tmr
>>>
>>> Patch removes setting @is_active to false in m_AVD_CLINIT_TMR_START
>>>
>>> diff --git a/osaf/services/saf/amf/amfd/include/timer.h
>>> b/osaf/services/saf/amf/amfd/include/timer.h
>>> --- a/osaf/services/saf/amf/amfd/include/timer.h
>>> +++ b/osaf/services/saf/amf/amfd/include/timer.h
>>> @@ -65,7 +65,6 @@ typedef struct avd_tmr_tag {
>>> #define m_AVD_CLINIT_TMR_START(cb) \
>>> {\
>>> saflog(LOG_NOTICE, amfSvcUsrName, "Starting cluster startup
>>> timer"); \
>>> -cb->amf_init_tmr.is_active = false; \
>>> cb->amf_init_tmr.type = AVD_TMR_CL_INIT; \
>>> avd_start_tmr(cb,&(cb->amf_init_tmr), avd_cluster-
saAmfClusterStartupTimeout); \
>>> }
>
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT [#2036]
Hi Nagu,
The macro m_AVD_CLINIT_TMR_START has the setting *is_active = false*, so
if this macro is called again without checking timer status, then
amf_init_tmr will be started again while the same instance of this timer
could be running.
In the fix of #1988, I could not reuse this macro and had to manually
start/stop this timer. But #1988 is in context of headless feature only,
so it would not cause any problem because m_AVD_CLINIT_TMR_START is
always called first.
Now in #2063 which has roaming feature enabled, m_AVD_CLINIT_TMR_START
is not always called first. The first instance of amf_init_tmr is
started in avd_node_up_evh(), because leds state of amfnd in active
promoted SC is green (true). Then, m_AVD_CLINIT_TMR_START get hit after
2N Opensaf SU (in promoted SC) is assigned ACTIVE.
So, to fix the problem in #2063, I do either (1) or (2):
(1). As in this patch, remove *is_active = false* in m_AVD_CLINIT_TMR_START
(2). Before call m_AVD_CLINIT_TMR_START, need to stop this timer if it's
running, likely:
diff --git a/osaf/services/saf/amf/amfd/ndfsm.cc
b/osaf/services/saf/amf/amfd/ndfsm.cc
--- a/osaf/services/saf/amf/amfd/ndfsm.cc
+++ b/osaf/services/saf/amf/amfd/ndfsm.cc
@@ -570,6 +570,8 @@ void avd_nd_ncs_su_assigned(AVD_CL_CB *c
cb->init_state = AVD_INIT_DONE;
/* start the cluster init timer. */
+ if (cb->amf_init_tmr.is_active == true)
+ avd_stop_tmr(cb, &cb->amf_init_tmr);
m_AVD_CLINIT_TMR_START(cb);
}
I think (1) is reasonable, the is_active could not be forced to be false
before starting timer regardless actual status of timer. The current
*is_active = false* in macro, I think it was meant as an initial
setting, so it'd better be done somewhere in initialize(void), similarly as
cb->heartbeat_tmr.is_active = false;
Do you agree if I also add *cb->amf_init_tmr.is_active = false* in
initialize(void)?
Thanks,
Minh
On 23/09/16 15:46, Nagendra Kumar wrote:
> Please don't change anything in this function. Please stop and start the
> timer or write a function if needed.
>
> Thanks
> -Nagu
>> -Original Message-
>> From: Minh Hon Chau [mailto:minh.c...@dektech.com.au]
>> Sent: 23 September 2016 05:33
>> To: hans.nordeb...@ericsson.com; Nagendra Kumar; Praveen Malviya;
>> gary@dektech.com.au; long.hb.ngu...@dektech.com.au;
>> minh.c...@dektech.com.au
>> Cc: opensaf-devel@lists.sourceforge.net
>> Subject: [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT
>> [#2036]
>>
>> osaf/services/saf/amf/amfd/include/timer.h | 1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>
>>
>> Since the AVD_TMR_CL_INIT can be started/restarted to wait for all SU
>> presence state
>> synchronization, so m_AVD_CLINIT_TMR_START could not always be called
>> as the first start. As
>> a result, due to @is_active is set to false before start timer, therefore the
>> timer can be
>> restarted without stop in advance. It appears a warning as below
>>
>> Sep 22 22:08:22.640710 osafamfd [477:timer.cc:0066] TEST >>
>> avd_start_tmr: 1
>> Sep 22 22:08:22.640717 osafamfd [477:sysf_tmr.c:0690] TR IN
>> LEAP_DBG_SINK
>> Sep 22 22:08:22.640758 osafamfd [477:timer.cc:0096] << avd_start_tmr
>>
>> Patch removes setting @is_active to false in m_AVD_CLINIT_TMR_START
>>
>> diff --git a/osaf/services/saf/amf/amfd/include/timer.h
>> b/osaf/services/saf/amf/amfd/include/timer.h
>> --- a/osaf/services/saf/amf/amfd/include/timer.h
>> +++ b/osaf/services/saf/amf/amfd/include/timer.h
>> @@ -65,7 +65,6 @@ typedef struct avd_tmr_tag {
>> #define m_AVD_CLINIT_TMR_START(cb) \
>> {\
>> saflog(LOG_NOTICE, amfSvcUsrName, "Starting cluster startup
>> timer"); \
>> -cb->amf_init_tmr.is_active = false; \
>> cb->amf_init_tmr.type = AVD_TMR_CL_INIT; \
>> avd_start_tmr(cb,&(cb->amf_init_tmr), avd_cluster-
>>> saAmfClusterStartupTimeout); \
>> }
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT [#2036]
Please don't change anything in this function. Please stop and start the timer
or write a function if needed.
Thanks
-Nagu
> -Original Message-
> From: Minh Hon Chau [mailto:minh.c...@dektech.com.au]
> Sent: 23 September 2016 05:33
> To: hans.nordeb...@ericsson.com; Nagendra Kumar; Praveen Malviya;
> gary@dektech.com.au; long.hb.ngu...@dektech.com.au;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT
> [#2036]
>
> osaf/services/saf/amf/amfd/include/timer.h | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
>
> Since the AVD_TMR_CL_INIT can be started/restarted to wait for all SU
> presence state
> synchronization, so m_AVD_CLINIT_TMR_START could not always be called
> as the first start. As
> a result, due to @is_active is set to false before start timer, therefore the
> timer can be
> restarted without stop in advance. It appears a warning as below
>
> Sep 22 22:08:22.640710 osafamfd [477:timer.cc:0066] TEST >>
> avd_start_tmr: 1
> Sep 22 22:08:22.640717 osafamfd [477:sysf_tmr.c:0690] TR IN
> LEAP_DBG_SINK
> Sep 22 22:08:22.640758 osafamfd [477:timer.cc:0096] << avd_start_tmr
>
> Patch removes setting @is_active to false in m_AVD_CLINIT_TMR_START
>
> diff --git a/osaf/services/saf/amf/amfd/include/timer.h
> b/osaf/services/saf/amf/amfd/include/timer.h
> --- a/osaf/services/saf/amf/amfd/include/timer.h
> +++ b/osaf/services/saf/amf/amfd/include/timer.h
> @@ -65,7 +65,6 @@ typedef struct avd_tmr_tag {
> #define m_AVD_CLINIT_TMR_START(cb) \
> {\
> saflog(LOG_NOTICE, amfSvcUsrName, "Starting cluster startup
> timer"); \
> - cb->amf_init_tmr.is_active = false; \
> cb->amf_init_tmr.type = AVD_TMR_CL_INIT; \
> avd_start_tmr(cb,&(cb->amf_init_tmr), avd_cluster-
> >saAmfClusterStartupTimeout); \
> }
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
[devel] [PATCH 2 of 2] AMFD: Fix double start timer of AVD_TMR_CL_INIT [#2036]
osaf/services/saf/amf/amfd/include/timer.h | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
Since the AVD_TMR_CL_INIT can be started/restarted to wait for all SU presence
state
synchronization, so m_AVD_CLINIT_TMR_START could not always be called as the
first start. As
a result, due to @is_active is set to false before start timer, therefore the
timer can be
restarted without stop in advance. It appears a warning as below
Sep 22 22:08:22.640710 osafamfd [477:timer.cc:0066] TEST >> avd_start_tmr: 1
Sep 22 22:08:22.640717 osafamfd [477:sysf_tmr.c:0690] TR IN LEAP_DBG_SINK
Sep 22 22:08:22.640758 osafamfd [477:timer.cc:0096] << avd_start_tmr
Patch removes setting @is_active to false in m_AVD_CLINIT_TMR_START
diff --git a/osaf/services/saf/amf/amfd/include/timer.h
b/osaf/services/saf/amf/amfd/include/timer.h
--- a/osaf/services/saf/amf/amfd/include/timer.h
+++ b/osaf/services/saf/amf/amfd/include/timer.h
@@ -65,7 +65,6 @@ typedef struct avd_tmr_tag {
#define m_AVD_CLINIT_TMR_START(cb) \
{\
saflog(LOG_NOTICE, amfSvcUsrName, "Starting cluster startup timer"); \
- cb->amf_init_tmr.is_active = false; \
cb->amf_init_tmr.type = AVD_TMR_CL_INIT; \
avd_start_tmr(cb,&(cb->amf_init_tmr),
avd_cluster->saAmfClusterStartupTimeout); \
}
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
