From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 26 Feb 2016 12:30:10 +0100 From: Gilles Chanteperdrix Message-ID: <20160226113010.GL31084@hermes.click-hack.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56CF1AF7.3020608@siemens.com> 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: Jan Kiszka Cc: xenomai@xenomai.org 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. -- Gilles. https://click-hack.org