This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Cyg_Alarm::initialize not thread (DSR) save


"Reinhard JESSICH" <Reinhard.JESSICH@frequentis.com> writes:

> We have two threads in our application which will initialize / disable the same alarm.
> The threads have different priorities.
> Sometimes we had troubles with a corrupted alarm_list in the counter.
> 
> After a long time of testing we found, that the lower priority
> thread calls Cyg_Alarm::initialize and gets interrupted just after
> "if( enabled ) counter->rem_alarm(this)" by the higher priority
> thread. The higher priority thread calls Cyg_Alarm::initialize with
> the same timer and go to sleep. Now the lower priority thread
> continues its work and executes "counter->add_alarm(this)" a second
> time on the same timer. After the last step the list is
> currupted. We use multi list timers, but I think the problem occures
> with single list timers, too.  Because Cyg_DNode::insert can not
> deal with nodes which are allready in a list and are inserted again.

Good catch. The original expectation was that alarms would mostly be
enabled/disabled by a single thread. This has certainly been the case
so far. However, the usage you describe should certainly be allowed.

> 
> We solved the problem with a mutex in our application, but from my
> point of view the kernel should be save. If in the above example a
> DSR is used instead of the higher priority thread the same problem
> will occur and it can't be solved with a mutex. Then the application
> have to lock/unlock the scheduler befor executing
> Cyg_Alarm::initialize. And I don't think it is good practice
> lock/unlock the scheduler from an application.

More to the point, it should be possible for alarm functions to
operate on other alarms. As it happens I have been in the process of
fixing a problem with that, so I'll include your fix with that in a
single large patch.

> 
> My suggestion is to modify "Cyg_Alarm::initialize" according to my
> attached patch (lock/unlock the scheduler).

Actually, the same locking should also be applied to the enable() and
disable() functions too. Which means that add_alarm() and rem_alarm()
don't need to lock the scheduler themselves because they are always
now called from functions which have already locked it. So the patch
needs to be slightly more far-reaching. I'll sort all this out and
check it in.

-- 
Nick Garnett                    eCos Kernel Architect
http://www.ecoscentric.com/     The eCos and RedBoot experts


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]