From mboxrd@z Thu Jan 1 00:00:00 1970 References: <20160224211048.GA31084@hermes.click-hack.org> <56CE1D68.5070309@siemens.com> <20160224212028.GB31084@hermes.click-hack.org> <56CE1EF6.5040408@siemens.com> <20160225151143.GE31084@hermes.click-hack.org> <56CF1AF7.3020608@siemens.com> <20160226113010.GL31084@hermes.click-hack.org> From: Jan Kiszka Message-ID: <56D03854.9010002@siemens.com> Date: Fri, 26 Feb 2016 12:34:44 +0100 MIME-Version: 1.0 In-Reply-To: <20160226113010.GL31084@hermes.click-hack.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/timer: Make xntimer_get_overruns robust against stopped timers List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gilles Chanteperdrix Cc: xenomai@xenomai.org 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 >>>>>>>> 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 >>>>>>>> >>>>>>>> --- >>>>>>>> >>>>>>>> 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