Re: cyg_alarm - suggestion for enhancement

["NavEcos" <ecos at navosha dot com>]

----- Original Message -----
From: "Bart Veer" <bartv@ecoscentric.com>
To: <ecos@navosha.com>
Cc: <nickg@ecoscentric.com>; <ecos-patches@sources.redhat.com>
Sent: Saturday, August 31, 2002 9:13 AM
Subject: Re: cyg_alarm - suggestion for enhancement


> >>>>> "Rich" == NavEcos  <ecos@navosha.com> writes:
>
>     <snip>
>
>     > However, one thing that did occur to me is that the set function
>     > should be protected by taking the scheduler lock. If an
>     > interrupt occurs between setting the alarm function and setting
>     > the data, we could end up calling the function with the wrong
>     > data.
>     >
>     > So the set_callback function should look like this:
>     >
>     > inline void Cyg_Alarm::set_callback (
>     >         cyg_alarm_fn  *alarmfn,
>     >         CYG_ADDRWORD  newdata
>     >         )
>     > {
>     >     Cyg_Scheduler::lock();
>     >     alarm = alarmfn;
>     >     data = newdata;
>     >     Cyg_Scheduler::unlock();
>     > }
>     >
>     > I think with that change I would be happy to approve it.
>
>     Rich> I see your point here but I did it that way since I (maybe
>     Rich> incorrectly) assumed that the alarm would be in an inactive
>     Rich> state if you are changing the callback function. If the
>     Rich> retrigger value is non 0 you have a race condition because
>     Rich> if you change the CB while it's running, you cannot know
>     Rich> beforehand which CB will get called since you can be
>     Rich> pre-empted at anytime.
>
> I had assumed that set_callback() would only be used for alarms which
> were currently attached to a counter, most probably periodic ones. If
> an alarm is not currently attached to a counter then a new callback
> can be installed easily enough using cyg_alarm_delete() followed by
> another call to cyg_alarm_create(). It is only if the alarm is
> attached to a counter and you do not want it to lose its place in the
> queue that you need a new function.

Your comment about destroying an alarm and recreating is true, but it's
messy to do it that way and it's slower.  It seems a round about way to
change the call back function.  To make it worse: with the vxWorks watchdog
99.99% of the time you give the watchdog the same function so in reality
you'd just be destroying alarm to recreate it under eCos.

> It is true that there is a race condition, i.e. if the alarm triggers
> at about the same time that the callback is changed then you cannot be
> sure whether it is the old or the new callback that will get called.
> That is really not a serious problem. If say you tried to disable the
> alarm instead of changing the callback, there would still be a
> possibility that the alarm triggers just before the disable operation
> takes effect. Alarms are inherently asynchronous relative to thread
> execution, and they will go off at inconvenient times.

I agree this isn't a serious problem.  It's up to the coder to see that he
or she doesn't do dangerous stuff.

>     Rich> How about this: I prevent the callback from being changed
>     Rich> unless the alarm's retrigger time is set to 0 or the alarm
>     Rich> is disabled? After all: my justification for this is to make
>     Rich> it compatible with the wd functions of vxWorks, which
>     Rich> doesn't have a retrigger value. Still, there is the
>     Rich> possibility that a second thread could enable the alarm
>     Rich> while the alarm is being modified, but if a coder is doing
>     Rich> that, they are asking for trouble anyhow.
>
> I am not sure what this gains you. Nick's approach, locking the
> scheduler around the update, provides a safe implementation that will
> work irrespective of whether or not the alarm is currently attached to
> a counter. If not attached then locking the scheduler adds a few
> unnecessary cycles to execution time, but nothing major. Your approach
> also adds a few instructions, and makes the function less
> general-purpose.
>
> Bart

Either way.  I don't care enough to fight about it.  I'll make the
modification.  I'm essentially trying to duplicate the functionality of the
watchdog functions of vxWorks.  It is true that using a scheduler lock will
fix the race condition but it doesn't make sense to *me* to modify the
callback function while the alarm is running which is why I made the
suggestion above.

When I combine the reads I'll have to remember to use the scheduler lock as
well.  I'll get the code posted (again) to eCos patches tonight.  It's a
Saturday here, and I'd like to get out of the house for a bit.

-Rich