Re: [PATCH 3/3] lib: sbi_timer: Add support for timer events

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, Apr 23, 2026 at 01:55:59PM +0530, Anup Patel wrote:
> On Thu, Apr 23, 2026 at 12:24 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed, Apr 15, 2026 at 04:30:02PM +0530, Anup Patel wrote:

> > > +void sbi_timer_event_start(struct sbi_timer_event *ev, u64 next_event)
> > > +{
> > > +     struct sbi_timer_event *tev, *next_ev = NULL;
> > > +     struct timer_state *tstate;
> > > +
> > > +     if (!ev)
> > > +             return;
> > > +
> > > +     /* Ensure that event is not on the per-HART event list */
> > > +     if (ev->hart_index > -1) {
> >
> > It is a bit worrying to permit sbi_timer_event_start() if the timer is
> > already on a list. Is that necessary, or could you return an error in
> > this case?
> >
> > I'm thinking problems like this -
> >
> > HART0                         HART1
> > sbi_timer_event_start()       sbi_timer_process()
> >
> >   if (hart_index > -1) {
> >                                 __sbi_timer_event_stop(ev)
> >                                 if (ev->callback) {
> >                                   spin_unlock();
> >     spin_lock();
> >     __sbi_timer_event_stop(ev)
> >     spin_unlock();
> >                                   callback_fn()
> >                                     sbi_timer_event_start(); /* re-add periodic timer */
> 
> It is generally good to have ability to stop a running timer
> event from any CPU but I agree the problem you are highlighting.
> I think the real problem in this patch is the spin_unlock()/spin_lock()
> dance around the callback function called by sbi_timer_process().
> 
> To avoid the spin_unlock()/spin_lock() dance in sbi_timer_process(),
> I think it is better to call the callback function with lock held and callback
> function can optionally return next_time_stamp if it wants to re-add
> periodic timer. Suggestions ?

Yes, if the timer callbacks are fine with it, then the simpler the better.

Returning next_time_stamp if a reschedule is needed seems reasonable to
me. sbi_timer_event_start_from_callback() would be okay too.

> > > +             tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> > > +                                             timer_state_off);
> > > +             spin_lock(&tstate->event_list_lock);
> > > +             __sbi_timer_event_stop(ev);
> > > +             spin_unlock(&tstate->event_list_lock);
> > > +     }
> > > +
> > > +     tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> > > +     spin_lock(&tstate->event_list_lock);
> > > +
> > > +     /* Find where to insert the event in per-HART event list */
> > > +     sbi_list_for_each_entry(tev, &tstate->event_list, head) {
> > > +             if (next_event < tev->time_stamp) {
> > > +                     next_ev = tev;
> > > +                     break;
> > > +             }
> > > +     }
> > > +
> > > +     /* Insert the event in per-HART event list */
> > > +     ev->hart_index = current_hartindex();
> > > +     ev->time_stamp = next_event;
> > > +     if (next_ev)
> > > +             sbi_list_add(&ev->head, &next_ev->head);
> > > +     else
> > > +             sbi_list_add_tail(&ev->head, &tstate->event_list);
> > > +
> > > +     __sbi_timer_update_device(tstate);
> > > +
> > > +     spin_unlock(&tstate->event_list_lock);
> > > +}
> > > +
> > > +void sbi_timer_event_stop(struct sbi_timer_event *ev)
> > > +{
> > > +     struct timer_state *tstate;
> > > +
> > > +     if (!ev)
> > > +             return;
> > > +
> > > +     /* Ensure that event is not on the per-HART event list */
> > > +     if (ev->hart_index > -1) {
> > > +             tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> > > +                                             timer_state_off);
> > > +             spin_lock(&tstate->event_list_lock);
> > > +             __sbi_timer_event_stop(ev);
> > > +             spin_unlock(&tstate->event_list_lock);
> >
> > Similar concern here, timer could be in the middle of running. Maybe it
> > is benign. I think it would be nice to make this a "sync" stop though,
> > so it can wait for potential running timers.
> >
> > timer event could have a 'running' state which is updated inside the
> > lock, then sbi_timer_event_stop() (and sbi_timer_exit()) could wait for
> > it to clear.
> 
> We might have to waste a lot of CPU cycles in the busy-loop.

That's true but I would expect it to not be common. Having the simpler
sync API as the first / default one would be good, add something more
complex if you find the need.

In any case, I think keeping the lock held over the callback should
make this a synchronous delete.

Thanks,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi