Hi all,
[other interested parties are welcome to contribute to the discussion]
On 06/09/2015 02:05 PM, Steven Walter wrote:
> Use a dedicated kthread for handling ports marked as low_latency. Since
> this thread is RT_FIFO, it is not subject to the same types of
> starvation as the normal priority kworker threads.
>
> Some UART applications are very latency sensitive. In at least one case
> (motor controller card controlled over RS-232), a reply needs to be sent
> within about 1ms after receiving a byte. Without this patch, normal
> priority kworker threads can be delayed substantially by userspace
> real-time threads. This has been observed to cause latencies up to
> 100ms. With this patch, no occurences of communications failure were
> observed, meaning the 1ms latency is reliably achieved.
For 4.4-rc*, I added an interface abstraction to the tty buffers
so that work can be restarted and cancelled with just a port parameter.
Please take a look.
I'd like to revisit this solution to RT serial input handling, and
re-open the discussion with the following topic/suggestions/open-issues.
First off, I've had a change-of-heart, and I think the kworker_thread
is a viable solution that we should take in mainline.
I've copied Alan on this because he has way more experience than me
with exposing userspace interfaces.
1. I'm concerned about hooking this up to the low_latency knob. There's lots
of old userspace utils that flip the ASYNC_LOW_LATENCY bit just because.
And plenty of out-of-tree drivers do that too just because. Plus look at
the in-tree drivers that set low_latency just because.
Multiple possibilities for exposing RT steering to userspace:
a. Existing solution - stick w/ low_latency
However, be aware that the current patch would be broken in this
regard because low_latency can be set via ioctl(TIOCSSERIAL) with a
running tty
b. Add an ASYNC_REALTIME bit to tty_port::flags
The benefit here is there is an existing interface that really is
never going away. The drawback is the it's only exposed via
TIOCGSERIAL/TIOCSSERIAL which could be an issue for other physical
transports that can't (or shouldn't) opt-in to the TIOCSSERIAL interface
c. We could expose the interface via sysfs knob(s).
d. (not a recommendation, just for completeness) Automagically on first
tty open based on the caller's scheduler.
e. some other solution not yet considered
2. How or whether to expose the RT prio level?
3. I'd like to split this patch into two patches:
a. the core functionality tied to a port->realtime steering knob w/o a
userspace interface, and
b. the userspace interface for configuring
4. I think the kthread should be per-port. The input handling for the
N_TTY line discipline can block for a number of a reasons that I think
should be per-tty (which means per-port). Note that the core already
allows multiple concurrent flush_to_ldisc() operations on different
ttys.
5. There's a technical issue with switching away from whatever input handling
method is current that needs to be dealt with: if a work is scheduled
but then the method is switched after the kworker starts running, then
cancelling might have raced and confuse what type of kworker to cancel.
Regards,
Peter Hurley
PS - I'd also like a userspace unit test that checks basic functionality. :)
I ask because I'd like to upstream a work-in-progress starter test-suite
for tty/serial before the end of the year. Not required for submission
though.
> Signed-off-by: Steven Walter <stevenrwalter@xxxxxxxxx>
> ---
> drivers/tty/tty_buffer.c | 39 +++++++++++++++++++++++++++++++++++----
> drivers/tty/tty_io.c | 1 +
> include/linux/tty.h | 3 +++
> 3 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index ed7b5c8..0560ca7 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -3,6 +3,7 @@
> */
>
> #include <linux/types.h>
> +#include <linux/kthread.h>
> #include <linux/errno.h>
> #include <linux/tty.h>
> #include <linux/tty_driver.h>
> @@ -11,6 +12,7 @@
> #include <linux/string.h>
> #include <linux/slab.h>
> #include <linux/sched.h>
> +#include <linux/sched/prio.h>
> #include <linux/wait.h>
> #include <linux/bitops.h>
> #include <linux/delay.h>
> @@ -431,9 +433,8 @@ receive_buf(struct tty_struct *tty, struct tty_buffer *head, int count)
> * 'consumer'
> */
>
> -static void flush_to_ldisc(struct work_struct *work)
> +static void flush_to_ldisc(struct tty_port *port)
> {
> - struct tty_port *port = container_of(work, struct tty_port, buf.work);
> struct tty_bufhead *buf = &port->buf;
> struct tty_struct *tty;
> struct tty_ldisc *disc;
> @@ -482,6 +483,18 @@ static void flush_to_ldisc(struct work_struct *work)
> tty_ldisc_deref(disc);
> }
>
> +static void flush_to_ldisc_kthread(struct kthread_work *work)
> +{
> + struct tty_port *port = container_of(work, struct tty_port, buf.kwork);
> + flush_to_ldisc(port);
> +}
> +
> +static void flush_to_ldisc_workq(struct work_struct *work)
> +{
> + struct tty_port *port = container_of(work, struct tty_port, buf.work);
> + flush_to_ldisc(port);
> +}
> +
> /**
> * tty_flush_to_ldisc
> * @tty: tty to push
> @@ -520,19 +533,36 @@ EXPORT_SYMBOL(tty_flip_buffer_push);
> * Must be called before the other tty buffer functions are used.
> */
>
> +static DEFINE_KTHREAD_WORKER(tty_buffer_worker);
>
> void tty_buffer_queue_work(struct tty_port *port)
> {
> struct tty_bufhead *buf = &port->buf;
> - schedule_work(&buf->work);
> + if (port->low_latency)
> + queue_kthread_work(&tty_buffer_worker, &buf->kwork);
> + else
> + schedule_work(&buf->work);
> }
>
> void tty_buffer_flush_work(struct tty_port *port)
> {
> struct tty_bufhead *buf = &port->buf;
> + /* Flush both so we don't have to worry about racing with
> + * changes to port->low_latency */
> + flush_kthread_work(&buf->kwork);
> flush_work(&buf->work);
> }
>
> +void tty_buffer_init_kthread()
> +{
> + struct task_struct *task;
> + /* Use same default priority as threaded irq handlers */
> + struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 };
> +
> + task = kthread_run(kthread_worker_fn, &tty_buffer_worker, "tty");
> + sched_setscheduler(task, SCHED_FIFO, ¶m);
> +}
> +
> void tty_buffer_init(struct tty_port *port)
> {
> struct tty_bufhead *buf = &port->buf;
> @@ -544,7 +574,8 @@ void tty_buffer_init(struct tty_port *port)
> init_llist_head(&buf->free);
> atomic_set(&buf->mem_used, 0);
> atomic_set(&buf->priority, 0);
> - INIT_WORK(&buf->work, flush_to_ldisc);
> + INIT_WORK(&buf->work, flush_to_ldisc_workq);
> + init_kthread_work(&buf->kwork, flush_to_ldisc_kthread);
> buf->mem_limit = TTYB_DEFAULT_MEM_LIMIT;
> }
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index c288473..abcd9a5 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3602,6 +3602,7 @@ void console_sysfs_notify(void)
> */
> int __init tty_init(void)
> {
> + tty_buffer_init_kthread();
> cdev_init(&tty_cdev, &tty_fops);
> if (cdev_add(&tty_cdev, MKDEV(TTYAUX_MAJOR, 0), 1) ||
> register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 7bad787..4dc584d 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -5,6 +5,7 @@
> #include <linux/major.h>
> #include <linux/termios.h>
> #include <linux/workqueue.h>
> +#include <linux/kthread.h>
> #include <linux/tty_driver.h>
> #include <linux/tty_ldisc.h>
> #include <linux/mutex.h>
> @@ -60,6 +61,7 @@ static inline char *flag_buf_ptr(struct tty_buffer *b, int ofs)
> struct tty_bufhead {
> struct tty_buffer *head; /* Queue head */
> struct work_struct work;
> + struct kthread_work kwork;
> struct mutex lock;
> atomic_t priority;
> struct tty_buffer sentinel;
> @@ -448,6 +450,7 @@ extern void tty_buffer_flush(struct tty_struct *tty);
> extern void tty_buffer_init(struct tty_port *port);
> extern void tty_buffer_queue_work(struct tty_port *port);
> extern void tty_buffer_flush_work(struct tty_port *port);
> +extern void tty_buffer_init_kthread(void);
> extern speed_t tty_termios_baud_rate(struct ktermios *termios);
> extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
> extern void tty_termios_encode_baud_rate(struct ktermios *termios,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
