diff --git a/kernel/clock.cpp b/kernel/clock.cpp index f80b8300..2827b281 100644 --- a/kernel/clock.cpp +++ b/kernel/clock.cpp @@ -235,6 +235,17 @@ void Clock::Cancel(Timer* timer) UnlockClock(); } +bool Clock::TryCancel(Timer* timer) +{ + LockClock(); + bool active = timer->flags & TIMER_ACTIVE; + if ( active ) + Unlink(timer); + UnlockClock(); + return active; +} + + // TODO: We need some method for threads to sleep for real but still be // interrupted by signals. struct timespec Clock::SleepDelay(struct timespec duration) @@ -350,10 +361,16 @@ static void Clock__FireTimer(void* timer_ptr) timer->clock->LockClock(); timer->num_overrun_events = timer->num_firings_scheduled; timer->num_firings_scheduled = 0; + bool may_deallocate = timer->flags & TIMER_FUNC_MAY_DEALLOCATE_TIMER; timer->clock->UnlockClock(); Clock__DoFireTimer(timer); + // The handler may have deallocated the storage for the timer, don't touch + // it again. + if ( may_deallocate ) + return; + // If additional events happened during the time of the event handler, we'll // have to handle them because the firing bit is set. We'll schedule another // worker thread job and resume there, so this worker thread can continue to @@ -376,6 +393,7 @@ static void Clock__FireTimer_InterruptWorker(void* timer_ptr, void*, size_t) void Clock::FireTimer(Timer* timer) { timer->flags &= ~TIMER_ACTIVE; + bool may_deallocate = timer->flags & TIMER_FUNC_MAY_DEALLOCATE_TIMER; // If the CPU is currently interrupted, we call the timer callback directly // only if it is known to work when the interrupts are disabled on this CPU. @@ -389,7 +407,8 @@ void Clock::FireTimer(Timer* timer) timer->num_firings_scheduled++; else { - timer->flags |= TIMER_FIRING; + if ( !may_deallocate ) + timer->flags |= TIMER_FIRING; Interrupt::ScheduleWork(Clock__FireTimer_InterruptWorker, timer, NULL, 0); } } @@ -405,13 +424,15 @@ void Clock::FireTimer(Timer* timer) timer->num_firings_scheduled++; else { - timer->flags |= TIMER_FIRING; + if ( !may_deallocate ) + timer->flags |= TIMER_FIRING; Worker::Schedule(Clock__FireTimer, timer); } } // Rearm the timer only if it is periodic. - if ( timespec_le(timer->value.it_interval, timespec_nul()) ) + if ( may_deallocate || + timespec_le(timer->value.it_interval, timespec_nul()) ) return; // TODO: If the period is too short (such a single nanosecond) on a delay diff --git a/kernel/include/sortix/kernel/clock.h b/kernel/include/sortix/kernel/clock.h index 264d3603..eb69f20d 100644 --- a/kernel/include/sortix/kernel/clock.h +++ b/kernel/include/sortix/kernel/clock.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013 Jonas 'Sortie' Termansen. + * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen. * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -55,6 +55,7 @@ public: void Register(Timer* timer); void Unlink(Timer* timer); void Cancel(Timer* timer); + bool TryCancel(Timer* timer); void LockClock(); void UnlockClock(); struct timespec SleepDelay(struct timespec duration); diff --git a/kernel/include/sortix/kernel/timer.h b/kernel/include/sortix/kernel/timer.h index 02125cd1..49bcf05b 100644 --- a/kernel/include/sortix/kernel/timer.h +++ b/kernel/include/sortix/kernel/timer.h @@ -35,6 +35,28 @@ static const int TIMER_ACTIVE = 1 << 1; static const int TIMER_FIRING = 1 << 2; static const int TIMER_FUNC_INTERRUPT_HANDLER = 1 << 3; static const int TIMER_FUNC_ADVANCE_THREAD = 1 << 4; +// The timer callback may deallocate the timer itself. The timer data structure +// will not be touched by the timer clock after running the callback and the +// clock and timer implementation will not have any issues with deallocating it. +// This feature cannot be combined with periodic timers. The Cancel method may +// not be called, as it ensures consistency (the timer is cancelled if pending, +// and if it's firing, then waiting for it to complete). Instead, use TryCancel +// which will return false if the timer wasn't pending. If the timer has been +// armed, and the handler has not yet run, that means the handler is scheduled +// to run and it's not safe to deallocate until the handler runs. It is not +// possible call the Set method on an armed timer, unless the timer has been +// successfully TryCancelled, or the handler has run. It's the user's +// responsibility to ensure deallocation of the timer only happens if no other +// threads will use the timer data structure. I.e. if some code wants to +// TryCancel a timer, it must synchronize with the timer handler, so the timer +// handler doesn't deallocate the timer and then the other thread calls +// TryCancel on a freed pointer. +// The object containing the timer could contain a mutex and a bool of whether +// the timer is armed and the handler has not run. If there's a need to destroy +// the object, attempt to TryCancel and timer and do so if it succeeds, +// otherwise delay the destruction until the timer handler, which also grabs the +// mutex and checks whether object destruction is supposed to happen. +static const int TIMER_FUNC_MAY_DEALLOCATE_TIMER = 1 << 5; class Timer { @@ -62,6 +84,7 @@ public: void Detach(); bool IsAttached() const { return clock; } void Cancel(); + bool TryCancel(); Clock* GetClock() const { return clock; } void Get(struct itimerspec* current); void Set(struct itimerspec* value, struct itimerspec* ovalue, int flags, diff --git a/kernel/timer.cpp b/kernel/timer.cpp index d854400a..e8dea1a3 100644 --- a/kernel/timer.cpp +++ b/kernel/timer.cpp @@ -77,6 +77,13 @@ void Timer::Cancel() clock->Cancel(this); } +bool Timer::TryCancel() +{ + if ( clock ) + return clock->TryCancel(this); + return true; +} + void Timer::GetInternal(struct itimerspec* current) { if ( !(this->flags & TIMER_ACTIVE ) ) @@ -103,6 +110,8 @@ void Timer::Set(struct itimerspec* new_value, struct itimerspec* old_value, void* new_user) { assert(clock); + assert(!(flags & TIMER_FUNC_MAY_DEALLOCATE_TIMER) || + timespec_le(new_value->it_interval, timespec_nul())); clock->LockClock();