Re: Clock.cxx: some bug fixes

[Nick Garnett <nickg at ecoscentric dot com>]

Jonathan Larmour <jifl@eCosCentric.com> writes:

> Nick Garnett wrote:
> > The following patch fixes a couple of bugs that have popped up on the
> > discussion list recently. The rem_alarm() fix is straightforward.
> > The tick() fix is a total rewrite of a section of code. Before
> > committing it I would like some others to look it over and check that
> > it does what it is supposed to. Since CVS diff has made the new code
> > hard to read in the patch, I have appended a clean copy to the end of
> > this message.
> 
> Perhaps the thing to do is write a test (or expand an existing one)
> that pokes it in the right way to cause the problems it's trying to
> fix, i.e. tweaking the alarms from within an alarm handler.

I have a simple test program that I used to check it out. I'll see if
I can turn it into something that will exercise things a little more
systematically.

> 
> > If I hear nothing in a few days, I'll go ahead and commit it.
> 
> I had already applied Thomas' patch (in a very slightly different way)
> at almost exactly the same time you sent this mail, sorry! The only
> difference (as I indicated in my mail) was to lock the scheduler
> _after_ the assign to alarm_list_ptr in the single list case. See the
> attached patch FAOD.
>

I'm not really sure that keeping the one or two instructions of that
assignment outside the lock is worth the obfuscation of the code that
results. There are now two lock calls and one unlock -- it just makes
the structure of the function a little more obscure. But it's not a
big deal and I wouldn't change it back now.


> I can't help but think that the extra checking for changes
> i.e. next_next, next_prev and head, can be a config option given this
> is a well-trodden piece of code.
>

Despite the fact that the diff has extracted some common pieces, this
really is a total reorganization of the code. The temporary lists are
gone and the way the loop is controlled is different. The only thing
that really remains is the bit that handles the alarm triggering.


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