Re: cyg_alarm - suggestion for enhancement

[Bart Veer <bartv at ecoscentric dot com>]

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

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.

    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