Messages in this thread |  | | Date | Mon, 15 May 2023 14:41:33 +0200 | | From | Sebastian Siewior <> | | Subject | Re: [PATCH v6 19/21] timer: Implement the hierarchical pull model |
| |
On 2023-05-15 12:50:25 [+0200], Anna-Maria Behnsen wrote: > The part will be replaced by: > > ---8<---- > > local_irq_disable(); > timer_lock_remote_bases(cpu);
Do you s/raw_spin_lock_irq/raw_spin_lock/ in timer_lock_remote_bases()? Otherwise you disable interrupts twice. Not wrong but not needed.
> raw_spin_lock(&tmc->lock); > > if (!tmc->online || !tmc->idle) { > timer_unlock_remote_bases(cpu); > goto unlock; > } else { > fetch_next_timer_interrupt_remote(jif, now, &tevt, cpu); > } > > timer_unlock_remote_bases(cpu);
The locking, unlocking and fetch_next_timer_interrupt_remote() is only used here. So it if it wouldn't be for tmc-> then you could do everything timer.c in once place…
> -> do the preparation and the walk > > unlock: > raw_spin_unlock_irq(&tmc->lock); > > ---8<---- > > As menitoned in the reply last week to Frederics objections regarding the > locking asymmetry, I would like to keep it to make the locking region of > timer base locks as small as possible and to prevent holding up to five > locks during the walk.
This looks okay. I wouldn't suggest to hold the timer_base::lock or tmigr_cpu::lock longer than needed. Both can be acquired independently cross CPU and have explicit locking order. So it is fine.
> Thanks, > > Anna-Maria
Sebastian
|  |