Message-ID: <6db427e4-4a82-4c63-b840-92654baf7e6b@kernel.org>
Date: Mon, 8 Dec 2025 08:26:51 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Xin Zhao <jackzxcui1989@....com>, gregkh@...uxfoundation.org,
 tj@...nel.org
Cc: hch@...radead.org, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org
Subject: Re: [PATCH v5] tty: tty_port: add workqueue to flip tty buffer

Hi,

On 05. 12. 25, 4:08, 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

In commit logs, we tend to add () after function names.

> layer uses system_unbound_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 preeempted by real-time

Too many 'e's in preeempted :).

> tasks or other high-prio tasks.
> In flush_to_ldisc, when executing n_tty_receive_buf_common,
> it wakes up

What is this "it" supposed to refer to? In English, this "it" here 
actually refers to n_tty_receive_buf_common().

> other tasks. __wake_up_common_lock calls spin_lock_irqsave, which does
> not disable preemption but disable migration in RT-Linux. This prevents

"disables"

> the kworker thread from being migrated to other cores by CPU's balancing
> logic, resulting in long delays.

Here should be another \n to separate paragraphs.

> 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.

One more \n here too.

> Introduce flip_wq in tty_port which can be set by tty_port_link_wq or as
> default linked to workqueue allocated when tty_register_driver using flag
> WQ_SYSFS, so that cpumask and nice can be set dynamically.

This is a heavy sentence. Split it.

> Introduce TTY_DRIVER_CUSTOM_WORKQUEUE flag meaning not to create the
> default single tty_driver workqueue.
> We set the cpumask to the same cpu where the IMU data is handled and has
> less long-time high-prio jobs, and then set nice to -20, the frame

Period after 20, not comma.

> processing interval remains between 10 and 15ms, no jitter occurs.

You can perhaps use some LLM to rephrase the text?

> ---
> Change in v5:
> - Do not allocate workqueue twice when CONFIG_UNIX98_PTYS and
>    CONFIG_LEGACY_PTYS are all enabled.
> 
> Change in v4:
> - Simplify the logic for creating and releasing the workqueue,
>    as suggested by Tejun Heo.
> - Allocate single workqueue of one tty_driver as default, link it to
>    port when tty_port register device or tty_driver.
> - Introduce tty_port_link_wq to link specific workqueue to port.
> - Add driver flag TTY_DRIVER_CUSTOM_WORKQUEUE meaning not to create the
>    default single tty_driver workqueue.
> - Link to v4: https://lore.kernel.org/all/202512041303.7192024b-lkp@intel.com/T/#t
> 
> Change in v3:
> - Add tty flip workqueue for all tty ports, as suggested by Greg KH.
>    Every tty port use an individual flip workqueue, while all pty ports
>    share the same workqueue created in pty_flip_wq_init.
> - Modify the commit log to describe the reason for latency spikes in
>    RT-Linux.
> - Link to v3: https://lore.kernel.org/all/20251027060929.394053-1-jackzxcui1989@163.com/
> 
> Change in v2:
> - Do not add new module parameters
>    as suggested by Greg KH
> - Set WQ_SYSFS to allow properties changes from userspace
>    as suggested by Tejun Heo
> - Link to v2: https://lore.kernel.org/all/20251024155534.2302590-1-jackzxcui1989@163.com
> 
> Signed-off-by: Xin Zhao <jackzxcui1989@....com>

This S-O-B is too late -- it would be dropped. You have to add it before 
the first "---".

> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -44,6 +44,8 @@ static struct tty_driver *pts_driver;
>   static DEFINE_MUTEX(devpts_mutex);
>   #endif
>   
> +static struct workqueue_struct *pty_flip_wq;

It's not clear to me, why ptys need a separate wq. IOW: you should 
describe this in the commit log.

> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3452,6 +3452,20 @@ int tty_register_driver(struct tty_driver *driver)
>   			goto err_unreg_char;
>   	}
>   
> +	if (!(driver->flags & TTY_DRIVER_CUSTOM_WORKQUEUE)) {
> +		driver->flip_wq = alloc_workqueue("%s-flip-wq",
> +						WQ_UNBOUND | WQ_SYSFS,
> +						0, driver->name);

Do you have to wrap the line here?

> +		if (!driver->flip_wq) {
> +			error = -ENOMEM;
> +			goto err_unreg_char;

Who is going to free cdevs in this fail path?

> +		}
> +		for (i = 0; i < driver->num; i++) {
> +			if (driver->ports[i] && !driver->ports[i]->buf.flip_wq)

You test it here and again in tty_port_link_driver_wq().

> +				tty_port_link_driver_wq(driver->ports[i], driver);

There are not many drivers having tty ports set at this point. Why are 
you doing this here, actually? Given you do this later again in 
register_device().

> +		}
> +	}
> +
>   	scoped_guard(mutex, &tty_mutex)
>   		list_add(&driver->tty_drivers, &tty_drivers);
>   
> @@ -3475,6 +3489,9 @@ int tty_register_driver(struct tty_driver *driver)
>   	scoped_guard(mutex, &tty_mutex)
>   		list_del(&driver->tty_drivers);
>   
> +	if (!(driver->flags & TTY_DRIVER_CUSTOM_WORKQUEUE))
> +		destroy_workqueue(driver->flip_wq);
> +
>   err_unreg_char:
>   	unregister_chrdev_region(dev, driver->num);
>   err:
...

> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -103,6 +103,22 @@ void tty_port_init(struct tty_port *port)
>   }
>   EXPORT_SYMBOL(tty_port_init);
>   
> +/**
> + * tty_port_link_wq - link tty_port and flip workqueue
> + * @port: tty_port of the device
> + * @flip_wq: workqueue to queue flip buffer work on
> + *
> + * Assign a specific workqueue to a certain port, instead of using the
> + * workqueue allocated in tty_register_driver when TTY_DRIVER_CUSTOM_WORKQUEUE

The same as for commit logs: functions end with (). Furthermore, 
constants start with % IIRC. A period is missing too. It should sound like:
"when TTY_DRIVER_CUSTOM_WORKQUEUE is used."

> + *
> + * Note tty port api will not destroy the workqueue in tty_port_destroy.

the TTY port API

> + */
> +void tty_port_link_wq(struct tty_port *port, struct workqueue_struct *flip_wq)
> +{
> +	port->buf.flip_wq = flip_wq;
> +}
> +EXPORT_SYMBOL(tty_port_link_wq);

_GPL likely?

> +
>   /**
>    * tty_port_link_device - link tty and tty_port
>    * @port: tty_port of the device
> @@ -161,6 +177,7 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
>   		const struct attribute_group **attr_grp)
>   {
>   	tty_port_link_device(port, driver, index);
> +	tty_port_link_driver_wq(port, driver);
>   	return tty_register_device_attr(driver, index, device, drvdata,
>   			attr_grp);
>   }
> @@ -187,6 +204,7 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
>   	struct device *dev;
>   
>   	tty_port_link_device(port, driver, index);
> +	tty_port_link_driver_wq(port, driver);
>   
>   	dev = serdev_tty_port_register(port, host, parent, driver, index);
>   	if (PTR_ERR(dev) != -ENODEV) {
> @@ -718,6 +736,7 @@ int tty_port_install(struct tty_port *port, struct tty_driver *driver,
>   		struct tty_struct *tty)
>   {
>   	tty->port = port;
> +	tty_port_link_driver_wq(port, driver);
>   	return tty_standard_install(driver, tty);
>   }
>   EXPORT_SYMBOL_GPL(tty_port_install);
> diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
> index 31125e3be..48adcb0e8 100644
> --- a/include/linux/tty_buffer.h
> +++ b/include/linux/tty_buffer.h
> @@ -34,6 +34,7 @@ static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs)
>   
>   struct tty_bufhead {
>   	struct tty_buffer *head;	/* Queue head */
> +	struct workqueue_struct *flip_wq;
>   	struct work_struct work;
>   	struct mutex	   lock;
>   	atomic_t	   priority;
> diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
> index 188ee9b76..cd93345bd 100644
> --- a/include/linux/tty_driver.h
> +++ b/include/linux/tty_driver.h
> @@ -69,6 +69,10 @@ struct serial_struct;
>    *	Do not create numbered ``/dev`` nodes. For example, create
>    *	``/dev/ttyprintk`` and not ``/dev/ttyprintk0``. Applicable only when a
>    *	driver for a single tty device is being allocated.
> + *
> + * @TTY_DRIVER_CUSTOM_WORKQUEUE:
> + *	Do not create workqueue when tty_register_driver. Set flip buffer
> + *	workqueue by tty_port_link_wq every port.

Sorry, parser error.

>    */
>   enum tty_driver_flag {
>   	TTY_DRIVER_INSTALLED		= BIT(0),
> @@ -79,6 +83,7 @@ enum tty_driver_flag {
>   	TTY_DRIVER_HARDWARE_BREAK	= BIT(5),
>   	TTY_DRIVER_DYNAMIC_ALLOC	= BIT(6),
>   	TTY_DRIVER_UNNUMBERED_NODE	= BIT(7),
> +	TTY_DRIVER_CUSTOM_WORKQUEUE	= BIT(8),
>   };
>   
>   enum tty_driver_type {
> @@ -506,6 +511,7 @@ struct tty_operations {
>    * @flags: tty driver flags (%TTY_DRIVER_)
>    * @proc_entry: proc fs entry, used internally
>    * @other: driver of the linked tty; only used for the PTY driver
> + * @flip_wq: workqueue to queue flip buffer work on
>    * @ttys: array of active &struct tty_struct, set by tty_standard_install()
>    * @ports: array of &struct tty_port; can be set during initialization by
>    *	   tty_port_link_device() and similar
> @@ -539,6 +545,7 @@ struct tty_driver {
>   	unsigned long	flags;
>   	struct proc_dir_entry *proc_entry;
>   	struct tty_driver *other;
> +	struct workqueue_struct *flip_wq;
>   
>   	/*
>   	 * Pointer to the tty data structures
> diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
> index 332ddb936..86e01bd51 100644
> --- a/include/linux/tty_port.h
> +++ b/include/linux/tty_port.h
> @@ -138,6 +138,7 @@ struct tty_port {
>   					   kernel */
>   
>   void tty_port_init(struct tty_port *port);
> +void tty_port_link_wq(struct tty_port *port, struct workqueue_struct *flip_wq);
>   void tty_port_link_device(struct tty_port *port, struct tty_driver *driver,
>   		unsigned index);
>   struct device *tty_port_register_device(struct tty_port *port,
> @@ -165,6 +166,14 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
>   	return NULL;
>   }
>   
> +/* No effect when TTY_DRIVER_CUSTOM_WORKQUEUE, as driver->flip_wq is NULL */
> +static inline void tty_port_link_driver_wq(struct tty_port *port,
> +					   struct tty_driver *driver)

I am not sure why you introduce two interfaces: 
tty_port_link_driver_wq() and tty_port_link_wq(). Can't you add the if 
to the latter and drop the former? To me at least, the latter is confusing.

> +{
> +	if (!port->buf.flip_wq)
> +		port->buf.flip_wq = driver->flip_wq;
> +}
> +
>   /* If the cts flow control is enabled, return true. */
>   static inline bool tty_port_cts_enabled(const struct tty_port *port)
>   {

thanks,
-- 
js
suse labs

Powered by blists - more mailing lists