* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
[not found] <E1aYgex-0004LE-7y@sd-51317.xenomai.org>
@ 2016-02-24 21:10 ` Gilles Chanteperdrix
2016-02-24 21:15 ` Jan Kiszka
0 siblings, 1 reply; 8+ messages in thread
From: Gilles Chanteperdrix @ 2016-02-24 21:10 UTC (permalink / raw)
To: xenomai
On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
> Module: xenomai-jki
> Branch: for-forge
> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
>
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date: Wed Feb 24 21:44:41 2016 +0100
>
> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
>
> xntimer_get_overruns might be called on meanwhile stopped timers,
> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
> stopped timer, and we should not restart it as well.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
> ---
>
> kernel/cobalt/timer.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
> index db6263d..3e85a53 100644
> --- a/kernel/cobalt/timer.c
> +++ b/kernel/cobalt/timer.c
> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
> overruns = xnarch_div64(delta, period);
> timer->pexpect_ticks += overruns;
>
> - q = xntimer_percpu_queue(timer);
> - xntimer_dequeue(timer, q);
> - while (xntimerh_date(&timer->aplink) < now) {
> - timer->periodic_ticks++;
> - xntimer_update_date(timer);
> + if (xntimer_running_p(timer)) {
> + q = xntimer_percpu_queue(timer);
> + xntimer_dequeue(timer, q);
> + while (xntimerh_date(&timer->aplink) < now) {
> + timer->periodic_ticks++;
> + xntimer_update_date(timer);
> + }
> + xntimer_enqueue_and_program(timer, q);
> }
> - xntimer_enqueue_and_program(timer, q);
This looks wrong. xntimer_get_overruns should not be called on a
stopped timer. We want the caller to return an error if called on a
stopped timer, not the condition being silently ignored.
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-24 21:10 ` [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers Gilles Chanteperdrix
@ 2016-02-24 21:15 ` Jan Kiszka
2016-02-24 21:20 ` Gilles Chanteperdrix
0 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2016-02-24 21:15 UTC (permalink / raw)
To: Gilles Chanteperdrix, xenomai
On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
>> Module: xenomai-jki
>> Branch: for-forge
>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>
>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>> Date: Wed Feb 24 21:44:41 2016 +0100
>>
>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
>>
>> xntimer_get_overruns might be called on meanwhile stopped timers,
>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
>> stopped timer, and we should not restart it as well.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> ---
>>
>> kernel/cobalt/timer.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
>> index db6263d..3e85a53 100644
>> --- a/kernel/cobalt/timer.c
>> +++ b/kernel/cobalt/timer.c
>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
>> overruns = xnarch_div64(delta, period);
>> timer->pexpect_ticks += overruns;
>>
>> - q = xntimer_percpu_queue(timer);
>> - xntimer_dequeue(timer, q);
>> - while (xntimerh_date(&timer->aplink) < now) {
>> - timer->periodic_ticks++;
>> - xntimer_update_date(timer);
>> + if (xntimer_running_p(timer)) {
>> + q = xntimer_percpu_queue(timer);
>> + xntimer_dequeue(timer, q);
>> + while (xntimerh_date(&timer->aplink) < now) {
>> + timer->periodic_ticks++;
>> + xntimer_update_date(timer);
>> + }
>> + xntimer_enqueue_and_program(timer, q);
>> }
>> - xntimer_enqueue_and_program(timer, q);
>
> This looks wrong. xntimer_get_overruns should not be called on a
> stopped timer. We want the caller to return an error if called on a
> stopped timer, not the condition being silently ignored.
The pattern that should trigger this (only tried with custom clocks so far):
- let a periodic timer send a signal
- block the signal so that it is queued
- stop the timer (timer_settime(0))
- process the signal (sigwait)
I don't see why the latter call should fail and not retrieve the queued
signals.
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-24 21:15 ` Jan Kiszka
@ 2016-02-24 21:20 ` Gilles Chanteperdrix
2016-02-24 21:21 ` Jan Kiszka
0 siblings, 1 reply; 8+ messages in thread
From: Gilles Chanteperdrix @ 2016-02-24 21:20 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai
On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
> > On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
> >> Module: xenomai-jki
> >> Branch: for-forge
> >> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >>
> >> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >> Date: Wed Feb 24 21:44:41 2016 +0100
> >>
> >> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
> >>
> >> xntimer_get_overruns might be called on meanwhile stopped timers,
> >> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
> >> stopped timer, and we should not restart it as well.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> ---
> >>
> >> kernel/cobalt/timer.c | 14 ++++++++------
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
> >> index db6263d..3e85a53 100644
> >> --- a/kernel/cobalt/timer.c
> >> +++ b/kernel/cobalt/timer.c
> >> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
> >> overruns = xnarch_div64(delta, period);
> >> timer->pexpect_ticks += overruns;
> >>
> >> - q = xntimer_percpu_queue(timer);
> >> - xntimer_dequeue(timer, q);
> >> - while (xntimerh_date(&timer->aplink) < now) {
> >> - timer->periodic_ticks++;
> >> - xntimer_update_date(timer);
> >> + if (xntimer_running_p(timer)) {
> >> + q = xntimer_percpu_queue(timer);
> >> + xntimer_dequeue(timer, q);
> >> + while (xntimerh_date(&timer->aplink) < now) {
> >> + timer->periodic_ticks++;
> >> + xntimer_update_date(timer);
> >> + }
> >> + xntimer_enqueue_and_program(timer, q);
> >> }
> >> - xntimer_enqueue_and_program(timer, q);
> >
> > This looks wrong. xntimer_get_overruns should not be called on a
> > stopped timer. We want the caller to return an error if called on a
> > stopped timer, not the condition being silently ignored.
>
> The pattern that should trigger this (only tried with custom clocks so far):
>
> - let a periodic timer send a signal
> - block the signal so that it is queued
> - stop the timer (timer_settime(0))
> - process the signal (sigwait)
>
> I don't see why the latter call should fail and not retrieve the queued
> signals.
Ok, but with your patch periodic_ticks no longer gets incremented in
that case, which looks wrong to me.
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-24 21:20 ` Gilles Chanteperdrix
@ 2016-02-24 21:21 ` Jan Kiszka
2016-02-25 15:11 ` Gilles Chanteperdrix
0 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2016-02-24 21:21 UTC (permalink / raw)
To: Gilles Chanteperdrix; +Cc: xenomai
On 2016-02-24 22:20, Gilles Chanteperdrix wrote:
> On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
>> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
>>> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-forge
>>>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>>
>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Date: Wed Feb 24 21:44:41 2016 +0100
>>>>
>>>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
>>>>
>>>> xntimer_get_overruns might be called on meanwhile stopped timers,
>>>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
>>>> stopped timer, and we should not restart it as well.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> ---
>>>>
>>>> kernel/cobalt/timer.c | 14 ++++++++------
>>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
>>>> index db6263d..3e85a53 100644
>>>> --- a/kernel/cobalt/timer.c
>>>> +++ b/kernel/cobalt/timer.c
>>>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
>>>> overruns = xnarch_div64(delta, period);
>>>> timer->pexpect_ticks += overruns;
>>>>
>>>> - q = xntimer_percpu_queue(timer);
>>>> - xntimer_dequeue(timer, q);
>>>> - while (xntimerh_date(&timer->aplink) < now) {
>>>> - timer->periodic_ticks++;
>>>> - xntimer_update_date(timer);
>>>> + if (xntimer_running_p(timer)) {
>>>> + q = xntimer_percpu_queue(timer);
>>>> + xntimer_dequeue(timer, q);
>>>> + while (xntimerh_date(&timer->aplink) < now) {
>>>> + timer->periodic_ticks++;
>>>> + xntimer_update_date(timer);
>>>> + }
>>>> + xntimer_enqueue_and_program(timer, q);
>>>> }
>>>> - xntimer_enqueue_and_program(timer, q);
>>>
>>> This looks wrong. xntimer_get_overruns should not be called on a
>>> stopped timer. We want the caller to return an error if called on a
>>> stopped timer, not the condition being silently ignored.
>>
>> The pattern that should trigger this (only tried with custom clocks so far):
>>
>> - let a periodic timer send a signal
>> - block the signal so that it is queued
>> - stop the timer (timer_settime(0))
>> - process the signal (sigwait)
>>
>> I don't see why the latter call should fail and not retrieve the queued
>> signals.
>
> Ok, but with your patch periodic_ticks no longer gets incremented in
> that case, which looks wrong to me.
>
To my understanding, we are only interested in the overruns in case of
stopped timer. Isn't periodic_ticks reset again start?
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-24 21:21 ` Jan Kiszka
@ 2016-02-25 15:11 ` Gilles Chanteperdrix
2016-02-25 15:17 ` Jan Kiszka
0 siblings, 1 reply; 8+ messages in thread
From: Gilles Chanteperdrix @ 2016-02-25 15:11 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai
On Wed, Feb 24, 2016 at 10:21:58PM +0100, Jan Kiszka wrote:
> On 2016-02-24 22:20, Gilles Chanteperdrix wrote:
> > On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
> >> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
> >>> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
> >>>> Module: xenomai-jki
> >>>> Branch: for-forge
> >>>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >>>>
> >>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Date: Wed Feb 24 21:44:41 2016 +0100
> >>>>
> >>>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
> >>>>
> >>>> xntimer_get_overruns might be called on meanwhile stopped timers,
> >>>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
> >>>> stopped timer, and we should not restart it as well.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> ---
> >>>>
> >>>> kernel/cobalt/timer.c | 14 ++++++++------
> >>>> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
> >>>> index db6263d..3e85a53 100644
> >>>> --- a/kernel/cobalt/timer.c
> >>>> +++ b/kernel/cobalt/timer.c
> >>>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
> >>>> overruns = xnarch_div64(delta, period);
> >>>> timer->pexpect_ticks += overruns;
> >>>>
> >>>> - q = xntimer_percpu_queue(timer);
> >>>> - xntimer_dequeue(timer, q);
> >>>> - while (xntimerh_date(&timer->aplink) < now) {
> >>>> - timer->periodic_ticks++;
> >>>> - xntimer_update_date(timer);
> >>>> + if (xntimer_running_p(timer)) {
> >>>> + q = xntimer_percpu_queue(timer);
> >>>> + xntimer_dequeue(timer, q);
> >>>> + while (xntimerh_date(&timer->aplink) < now) {
> >>>> + timer->periodic_ticks++;
> >>>> + xntimer_update_date(timer);
> >>>> + }
> >>>> + xntimer_enqueue_and_program(timer, q);
> >>>> }
> >>>> - xntimer_enqueue_and_program(timer, q);
> >>>
> >>> This looks wrong. xntimer_get_overruns should not be called on a
> >>> stopped timer. We want the caller to return an error if called on a
> >>> stopped timer, not the condition being silently ignored.
> >>
> >> The pattern that should trigger this (only tried with custom clocks so far):
> >>
> >> - let a periodic timer send a signal
> >> - block the signal so that it is queued
> >> - stop the timer (timer_settime(0))
> >> - process the signal (sigwait)
> >>
> >> I don't see why the latter call should fail and not retrieve the queued
> >> signals.
> >
> > Ok, but with your patch periodic_ticks no longer gets incremented in
> > that case, which looks wrong to me.
> >
>
> To my understanding, we are only interested in the overruns in case of
> stopped timer. Isn't periodic_ticks reset again start?
I am not sure I understand your sentence. Both the POSIX timer
interface and the Linux timerfd interface keep track of the overruns
at each tick, so no, we are interested in the overruns for every
tick.
The aim of this piece of code is to prevent xntimer_get_overruns to
account twice for the same overrun. I am not sure I understand well
enough the problem you are trying to avoid to know if yes or no, the
date should be incremented, but it seems simple to keep the part
that increments timer->perdiodic_ticks and only make the
unqueue/requeue code conditional.
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-25 15:11 ` Gilles Chanteperdrix
@ 2016-02-25 15:17 ` Jan Kiszka
2016-02-26 11:30 ` Gilles Chanteperdrix
0 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2016-02-25 15:17 UTC (permalink / raw)
To: Gilles Chanteperdrix; +Cc: xenomai
On 2016-02-25 16:11, Gilles Chanteperdrix wrote:
> On Wed, Feb 24, 2016 at 10:21:58PM +0100, Jan Kiszka wrote:
>> On 2016-02-24 22:20, Gilles Chanteperdrix wrote:
>>> On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
>>>> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
>>>>> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
>>>>>> Module: xenomai-jki
>>>>>> Branch: for-forge
>>>>>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>>>>
>>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Date: Wed Feb 24 21:44:41 2016 +0100
>>>>>>
>>>>>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
>>>>>>
>>>>>> xntimer_get_overruns might be called on meanwhile stopped timers,
>>>>>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
>>>>>> stopped timer, and we should not restart it as well.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> kernel/cobalt/timer.c | 14 ++++++++------
>>>>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
>>>>>> index db6263d..3e85a53 100644
>>>>>> --- a/kernel/cobalt/timer.c
>>>>>> +++ b/kernel/cobalt/timer.c
>>>>>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
>>>>>> overruns = xnarch_div64(delta, period);
>>>>>> timer->pexpect_ticks += overruns;
>>>>>>
>>>>>> - q = xntimer_percpu_queue(timer);
>>>>>> - xntimer_dequeue(timer, q);
>>>>>> - while (xntimerh_date(&timer->aplink) < now) {
>>>>>> - timer->periodic_ticks++;
>>>>>> - xntimer_update_date(timer);
>>>>>> + if (xntimer_running_p(timer)) {
>>>>>> + q = xntimer_percpu_queue(timer);
>>>>>> + xntimer_dequeue(timer, q);
>>>>>> + while (xntimerh_date(&timer->aplink) < now) {
>>>>>> + timer->periodic_ticks++;
>>>>>> + xntimer_update_date(timer);
>>>>>> + }
>>>>>> + xntimer_enqueue_and_program(timer, q);
>>>>>> }
>>>>>> - xntimer_enqueue_and_program(timer, q);
>>>>>
>>>>> This looks wrong. xntimer_get_overruns should not be called on a
>>>>> stopped timer. We want the caller to return an error if called on a
>>>>> stopped timer, not the condition being silently ignored.
>>>>
>>>> The pattern that should trigger this (only tried with custom clocks so far):
>>>>
>>>> - let a periodic timer send a signal
>>>> - block the signal so that it is queued
>>>> - stop the timer (timer_settime(0))
>>>> - process the signal (sigwait)
>>>>
>>>> I don't see why the latter call should fail and not retrieve the queued
>>>> signals.
>>>
>>> Ok, but with your patch periodic_ticks no longer gets incremented in
>>> that case, which looks wrong to me.
>>>
>>
>> To my understanding, we are only interested in the overruns in case of
>> stopped timer. Isn't periodic_ticks reset again start?
>
> I am not sure I understand your sentence. Both the POSIX timer
> interface and the Linux timerfd interface keep track of the overruns
> at each tick, so no, we are interested in the overruns for every
> tick.
>
> The aim of this piece of code is to prevent xntimer_get_overruns to
> account twice for the same overrun. I am not sure I understand well
> enough the problem you are trying to avoid to know if yes or no, the
> date should be incremented, but it seems simple to keep the part
> that increments timer->perdiodic_ticks and only make the
> unqueue/requeue code conditional.
The timer is stopped in the case the change covers, so periodic_ticks
plays no role any more until the timer is restart - in which case
periodic_ticks is reset anyway.
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-25 15:17 ` Jan Kiszka
@ 2016-02-26 11:30 ` Gilles Chanteperdrix
2016-02-26 11:34 ` Jan Kiszka
0 siblings, 1 reply; 8+ messages in thread
From: Gilles Chanteperdrix @ 2016-02-26 11:30 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai
On Thu, Feb 25, 2016 at 04:17:11PM +0100, Jan Kiszka wrote:
> On 2016-02-25 16:11, Gilles Chanteperdrix wrote:
> > On Wed, Feb 24, 2016 at 10:21:58PM +0100, Jan Kiszka wrote:
> >> On 2016-02-24 22:20, Gilles Chanteperdrix wrote:
> >>> On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
> >>>> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
> >>>>> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
> >>>>>> Module: xenomai-jki
> >>>>>> Branch: for-forge
> >>>>>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
> >>>>>>
> >>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> Date: Wed Feb 24 21:44:41 2016 +0100
> >>>>>>
> >>>>>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
> >>>>>>
> >>>>>> xntimer_get_overruns might be called on meanwhile stopped timers,
> >>>>>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
> >>>>>> stopped timer, and we should not restart it as well.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> kernel/cobalt/timer.c | 14 ++++++++------
> >>>>>> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
> >>>>>> index db6263d..3e85a53 100644
> >>>>>> --- a/kernel/cobalt/timer.c
> >>>>>> +++ b/kernel/cobalt/timer.c
> >>>>>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
> >>>>>> overruns = xnarch_div64(delta, period);
> >>>>>> timer->pexpect_ticks += overruns;
> >>>>>>
> >>>>>> - q = xntimer_percpu_queue(timer);
> >>>>>> - xntimer_dequeue(timer, q);
> >>>>>> - while (xntimerh_date(&timer->aplink) < now) {
> >>>>>> - timer->periodic_ticks++;
> >>>>>> - xntimer_update_date(timer);
> >>>>>> + if (xntimer_running_p(timer)) {
> >>>>>> + q = xntimer_percpu_queue(timer);
> >>>>>> + xntimer_dequeue(timer, q);
> >>>>>> + while (xntimerh_date(&timer->aplink) < now) {
> >>>>>> + timer->periodic_ticks++;
> >>>>>> + xntimer_update_date(timer);
> >>>>>> + }
> >>>>>> + xntimer_enqueue_and_program(timer, q);
> >>>>>> }
> >>>>>> - xntimer_enqueue_and_program(timer, q);
> >>>>>
> >>>>> This looks wrong. xntimer_get_overruns should not be called on a
> >>>>> stopped timer. We want the caller to return an error if called on a
> >>>>> stopped timer, not the condition being silently ignored.
> >>>>
> >>>> The pattern that should trigger this (only tried with custom clocks so far):
> >>>>
> >>>> - let a periodic timer send a signal
> >>>> - block the signal so that it is queued
> >>>> - stop the timer (timer_settime(0))
> >>>> - process the signal (sigwait)
> >>>>
> >>>> I don't see why the latter call should fail and not retrieve the queued
> >>>> signals.
> >>>
> >>> Ok, but with your patch periodic_ticks no longer gets incremented in
> >>> that case, which looks wrong to me.
> >>>
> >>
> >> To my understanding, we are only interested in the overruns in case of
> >> stopped timer. Isn't periodic_ticks reset again start?
> >
> > I am not sure I understand your sentence. Both the POSIX timer
> > interface and the Linux timerfd interface keep track of the overruns
> > at each tick, so no, we are interested in the overruns for every
> > tick.
> >
> > The aim of this piece of code is to prevent xntimer_get_overruns to
> > account twice for the same overrun. I am not sure I understand well
> > enough the problem you are trying to avoid to know if yes or no, the
> > date should be incremented, but it seems simple to keep the part
> > that increments timer->perdiodic_ticks and only make the
> > unqueue/requeue code conditional.
>
> The timer is stopped in the case the change covers, so periodic_ticks
> plays no role any more until the timer is restart - in which case
> periodic_ticks is reset anyway.
I am OK with the patch.
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers
2016-02-26 11:30 ` Gilles Chanteperdrix
@ 2016-02-26 11:34 ` Jan Kiszka
0 siblings, 0 replies; 8+ messages in thread
From: Jan Kiszka @ 2016-02-26 11:34 UTC (permalink / raw)
To: Gilles Chanteperdrix; +Cc: xenomai
On 2016-02-26 12:30, Gilles Chanteperdrix wrote:
> On Thu, Feb 25, 2016 at 04:17:11PM +0100, Jan Kiszka wrote:
>> On 2016-02-25 16:11, Gilles Chanteperdrix wrote:
>>> On Wed, Feb 24, 2016 at 10:21:58PM +0100, Jan Kiszka wrote:
>>>> On 2016-02-24 22:20, Gilles Chanteperdrix wrote:
>>>>> On Wed, Feb 24, 2016 at 10:15:20PM +0100, Jan Kiszka wrote:
>>>>>> On 2016-02-24 22:10, Gilles Chanteperdrix wrote:
>>>>>>> On Wed, Feb 24, 2016 at 10:07:47PM +0100, git repository hosting wrote:
>>>>>>>> Module: xenomai-jki
>>>>>>>> Branch: for-forge
>>>>>>>> Commit: 8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=8e6d76e07af74056f6cb4d8cfb192485d07732f7
>>>>>>>>
>>>>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> Date: Wed Feb 24 21:44:41 2016 +0100
>>>>>>>>
>>>>>>>> cobalt/timer: Make xntimer_get_overruns robust against stopped timers
>>>>>>>>
>>>>>>>> xntimer_get_overruns might be called on meanwhile stopped timers,
>>>>>>>> specifically by cobalt_timer_deliver. We crash if we try to dequeue a
>>>>>>>> stopped timer, and we should not restart it as well.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> kernel/cobalt/timer.c | 14 ++++++++------
>>>>>>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/kernel/cobalt/timer.c b/kernel/cobalt/timer.c
>>>>>>>> index db6263d..3e85a53 100644
>>>>>>>> --- a/kernel/cobalt/timer.c
>>>>>>>> +++ b/kernel/cobalt/timer.c
>>>>>>>> @@ -602,13 +602,15 @@ unsigned long long xntimer_get_overruns(struct xntimer *timer, xnticks_t now)
>>>>>>>> overruns = xnarch_div64(delta, period);
>>>>>>>> timer->pexpect_ticks += overruns;
>>>>>>>>
>>>>>>>> - q = xntimer_percpu_queue(timer);
>>>>>>>> - xntimer_dequeue(timer, q);
>>>>>>>> - while (xntimerh_date(&timer->aplink) < now) {
>>>>>>>> - timer->periodic_ticks++;
>>>>>>>> - xntimer_update_date(timer);
>>>>>>>> + if (xntimer_running_p(timer)) {
>>>>>>>> + q = xntimer_percpu_queue(timer);
>>>>>>>> + xntimer_dequeue(timer, q);
>>>>>>>> + while (xntimerh_date(&timer->aplink) < now) {
>>>>>>>> + timer->periodic_ticks++;
>>>>>>>> + xntimer_update_date(timer);
>>>>>>>> + }
>>>>>>>> + xntimer_enqueue_and_program(timer, q);
>>>>>>>> }
>>>>>>>> - xntimer_enqueue_and_program(timer, q);
>>>>>>>
>>>>>>> This looks wrong. xntimer_get_overruns should not be called on a
>>>>>>> stopped timer. We want the caller to return an error if called on a
>>>>>>> stopped timer, not the condition being silently ignored.
>>>>>>
>>>>>> The pattern that should trigger this (only tried with custom clocks so far):
>>>>>>
>>>>>> - let a periodic timer send a signal
>>>>>> - block the signal so that it is queued
>>>>>> - stop the timer (timer_settime(0))
>>>>>> - process the signal (sigwait)
>>>>>>
>>>>>> I don't see why the latter call should fail and not retrieve the queued
>>>>>> signals.
>>>>>
>>>>> Ok, but with your patch periodic_ticks no longer gets incremented in
>>>>> that case, which looks wrong to me.
>>>>>
>>>>
>>>> To my understanding, we are only interested in the overruns in case of
>>>> stopped timer. Isn't periodic_ticks reset again start?
>>>
>>> I am not sure I understand your sentence. Both the POSIX timer
>>> interface and the Linux timerfd interface keep track of the overruns
>>> at each tick, so no, we are interested in the overruns for every
>>> tick.
>>>
>>> The aim of this piece of code is to prevent xntimer_get_overruns to
>>> account twice for the same overrun. I am not sure I understand well
>>> enough the problem you are trying to avoid to know if yes or no, the
>>> date should be incremented, but it seems simple to keep the part
>>> that increments timer->perdiodic_ticks and only make the
>>> unqueue/requeue code conditional.
>>
>> The timer is stopped in the case the change covers, so periodic_ticks
>> plays no role any more until the timer is restart - in which case
>> periodic_ticks is reset anyway.
>
> I am OK with the patch.
>
Thanks for the thorough review!
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-26 11:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <E1aYgex-0004LE-7y@sd-51317.xenomai.org>
2016-02-24 21:10 ` [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers Gilles Chanteperdrix
2016-02-24 21:15 ` Jan Kiszka
2016-02-24 21:20 ` Gilles Chanteperdrix
2016-02-24 21:21 ` Jan Kiszka
2016-02-25 15:11 ` Gilles Chanteperdrix
2016-02-25 15:17 ` Jan Kiszka
2016-02-26 11:30 ` Gilles Chanteperdrix
2016-02-26 11:34 ` Jan Kiszka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.