This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: cyg_alarm - suggestion for enhancement
----- 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