Re: [PATCH v8] tty: tty_port: add workqueue to flip TTY buffer

[Jiri Slaby <jirislaby@kernel.org>]

On 23. 12. 25, 4:48, Xin Zhao wrote:
> On the embedded platform, certain critical data, such as IMU data, is
> transmitted through UART. The tty_flip_buffer_push() interface in the TTY
> layer uses system_dfl_wq to handle the flipping of the TTY buffer.
> Although the unbound workqueue can create new threads on demand and wake
> up the kworker thread on an idle CPU, it may be preempted by real-time
> tasks or other high-prio tasks.
> 
> flush_to_ldisc() needs to wake up the relevant data handle thread. When
> executing __wake_up_common_lock(), it calls spin_lock_irqsave(), which
> does not disable preemption but disables migration in RT-Linux. This
> prevents the kworker thread from being migrated to other cores by CPU's
> balancing logic, resulting in long delays. The call trace is as follows:
>      __wake_up_common_lock
>      __wake_up
>      ep_poll_callback
>      __wake_up_common
>      __wake_up_common_lock
>      __wake_up
>      n_tty_receive_buf_common
>      n_tty_receive_buf2
>      tty_ldisc_receive_buf
>      tty_port_default_receive_buf
>      flush_to_ldisc
> 
> In our system, the processing interval for each frame of IMU data
> transmitted via UART can experience significant jitter due to this issue.
> Instead of the expected 10 to 15 ms frame processing interval, we see
> spikes up to 30 to 35 ms. Moreover, in just one or two hours, there can
> be 2 to 3 occurrences of such high jitter, which is quite frequent. This
> jitter exceeds the software's tolerable limit of 20 ms.
> 
> Introduce flip_wq in tty_port which can be set by tty_port_link_wq() or as
> default linked to default workqueue allocated when tty_register_driver().
> The default workqueue is allocated with flag WQ_SYSFS, so that cpumask and
> nice can be set dynamically. The execution timing of tty_port_link_wq() is
> not clearly restricted. The newly added function tty_port_link_driver_wq()
> checks whether the flip_wq of the tty_port has already been assigned when
> linking the default tty_driver's workqueue to the port. After the user has
> set a custom workqueue for a certain tty_port using tty_port_link_wq(), the
> system will only use this custom workqueue, even if tty_driver does not
> have %TTY_DRIVER_CUSTOM_WORKQUEUE flag.
> 
> Introduce %TTY_DRIVER_CUSTOM_WORKQUEUE flag meaning not to create the
> default single tty_driver workqueue. Two reasons why need to introduce the
> %TTY_DRIVER_CUSTOM_WORKQUEUE flag:
> 1. If the WQ_SYSFS parameter is enabled, workqueue_sysfs_register() will
> fail when trying to create a workqueue with the same name. The pty is an
> example of this; if both CONFIG_LEGACY_PTYS and CONFIG_UNIX98_PTYS are
> enabled, the call to tty_register_driver() in unix98_pty_init() will fail.
> 2. Different tty ports may be used for different tasks, which may require
> separate core binding control via workqueues. In this case, the workqueue
> created by default in the tty driver is unnecessary. Enabling this flag
> prevents the creation of this redundant workqueue.
> 
> After applying this patch, we can set the related UART TTY flip buffer
> workqueue by sysfs. We set the cpumask to CPU cores associated with the
> IMU tasks, and set the nice to -20. Testing has shown significant
> improvement in the previously described issue, with almost no stuttering
> occurring anymore.
> 
> Signed-off-by: Xin Zhao <jackzxcui1989@163.com>

This forgotten XMAS present LGTM ;).

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

-- 
js
suse labs