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

[Jiri Slaby]

Hi,

On 10. 12. 25, 4:18, 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_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 preempted by real-time
> tasks or other high-prio tasks.
...
> 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@xxxxxxx>

The commit log looks fine to me now.

> ---

I think Greg would mind missing "Change in v6" here :).

> Change in v5:
> - Do not allocate workqueue twice when CONFIG_UNIX98_PTYS and
>    CONFIG_LEGACY_PTYS are all enabled.
...
> --- 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;

Can't pty simply "link" to system_unbound_wq instead of allocating a 
custom one:

> @@ -407,6 +409,8 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
>   	o_tty->link = tty;
>   	tty_port_init(ports[0]);
>   	tty_port_init(ports[1]);
> +	tty_port_link_wq(ports[0], pty_flip_wq);
> +	tty_port_link_wq(ports[1], pty_flip_wq);
>   	tty_buffer_set_limit(ports[0], 8192);
>   	tty_buffer_set_limit(ports[1], 8192);
>   	o_tty->port = ports[0];
> @@ -536,14 +540,16 @@ static void __init legacy_pty_init(void)
...
> @@ -940,6 +948,9 @@ static inline void unix98_pty_init(void) { }
>   
>   static int __init pty_init(void)
>   {
> +	pty_flip_wq = alloc_workqueue("pty-flip-wq", WQ_UNBOUND | WQ_SYSFS, 0);
> +	if (!pty_flip_wq)
> +		panic("Couldn't allocate pty flip workqueue");

here ^^^?

> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
...
> @@ -3475,6 +3488,10 @@ int tty_register_driver(struct tty_driver *driver)
>   	scoped_guard(mutex, &tty_mutex)
>   		list_del(&driver->tty_drivers);
>   
> +err_destroy_wq:
> +	if (!(driver->flags & TTY_DRIVER_CUSTOM_WORKQUEUE))
> +		destroy_workqueue(driver->flip_wq);
> +

The fail path appears to be correct now.

> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -103,6 +103,26 @@ 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
> + *
> + * When %TTY_DRIVER_CUSTOM_WORKQUEUE is used, you must link every tty port to

Passive voice, please:
every tty port shall be linked to a workqueue manually by this function

> + * workqueue manually by this function, otherwise tty_flip_buffer_push() will
> + * see NULL flip_wq pointer when queue_work.

%NULL flip_wq pointer on queue_work().

> + * When %TTY_DRIVER_CUSTOM_WORKQUEUE is NOT used, you can also use the function

this function can be used

> + * to link a certain port to a specific workqueue, instead of using the
> + * workqueue allocated in tty_register_driver().
> + *
> + * Note tty port api will not destroy the workqueue in the TTY port API.

You sometimes write "tty port", other times "TTY port" -- unify to 
whatever surrounding code does.

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

> --- 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(). In the case, you must
> + *	set flip buffer workqueue by tty_port_link_wq() every port.

Do not create a workqueue in tty_register_driver(). When set, flip 
buffer workqueue shall be set by tty_port_link_wq() for every port.

> --- 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,17 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
>   	return NULL;
>   }
>   
> +/*
> + * Never overwrite the workqueue set by tty_port_link_wq().
> + * No effect when %TTY_DRIVER_CUSTOM_WORKQUEUE, as driver->flip_wq is NULL.

when %TTY_DRIVER_CUSTOM_WORKQUEUE is set

%NULL

thanks,
--
js
suse labs