On Thu, May 08, 2025 at 07:39:12PM +0300, Vitaliy Makkoveev wrote:
> On Thu, May 08, 2025 at 02:25:04PM +0000, Visa Hankala wrote:
> > On Thu, May 08, 2025 at 03:43:13AM +0300, Vitaliy Makkoveev wrote:
> > > On Thu, May 08, 2025 at 03:32:22AM +0300, Vitaliy Makkoveev wrote:
> > > > The last one kernel locked from kern/kern_event.c. 
> > > > 
> > > > Introduce 'filt_timer' opaque structure to keep the timeout(9), the
> > > > `ft_reschedule' flag and protecting mutex(9). The timeout(9) handler
> > > > could reschedule itself, so `ft_reschedule' to prevent this while we
> > > > canceling timeout. The `ft_mtx' mutex(9) also protects the
> > > > re-initialization, because it could be concurrently accessed from
> > > > another filt_timermodify() thread.
> > 
> > Concurrent filt_timermodify() calls cannot happen, though, because
> > the kqueue subsystem serializes the same knote's f_attach, f_detach,
> > f_modify, and f_process calls automatically.
> > 
> 
> Nice to hear. So I need serialization only with running timeout handler.

There can be concurrency between the timeout and {f_modify,f_process}.
For example, the timeout can hit while the user process is reading
the event (f_process call). This is why you should not take shortcuts
with the locking. Your previous diff looked better in this respect.

> I removed recursion from filt_timeradd()/filt_timerexpire() to reduce
> `ft_mtx' usage.

If you want to refactor the code please do so in separate commits
so that it is easier to see that the logic remains correct. The gist
and the trickiest part of EVFILT_TIMER is the timeout and knote
handling, and this code should not have unnecessary duplication.

> +	ft = malloc(sizeof(*ft), M_KEVENT, M_WAITOK);
> +	mtx_init(&ft->ft_mtx, IPL_MPSAFE);

Please change this ipl argument to IPL_SOFTCLOCK because the mutex
is used in process context and timeout soft interrupt context.
IPL_MPSAFE is incorrect (it is not a proper IPL value but a hint to
interrupt handlers that the kernel lock is not needed).