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

[Michael R. <mi...@re...>]

Hello Mattia,

I think you are right with your analysis and your solution.

What I currently do not understand: why would somebody do a malloc(sizeof(int)) ??




Am 2012-03-26 13:10, schrieb Mattia Jona-Lasinio:
> 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

-- 
Michael Reinelt <mi...@re...>
http://home.pages.at/reinelt
GPG-Key 0xDF13BA50
ICQ #288386781