Menu
▾
▴
Re: [Lcd4linux-devel] lcd4linux: timers and timer groups
|
From: Mattia Jona-L. <mat...@gm...> - 2012-03-26 13:00:01
|
I totally agree :) and the answer is: I have no idea :)
The only reason I see is that when calling
timer_add(timer_process_group, TimerGroups[group].interval, interval,
0) in timer_group.c, the time interval is passed as the argument of
the callback function. But we can make the interval member in the
TIMER_GROUP structure an int instead of int* and rewrite
timer_add(timer_process_group, &TimerGroups[group].interval, interval,
0).
On Mon, Mar 26, 2012 at 2:20 PM, Michael Reinelt <mi...@re...> wrote:
> 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
|
