Re: [patch V2 31/50] posix-timers: Add proper state tracking

From: Anna-Maria Behnsen
Date: Wed Apr 17 2024 - 09:41:50 EST


Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:

> Right now the state tracking is done by two struct members:
>
> - it_active:
> A boolean which tracks armed/disarmed state
>
> - it_signal_seq:
> A sequence counter which is used to invalidate settings
> and prevent rearming
>
> Replace it_active with it_status and keep properly track about the states
> in one place.
>
> This allows to reuse it_signal_seq to track reprogramming, disarm and
> delete operations in order to drop signals which are related to the state
> previous of those operations.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>

[...]

> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -453,7 +453,6 @@ static void disarm_timer(struct k_itimer
> struct cpu_timer *ctmr = &timer->it.cpu;
> struct posix_cputimer_base *base;
>
> - timer->it_active = 0;
> if (!cpu_timer_dequeue(ctmr))
> return;
>
> @@ -494,11 +493,12 @@ static int posix_cpu_timer_del(struct k_
> */
> WARN_ON_ONCE(ctmr->head || timerqueue_node_queued(&ctmr->node));
> } else {
> - if (timer->it.cpu.firing)
> + if (timer->it.cpu.firing) {
> ret = TIMER_RETRY;
> - else
> + } else {
> disarm_timer(timer, p);
> -
> + timer->it_status = POSIX_TIMER_DISARMED;
> + }
> unlock_task_sighand(p, &flags);
> }

Why do you move the update of the it_status here and do not reuse the
place where you added the it_active in patch 21 "posix-cpu-timers: Make
k_itimer::it_active consistent"? Then the update of the state would
still be next to cpu_timer_dequeue().

[...]

> @@ -647,10 +650,10 @@ void common_timer_get(struct k_itimer *t
> /* interval timer ? */
> if (iv) {
> cur_setting->it_interval = ktime_to_timespec64(iv);
> - } else if (!timr->it_active) {
> + } else if (timr->it_status == POSIX_TIMER_DISARMED) {
> /*
> * SIGEV_NONE oneshot timers are never queued and therefore
> - * timr->it_active is always false. The check below
> + * timr->it_status is always DISARMED. The check below

s/DISARMED/POSIX_TIMER_DISARMED/

This change would help when using grep.

> * vs. remaining time will handle this case.
> *
> * For all other timers there is nothing to update here, so