Re: [PATCH] pps: improve PREEMPT_RT performance

[Calvin Owens]

On Monday 04/06 at 14:11 +0200, Michael Byczkowski wrote:
> Dear Rodolfo,
> 
> Andrew Morton pointed me your way as PPS maintainer. I'm running a
> precision NTP time server on a Raspberry Pi 5 with a PREEMPT_RT kernel
> and a u-blox ZED-F9P GPS receiver on a Rapsberry Pi HAT providing PPS via GPIO.
> 
> I found three areas in the PPS subsystem that cause unnecessary jitter
> under PREEMPT_RT, while being fully backward-compatible with non-RT
> kernels:
> 
> 1. pps-gpio: The IRQ handler is force-threaded on PREEMPT_RT, so the
>   PPS timestamp is captured after scheduling delay rather than at
>   interrupt entry. Fix: split into a hardirq primary handler (captures
>   timestamp only) and a threaded handler (processes the event).
>   On non-RT kernels, request_threaded_irq with an explicit primary
>   handler behaves identically to the current code.
> 
> 2. pps_device.lock: spinlock_t becomes a sleeping mutex on PREEMPT_RT,
>   allowing pps_event() to be preempted mid-update. Fix: convert to
>   raw_spinlock_t, which compiles to identical code on non-RT.
> 
> 3. pps_kc_hardpps_lock: Same issue as (2), in the kernel consumer path
>   that calls hardpps(). Fix: convert to DEFINE_RAW_SPINLOCK.

Hi Michael,

Thanks for working on this, and for the nice clean patches :)

Your series is happily running on my pps-gpio setup. When you resend
the patches, feel free to add:

Tested-by: Calvin Owens <calvin@xxxxxxxxxx>

One quick thought below:

> All three patches are tested on a Raspberry Pi 5 running a 7.0.0-rc6
> PREEMPT_RT kernel with a PPS-disciplined NTP server. They apply cleanly
> against mainline. On non-RT kernels, raw_spinlock_t compiles identically
> to spinlock_t, and request_threaded_irq with a primary handler works
> the same as request_irq — so there is zero behavioral change for
> non-RT users.
> 
> The patches are available as individual commits at:
>  https://github.com/by/linux-PPS
> 
> [PATCH 1/3] pps: pps-gpio: split handler into hardirq timestamp and threaded processing
> https://github.com/by/linux-PPS/commit/e811a4e6f63f39a782db2a2fe1588419de96275b
> 
> [PATCH 2/3] pps: convert pps_device lock to raw_spinlock for PREEMPT_RT
> (covers include/linux/pps_kernel.h, drivers/pps/kapi.c, drivers/pps/pps.c)
> https://github.com/by/linux-PPS/commit/918e6f9c87adc552bda3b9b5847eb535f943c68e
> https://github.com/by/linux-PPS/commit/d1eb9d80768f6648ff4638b5f83028344f0860e9
> https://github.com/by/linux-PPS/commit/1f149bf2c449a36730adb341fdf626ed07953f5f

These should be combined into one commit to avoid bisection problems,
the intermediate states don't compile. But maybe you already intend to
do that, I see you've grouped them together here.

Cheers,
Calvin