Mailing List Archive: Re: [PATH] isisd threads problems

>
> ------------------------------------------------------------------------
>
> 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]);
>
>

Vincent Jardin wrote:
> > Hasso Tepper <hasso@estpak.ee>
> >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 ?

Fixes? They are fixes to the problem I described that there were
attempts to cancel already unused threads. First two parts are fixes
to the isis_run_dr_l[1|2]() functions. During thread run pointer to
the thread should be set to NULL.

After fixing this I hit similar bug in isis_dr_resign(). This was just
copy/paste error (I think). There was attempt to cancel thread of
wrong level.

I will commit these parts if noone objects.

> 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 !?

This is how threading works in quagga. After canceling thread it's not
freed but flaged as unused and moved to the list of unused threads.
If there is need for new thread thread_get() looks at list of unused
threads. If there is at least one, it will use that. If there is
none, new is created.

> >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.

Ah, yes.

> Moreover, I think that THREAD_READ|WRITE|TIMER_OFF should be used
> instead of THREAD_OFF. It helps to understand the code ;-D

Agree :).

I will go through isisd code and try to fix these. Will post patch to
the list.

--
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator