[PATCH] serial: 8250_dma: add workqueue to flip tty buffer

[PATCH] serial: 8250_dma: add workqueue to flip tty buffer

[Xin Zhao]

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 preeempted by real-time
tasks or other high-prio tasks. dma_rx_complete calls spin_lock_irqsave
which do not disable preempt but disable migrate in rt-linux, leading to
the kworker thread running the dma_rx_complete work cannot be pulled by
other cpu when idle_balance, causing long delays.
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 wq_tty_flip in tty_port, allocating a workqueue using WQ_SYSFS,
so that we can set cpumask and nice dynamically.
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
processing interval remains between 10 and 15ms, no jitter occurs.

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

Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
---
 drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++++-
 drivers/tty/tty_buffer.c           |  2 +-
 include/linux/tty_port.h           |  1 +
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index bdd26c9f3..7ff705a78 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -207,6 +207,7 @@ EXPORT_SYMBOL_GPL(serial8250_rx_dma_flush);
 int serial8250_request_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma	*dma = p->dma;
+	struct tty_port		*tport = &p->port.state->port;
 	phys_addr_t rx_dma_addr = dma->rx_dma_addr ?
 				  dma->rx_dma_addr : p->port.mapbase;
 	phys_addr_t tx_dma_addr = dma->tx_dma_addr ?
@@ -244,6 +245,11 @@ int serial8250_request_dma(struct uart_8250_port *p)
 		goto release_rx;
 	}
 
+	/* Use the default workqueue then if alloc_workqueue failed */
+	tport->wq_tty_flip = alloc_workqueue("ttyS%d-flip-wq",
+					     WQ_UNBOUND | WQ_SYSFS,
+					     0, p->port.line);
+
 	dmaengine_slave_config(dma->rxchan, &dma->rxconf);
 
 	/* Get a channel for TX */
@@ -252,7 +258,7 @@ int serial8250_request_dma(struct uart_8250_port *p)
 						       p->port.dev, "tx");
 	if (!dma->txchan) {
 		ret = -ENODEV;
-		goto release_rx;
+		goto release_rx_wq;
 	}
 
 	/* 8250 tx dma requires dmaengine driver to support terminate */
@@ -294,6 +300,11 @@ int serial8250_request_dma(struct uart_8250_port *p)
 	return 0;
 err:
 	dma_release_channel(dma->txchan);
+release_rx_wq:
+	if (tport->wq_tty_flip) {
+		destroy_workqueue(tport->wq_tty_flip);
+		tport->wq_tty_flip = NULL;
+	}
 release_rx:
 	dma_release_channel(dma->rxchan);
 	return ret;
@@ -303,6 +314,7 @@ EXPORT_SYMBOL_GPL(serial8250_request_dma);
 void serial8250_release_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
+	struct tty_port *tport = &p->port.state->port;
 
 	if (!dma)
 		return;
@@ -322,6 +334,11 @@ void serial8250_release_dma(struct uart_8250_port *p)
 	dma->txchan = NULL;
 	dma->tx_running = 0;
 
+	if (tport->wq_tty_flip) {
+		destroy_workqueue(tport->wq_tty_flip);
+		tport->wq_tty_flip = NULL;
+	}
+
 	dev_dbg_ratelimited(p->port.dev, "dma channels released\n");
 }
 EXPORT_SYMBOL_GPL(serial8250_release_dma);
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 67271fc0b..7f83f377f 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -530,7 +530,7 @@ void tty_flip_buffer_push(struct tty_port *port)
 	struct tty_bufhead *buf = &port->buf;
 
 	tty_flip_buffer_commit(buf->tail);
-	queue_work(system_unbound_wq, &buf->work);
+	queue_work(port->wq_tty_flip ?: system_unbound_wq, &buf->work);
 }
 EXPORT_SYMBOL(tty_flip_buffer_push);
 
diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
index 332ddb936..f5a5e50ff 100644
--- a/include/linux/tty_port.h
+++ b/include/linux/tty_port.h
@@ -121,6 +121,7 @@ struct tty_port {
 	int			drain_delay;
 	struct kref		kref;
 	void			*client_data;
+	struct workqueue_struct *wq_tty_flip;
 };
 
 /* tty_port::iflags bits -- use atomic bit ops */
-- 
2.34.1

Re: [PATCH] serial: 8250_dma: add workqueue to flip tty buffer

[Greg KH]

On Fri, Oct 24, 2025 at 02:51:20PM +0800, 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 preeempted by real-time
> tasks or other high-prio tasks. dma_rx_complete calls spin_lock_irqsave
> which do not disable preempt but disable migrate in rt-linux, leading to
> the kworker thread running the dma_rx_complete work cannot be pulled by
> other cpu when idle_balance, causing long delays.
> 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 wq_tty_flip in tty_port, allocating a workqueue using WQ_SYSFS,
> so that we can set cpumask and nice dynamically.
> 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
> processing interval remains between 10 and 15ms, no jitter occurs.
> 
> ---
> 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
> 
> Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++++-
>  drivers/tty/tty_buffer.c           |  2 +-
>  include/linux/tty_port.h           |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index bdd26c9f3..7ff705a78 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -207,6 +207,7 @@ EXPORT_SYMBOL_GPL(serial8250_rx_dma_flush);
>  int serial8250_request_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma	*dma = p->dma;
> +	struct tty_port		*tport = &p->port.state->port;
>  	phys_addr_t rx_dma_addr = dma->rx_dma_addr ?
>  				  dma->rx_dma_addr : p->port.mapbase;
>  	phys_addr_t tx_dma_addr = dma->tx_dma_addr ?
> @@ -244,6 +245,11 @@ int serial8250_request_dma(struct uart_8250_port *p)
>  		goto release_rx;
>  	}
>  
> +	/* Use the default workqueue then if alloc_workqueue failed */
> +	tport->wq_tty_flip = alloc_workqueue("ttyS%d-flip-wq",
> +					     WQ_UNBOUND | WQ_SYSFS,
> +					     0, p->port.line);
> +
>  	dmaengine_slave_config(dma->rxchan, &dma->rxconf);
>  
>  	/* Get a channel for TX */
> @@ -252,7 +258,7 @@ int serial8250_request_dma(struct uart_8250_port *p)
>  						       p->port.dev, "tx");
>  	if (!dma->txchan) {
>  		ret = -ENODEV;
> -		goto release_rx;
> +		goto release_rx_wq;
>  	}
>  
>  	/* 8250 tx dma requires dmaengine driver to support terminate */
> @@ -294,6 +300,11 @@ int serial8250_request_dma(struct uart_8250_port *p)
>  	return 0;
>  err:
>  	dma_release_channel(dma->txchan);
> +release_rx_wq:
> +	if (tport->wq_tty_flip) {
> +		destroy_workqueue(tport->wq_tty_flip);
> +		tport->wq_tty_flip = NULL;
> +	}
>  release_rx:
>  	dma_release_channel(dma->rxchan);
>  	return ret;
> @@ -303,6 +314,7 @@ EXPORT_SYMBOL_GPL(serial8250_request_dma);
>  void serial8250_release_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma *dma = p->dma;
> +	struct tty_port *tport = &p->port.state->port;
>  
>  	if (!dma)
>  		return;
> @@ -322,6 +334,11 @@ void serial8250_release_dma(struct uart_8250_port *p)
>  	dma->txchan = NULL;
>  	dma->tx_running = 0;
>  
> +	if (tport->wq_tty_flip) {
> +		destroy_workqueue(tport->wq_tty_flip);
> +		tport->wq_tty_flip = NULL;
> +	}
> +
>  	dev_dbg_ratelimited(p->port.dev, "dma channels released\n");
>  }
>  EXPORT_SYMBOL_GPL(serial8250_release_dma);
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 67271fc0b..7f83f377f 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -530,7 +530,7 @@ void tty_flip_buffer_push(struct tty_port *port)
>  	struct tty_bufhead *buf = &port->buf;
>  
>  	tty_flip_buffer_commit(buf->tail);
> -	queue_work(system_unbound_wq, &buf->work);
> +	queue_work(port->wq_tty_flip ?: system_unbound_wq, &buf->work);

Why not just do this for all tty ports?  What is the benifit of keeping
this on the system_unbound_wq for all other tty devices?

>  }
>  EXPORT_SYMBOL(tty_flip_buffer_push);
>  
> diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
> index 332ddb936..f5a5e50ff 100644
> --- a/include/linux/tty_port.h
> +++ b/include/linux/tty_port.h
> @@ -121,6 +121,7 @@ struct tty_port {
>  	int			drain_delay;
>  	struct kref		kref;
>  	void			*client_data;
> +	struct workqueue_struct *wq_tty_flip;
>  };

You forgot to document this new member, and so the documentation build
will throw a warning.

thanks,

greg k-h

Re: [PATCH] serial: 8250_dma: add workqueue to flip tty buffer

[Xin Zhao]

On Fri, 24 Oct 2025 13:21:45 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:

> >  	tty_flip_buffer_commit(buf->tail);
> > -	queue_work(system_unbound_wq, &buf->work);
> > +	queue_work(port->wq_tty_flip ?: system_unbound_wq, &buf->work);
> 
> Why not just do this for all tty ports?  What is the benifit of keeping
> this on the system_unbound_wq for all other tty devices?

Thank you for your reminder. I think we should create a workqueue for all
tty_ports to avoid similar issues that other tty_ports might encounter later.
Creating a workqueue for each tty_port only slightly increases memory usage,
but it allows users to configure it, which should result in benefits far
outweighing the drawbacks.
I plan to allocate the workqueue in tty_port_init and delete it in
tty_port_destroy. I briefly scanned the code and found that currently,
tty_port_init seems to match one-to-one with tty_port_destroy. I’m not sure
if I have checked everything, so I would appreciate your suggestions about
it.
Additionally, I will use the name in tty_struct to differentiate the names
of the nodes created due to WQ_SYSFS flag. If the WQ_SYSFS flag is set and
a duplicate name is detected, a warning will be printed in dmesg, and the
creation will fail. I plan to directly assign system_unbound_wq to the
variable for wq_tty_flip when creation fails. This way, during each flip,
we won’t have to perform '?:' operation; I will only need to check in
tty_port_destroy whether it is system_unbound_wq. If it’s not, I will
destroy the workqueue. I’m wondering if my approach is appropriate, as is
there any case tty_port_destory is called without calling tty_port_init
before?


> You forgot to document this new member, and so the documentation build
> will throw a warning.

I thought about it later, and since this workqueue is related to the logic
of tty_bufhead, which is also used for flipping, I plan to place the
wq_tty_flip workqueue inside tty_buffer. Since it will be placed together
with work in tty_buffer, it is self-explanatory. If I organize it this way,
there shouldn't be a need for additional comments. What do you think?

--
Xin Zhao