Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Mar 2019 09:23:13 +0100
From:   Alexander Sverdlin <alexander.sverdlin@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Steven Walter <stevenrwalter@gmail.com>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/3] drivers/tty: convert tty_port to use
 kthread_worker
Message-Id: <20190311092313.ebf38daafe4e5b0a6fcb838f@gmail.com>
In-Reply-To: <20190110101232.9398-3-o.rempel@pengutronix.de>
References: <20190110101232.9398-1-o.rempel@pengutronix.de>
        <20190110101232.9398-3-o.rempel@pengutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Oleksij,

On Thu, 10 Jan 2019 11:12:31 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> From: Steven Walter <stevenrwalter@gmail.com>
> 
> Use kthread_worker instead of workqueues.  For now there is only a
> single workqueue, but the intention is to bring back the "low_latency"
> tty option, along with a second high-priority kthread worker.
> 
> Even without a second worker this patch allows to give a higher priority
> to tty processing by modifying the priority of the corresponding
> kthread.
> 
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

In general I agree with Linus, that a thread with some random
priority is suboptimal, and we actually should move this
work into the context of the receiving thread, to properly
inherit its priority. But this would be quite amount of rework.

In the meanwhile this patch series restores the "low_latency"
tty option. Thanks for your efforts!

> ---
>  drivers/tty/tty_buffer.c | 21 ++++++++++++++-------
>  drivers/tty/tty_io.c     |  1 +
>  include/linux/tty.h      |  7 ++++---
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index e0090e65d83a..18bd7f48a319 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/types.h>
> +#include <linux/kthread.h>
>  #include <linux/errno.h>
>  #include <linux/tty.h>
>  #include <linux/tty_driver.h>
> @@ -497,7 +498,7 @@ receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
>   *		 'consumer'
>   */
>  
> -static void flush_to_ldisc(struct work_struct *work)
> +static void flush_to_ldisc(struct kthread_work *work)
>  {
>  	struct tty_port *port = container_of(work, struct tty_port, buf.work);
>  	struct tty_bufhead *buf = &port->buf;
> @@ -557,10 +558,16 @@ void tty_flip_buffer_push(struct tty_port *port)
>  }
>  EXPORT_SYMBOL(tty_flip_buffer_push);
>  
> +static DEFINE_KTHREAD_WORKER(tty_buffer_worker);
> +
>  bool tty_buffer_queue_work(struct tty_port *port)
>  {
> -	struct tty_bufhead *buf = &port->buf;
> -	return schedule_work(&buf->work);
> +	return kthread_queue_work(&tty_buffer_worker, &port->buf.work);
> +}
> +
> +void tty_buffer_init_kthread(void)
> +{
> +	kthread_run(kthread_worker_fn, &tty_buffer_worker, "tty");
>  }
>  
>  /**
> @@ -582,7 +589,7 @@ 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);
> +	kthread_init_work(&buf->work, flush_to_ldisc);
>  	buf->mem_limit = TTYB_DEFAULT_MEM_LIMIT;
>  }
>  
> @@ -614,12 +621,12 @@ bool tty_buffer_restart_work(struct tty_port *port)
>  	return tty_buffer_queue_work(port);
>  }
>  
> -bool tty_buffer_cancel_work(struct tty_port *port)
> +void tty_buffer_cancel_work(struct tty_port *port)
>  {
> -	return cancel_work_sync(&port->buf.work);
> +	tty_buffer_flush_work(port);
>  }
>  
>  void tty_buffer_flush_work(struct tty_port *port)
>  {
> -	flush_work(&port->buf.work);
> +	kthread_flush_work(&port->buf.work);
>  }
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index bfe9ad85b362..5fd7cecbe4e7 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3476,6 +3476,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 226a9eff0766..924c1093967e 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -3,9 +3,9 @@
>  #define _LINUX_TTY_H
>  
>  #include <linux/fs.h>
> +#include <linux/kthread.h>
>  #include <linux/major.h>
>  #include <linux/termios.h>
> -#include <linux/workqueue.h>
>  #include <linux/tty_driver.h>
>  #include <linux/tty_ldisc.h>
>  #include <linux/mutex.h>
> @@ -84,7 +84,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 work;
>  	struct mutex	   lock;
>  	atomic_t	   priority;
>  	struct tty_buffer sentinel;
> @@ -510,8 +510,9 @@ extern void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld);
>  extern bool tty_buffer_queue_work(struct tty_port *port);
>  extern void tty_buffer_init(struct tty_port *port);
>  extern void tty_buffer_set_lock_subclass(struct tty_port *port);
> +extern void tty_buffer_init_kthread(void);
>  extern bool tty_buffer_restart_work(struct tty_port *port);
> -extern bool tty_buffer_cancel_work(struct tty_port *port);
> +extern void tty_buffer_cancel_work(struct tty_port *port);
>  extern void tty_buffer_flush_work(struct tty_port *port);
>  extern speed_t tty_termios_baud_rate(struct ktermios *termios);
>  extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);


-- 
Alexander Sverdlin.