Re: [Lcd4linux-devel] lcd4linux: timers and timer groups

[Mattia Jona-L. <mat...@gm...>]

Hmmmm, yes, I see, I didn't think about that. In the meantime here's
another patch for the realloc problem. This is to be really picky.

At line 194 of timer_group.c we have

    if (group == nTimerGroups) {
        TIMER_GROUP *tmp;
	
	if ((tmp = realloc(TimerGroups, (nTimerGroups + 1) *
sizeof(*TimerGroups))) == NULL) {
	    /* signal unsuccessful timer group creation */
	    return -1;
	}
	TimerGroups = tmp;
	nTimerGroups++;

	if ((TimerGroups[group].interval = malloc(sizeof(int))) == NULL) {
	    /* signal unsuccessful timer group creation */
	    return -1;
	}
    }

This code is buggy because if the second malloc fails, the timer group
that was just created is incomplete but we already updated both
TimerGroups and nTimerGroups. Wrong!

    if (group == nTimerGroups) {
 	int *tmpi;
        TIMER_GROUP *tmpg;
	
	if ((tmpi = malloc(sizeof(int))) == NULL) {
	    /* signal unsuccessful timer group creation */
	    return -1;
	}

	if ((tmpg = realloc(TimerGroups, (nTimerGroups + 1) *
sizeof(*TimerGroups))) == NULL) {
	    /* signal unsuccessful timer group creation */
	    free(tmpi);
	    return -1;
	}
	TimerGroups = tmpg;
	TimerGroups[group].interval = tmpi;
	nTimerGroups++;
    }

This is my solution. If everybody agrees I commit the new patch.

Best

Mattia



On Mon, Mar 26, 2012 at 11:44 AM, Michael Reinelt <mi...@re...> wrote:
> Hi Mattia,
>
> sorry, I hate to :-), but I disagree again:
>
> the "root cause" for timers are various polling loops in varous drivers
> (e.g. keypad polling)
>
> these timers are really "continuous" timers, there is no need to resubmit,
> this would produce some overhead.
>
> Not to mention that I don't want to touch existing drivers that are known to
> work, and where I don't have the hardware to test them (I have a really lot
> of parallel port displays, I still have these displays, but I'm running out
> of parallel ports :-)
>
>
> Am 2012-03-26 11:05, schrieb Mattia Jona-Lasinio:
>
>> Oh and, by the way. I also find redundant the distinction between
>> one-shot timer and continuous timer. All timers should be one-shot. If
>> a timer wants to fire countinuously then it has to resubmit itself in
>> the callback function. I think that the whole timer framework would
>> benefit from this simplification.
>>
>> Best
>>
>> Mattia
>>
>> On Mon, Mar 26, 2012 at 9:44 AM, Mattia Jona-Lasinio
>> <mat...@gm...>  wrote:
>>>
>>> Hi everybody,
>>>
>>> nice to read you too Michael! I was thinking about your mail and I
>>> think that you have a point. When I suggested the linked list
>>> implementation I had in mind something similar to a scheduler.
>>> Actually the Linux scheduler works in a similar way! However her we
>>> don't need a fully featured scheduler because timers do not have a
>>> fixed timeslice (the interval field may vary) and their number is
>>> constant on average once the program is running. Even if we implement
>>> a time quantum, we still have a limited number of timers that does not
>>> require a real scheduler.
>>>
>>> Best
>>>
>>> Mattia
>>>
>>> On Mon, Mar 26, 2012 at 7:05 AM, Michael Reinelt<mi...@re...>
>>>  wrote:
>>>>
>>>> Hello everybode,
>>>>
>>>> to sum up some of the previous mails:
>>>>
>>>> 1. I don't like the idea of linked lists for timers (I like linked
>>>> lists,
>>>> but not in every case. In german we say "If you have a hammer,
>>>> everything
>>>> looks like a nail" :-)
>>>>
>>>> The current "variable length array" approach results in a single memory
>>>> object, which grows at the beginning, but should stay at constant size
>>>> and
>>>> memroy location. Timers come and go, but the maximum number of timers
>>>> should
>>>> be constant.
>>>>
>>>> A (double) linked list has several memory objects. As many timers are
>>>> "one-shot timers" they may be created and destroyed very regularly. This
>>>> would lead to lots of malloc()/free() calls.
>>>>
>>>>
>>>> 2. Probably I have found the problem with "twice evaluation": there are
>>>> some
>>>> lines of code in timer.c (but probably not in timer_group.c):
>>>>
>>>>    /* one-shot timers should NOT fire immediately, so delay them by a
>>>>       single timer interval */
>>>>    if (one_shot) {
>>>>        timer_inc(timer,&now);
>>>>    }
>>>>
>>>> so even if a new timer is added to the list after the currently being
>>>> processed timer, it will not be triggered in the current loop.
>>>>
>>>>
>>>> 3. I think having both timers and timer groups is a nuisance. Why do we
>>>> have
>>>> this? probably because timer-groups has been added as a "enhancement"
>>>> which
>>>> should not change current behaviour?
>>>>
>>>> Shall we try to "merge" them?
>>>>
>>>> IIRC the timer-groups have been introduced to avoid "fading" of timers
>>>> because of different execution time... is this correct? is there any
>>>> other
>>>> reason for having these groups?
>>>>
>>>> I think there should be a better solution for this. probably some kind
>>>> of
>>>> "quantisation", which means that timers cannot have an arbitrary
>>>> execution
>>>> time, but must fit into defined values (eg every 10 msec). So if a 50
>>>> msec
>>>> timer which is executed at 352 msec would get executed next time not at
>>>> 402
>>>> msec, but rounded down to 350 msec
>>>>
>>>>
>>>> Note that I wrote this email without having a close look at the code, so
>>>> I
>>>> may be plain wrong :-)
>>>>
>>>>
>>>>
>>>>
>>>> regards, Michael
>>>>
>>>>
>>>>
>>>> --
>>>> Michael Reinelt<mi...@re...>
>>>> http://home.pages.at/reinelt
>>>> GPG-Key 0xDF13BA50
>>>> ICQ #288386781
>>
>>
>>
>
> --
> Michael Reinelt <mi...@re...>
> http://home.pages.at/reinelt
> GPG-Key 0xDF13BA50
> ICQ #288386781