>
> ------------------------------------------------------------------------
>
> Subject:
> [PATCH] isisd threads problems
> From:
> Hasso Tepper <hasso@estpak.ee>
> Date:
> Sun, 28 Dec 2003 17:35:05 +0200
> To:
> quagga-dev@lists.quagga.net
>
>
>Attached small patch fixes problems with canceling threads in isisd
>described in [quagga-dev 575]. First three parts of patch are really
>fixes. About last two ones I don't know.
>
Which ones do you mean ?
>> 1) ISISd can't be started with config file it writes. ISISd expects
>> "router isis XXXX" node commands entered before interface node
>> commands - ie. area should be defined before putting interface to the
>> area.
>> 2) ISISd will be very confused when DIS changes. Don't remember
>> details.
>> 3) There is at least one crash related to threads. We solved it with
>> hack in lib/thread.c [don't try to use unused threads]. Of course it
>> isn't correct solution and this needs to be tracked down.
>> 4) Time used in spf doubles with every run if both ipv4 and ipv6 is
>> enabled in interface.
>>
>> interface eth0
>> ip router isis DEAD
>> ipv6 router isis DEAD
>> !
>I'm still not too familiar with quagga threads. What happens if new
>thread is added without canceling previous one? In these cases I hit
>the situation where circuit->u.bc.t_run_dr[idx] existed and I just
>felt uncomfortable about it.
>
>IMHO after thread_cancel(thread); there should be thread=NULL; (like
>THREAD_OFF macro does).
>
Moreover, there is an "unused" flag - THREAD_UNUSED - that is set when a
thread is canceled. Don't you think that it could be used or is it a
private information of thread.c !?
>Before thread=thread_add_timer(); there should be check if(thread ==
>NULL). And if it it isn't and there is need for rechedule thread, old
>thread should be canceled before.
>
THREAD_OFF(x) already check if x is not NULL.
Then, for example,
+ if (circuit->u.bc.t_run_dr[1])
+ THREAD_OFF(circuit->u.bc.t_run_dr[1]);
could be replaced by
THREAD_OFF(circuit->u.bc.t_run_dr[1]);
that is enough.
Moreover, I think that THREAD_READ|WRITE|TIMER_OFF should be used
instead of THREAD_OFF. It helps to understand the code ;-D
>It just helps to avoid problems which might be very hard to debug
>IMHO.
>
agree ;-)
Vincent
>Or am I missing something?
>
>
>
>
>------------------------------------------------------------------------
>
>Index: isis_dr.c
>===================================================================
>RCS file: /var/cvsroot/quagga/isisd/isis_dr.c,v
>retrieving revision 1.1.1.1
>diff -u -3 -p -u -w -b -r1.1.1.1 isis_dr.c
>--- isis_dr.c 23 Dec 2003 08:09:54 -0000 1.1.1.1
>+++ isis_dr.c 28 Dec 2003 12:32:23 -0000
>@@ -80,6 +80,7 @@ isis_run_dr_l1 (struct thread *thread)
> if (circuit->u.bc.run_dr_elect[0])
> zlog_warn ("isis_run_dr(): run_dr_elect already set for l1");
>
>+ circuit->u.bc.t_run_dr[0] = NULL;
> circuit->u.bc.run_dr_elect[0] = 1;
>
> return ISIS_OK;
>@@ -97,6 +98,7 @@ isis_run_dr_l2 (struct thread *thread)
> zlog_warn ("isis_run_dr(): run_dr_elect already set for l2");
>
>
>+ circuit->u.bc.t_run_dr[1] = NULL;
> circuit->u.bc.run_dr_elect[1] = 1;
>
> return ISIS_OK;
>@@ -273,8 +275,8 @@ isis_dr_resign (struct isis_circuit *cir
> } else {
> memset (circuit->u.bc.l2_desig_is, 0, ISIS_SYS_ID_LEN + 1);
>
>- if (circuit->t_send_csnp[0])
>- thread_cancel (circuit->t_send_csnp[0]);
>+ if (circuit->t_send_csnp[1])
>+ thread_cancel (circuit->t_send_csnp[1]);
>
> circuit->u.bc.t_run_dr[1] =
> thread_add_timer (master, isis_run_dr_l2, circuit,
>@@ -328,6 +330,8 @@ isis_dr_commence (struct isis_circuit *c
> thread_cancel (circuit->t_send_l1_psnp); */
> lsp_l1_pseudo_generate (circuit);
>
>+ if (circuit->u.bc.t_run_dr[0])
>+ THREAD_OFF(circuit->u.bc.t_run_dr[0]);
> circuit->u.bc.t_run_dr[0] =
> thread_add_timer (master, isis_run_dr_l1, circuit,
> 2 * circuit->hello_interval[0]);
>@@ -353,6 +357,8 @@ isis_dr_commence (struct isis_circuit *c
> thread_cancel (circuit->t_send_l1_psnp); */
> lsp_l2_pseudo_generate (circuit);
>
>+ if (circuit->u.bc.t_run_dr[1])
>+ THREAD_OFF(circuit->u.bc.t_run_dr[1]);
> circuit->u.bc.t_run_dr[1] =
> thread_add_timer (master, isis_run_dr_l2, circuit,
> 2 * circuit->hello_interval[1]);
>
>
> ------------------------------------------------------------------------
>
> Subject:
> [PATCH] isisd threads problems
> From:
> Hasso Tepper <hasso@estpak.ee>
> Date:
> Sun, 28 Dec 2003 17:35:05 +0200
> To:
> quagga-dev@lists.quagga.net
>
>
>Attached small patch fixes problems with canceling threads in isisd
>described in [quagga-dev 575]. First three parts of patch are really
>fixes. About last two ones I don't know.
>
Which ones do you mean ?
>> 1) ISISd can't be started with config file it writes. ISISd expects
>> "router isis XXXX" node commands entered before interface node
>> commands - ie. area should be defined before putting interface to the
>> area.
>> 2) ISISd will be very confused when DIS changes. Don't remember
>> details.
>> 3) There is at least one crash related to threads. We solved it with
>> hack in lib/thread.c [don't try to use unused threads]. Of course it
>> isn't correct solution and this needs to be tracked down.
>> 4) Time used in spf doubles with every run if both ipv4 and ipv6 is
>> enabled in interface.
>>
>> interface eth0
>> ip router isis DEAD
>> ipv6 router isis DEAD
>> !
>I'm still not too familiar with quagga threads. What happens if new
>thread is added without canceling previous one? In these cases I hit
>the situation where circuit->u.bc.t_run_dr[idx] existed and I just
>felt uncomfortable about it.
>
>IMHO after thread_cancel(thread); there should be thread=NULL; (like
>THREAD_OFF macro does).
>
Moreover, there is an "unused" flag - THREAD_UNUSED - that is set when a
thread is canceled. Don't you think that it could be used or is it a
private information of thread.c !?
>Before thread=thread_add_timer(); there should be check if(thread ==
>NULL). And if it it isn't and there is need for rechedule thread, old
>thread should be canceled before.
>
THREAD_OFF(x) already check if x is not NULL.
Then, for example,
+ if (circuit->u.bc.t_run_dr[1])
+ THREAD_OFF(circuit->u.bc.t_run_dr[1]);
could be replaced by
THREAD_OFF(circuit->u.bc.t_run_dr[1]);
that is enough.
Moreover, I think that THREAD_READ|WRITE|TIMER_OFF should be used
instead of THREAD_OFF. It helps to understand the code ;-D
>It just helps to avoid problems which might be very hard to debug
>IMHO.
>
agree ;-)
Vincent
>Or am I missing something?
>
>
>
>
>------------------------------------------------------------------------
>
>Index: isis_dr.c
>===================================================================
>RCS file: /var/cvsroot/quagga/isisd/isis_dr.c,v
>retrieving revision 1.1.1.1
>diff -u -3 -p -u -w -b -r1.1.1.1 isis_dr.c
>--- isis_dr.c 23 Dec 2003 08:09:54 -0000 1.1.1.1
>+++ isis_dr.c 28 Dec 2003 12:32:23 -0000
>@@ -80,6 +80,7 @@ isis_run_dr_l1 (struct thread *thread)
> if (circuit->u.bc.run_dr_elect[0])
> zlog_warn ("isis_run_dr(): run_dr_elect already set for l1");
>
>+ circuit->u.bc.t_run_dr[0] = NULL;
> circuit->u.bc.run_dr_elect[0] = 1;
>
> return ISIS_OK;
>@@ -97,6 +98,7 @@ isis_run_dr_l2 (struct thread *thread)
> zlog_warn ("isis_run_dr(): run_dr_elect already set for l2");
>
>
>+ circuit->u.bc.t_run_dr[1] = NULL;
> circuit->u.bc.run_dr_elect[1] = 1;
>
> return ISIS_OK;
>@@ -273,8 +275,8 @@ isis_dr_resign (struct isis_circuit *cir
> } else {
> memset (circuit->u.bc.l2_desig_is, 0, ISIS_SYS_ID_LEN + 1);
>
>- if (circuit->t_send_csnp[0])
>- thread_cancel (circuit->t_send_csnp[0]);
>+ if (circuit->t_send_csnp[1])
>+ thread_cancel (circuit->t_send_csnp[1]);
>
> circuit->u.bc.t_run_dr[1] =
> thread_add_timer (master, isis_run_dr_l2, circuit,
>@@ -328,6 +330,8 @@ isis_dr_commence (struct isis_circuit *c
> thread_cancel (circuit->t_send_l1_psnp); */
> lsp_l1_pseudo_generate (circuit);
>
>+ if (circuit->u.bc.t_run_dr[0])
>+ THREAD_OFF(circuit->u.bc.t_run_dr[0]);
> circuit->u.bc.t_run_dr[0] =
> thread_add_timer (master, isis_run_dr_l1, circuit,
> 2 * circuit->hello_interval[0]);
>@@ -353,6 +357,8 @@ isis_dr_commence (struct isis_circuit *c
> thread_cancel (circuit->t_send_l1_psnp); */
> lsp_l2_pseudo_generate (circuit);
>
>+ if (circuit->u.bc.t_run_dr[1])
>+ THREAD_OFF(circuit->u.bc.t_run_dr[1]);
> circuit->u.bc.t_run_dr[1] =
> thread_add_timer (master, isis_run_dr_l2, circuit,
> 2 * circuit->hello_interval[1]);
>
>