Re: [PATCH v1 3/3] drivers/tty: increase priority for tty_buffer_worker

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 3/3] drivers/tty: increase priority for tty_buffer_worker
Date: Thu, 10 Jan 2019 08:59:51 -0800	[thread overview]
Message-ID: <CAHk-=whSbmZm8aZyoR9XjoLzotbXdxNuUpcxUTBo7svS77R6+A@mail.gmail.com> (raw)
In-Reply-To: <20190110151953.qpat4t7lat6plfk6@pengutronix.de>

On Thu, Jan 10, 2019 at 7:19 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> It is for industrial low latency RS-422 based application. The loopback
> test is just easy way to test/reproduce it without additional hardware.
>
> What is good, mainlineable way to implement it?

I can easily see that for that specific use it might be the right
thing and the easiest way to give better latency guarantees. I just
don't think it's generally applicable, because of the reasons stated
(ie "giving one thread much higher priority can have some detrimental
effects on other cases").

And obviously for people who do *not* want this, the extra kthreads
for just tty flushing are just ugly overhead. We've tried to no longer
have a lot of special threads. At least yours isn't percpu..

I wonder if  you could get at least similar behavior by:

 - allocating your own workqueue

      tty_wq = alloc_workqueue("tty", WQ_HIGHPRI, 0);

 - and then instead of the current

     queue_work(system_unbound_wq, &buf->work);

   you'd queue it onto that WQ_HIGHPRI workqueue.

NOTE! The advantage of the above is that it should be much easier to
make conditional. It could be a small boot-time option to say "create
your own tty high-priority workqueue", and it would be off by default
- and the tty buffer code would just use the default
"system_unbound_wq" normally, but then with the special option to use
that HIGHPRI workqueue.

See what I'm saying? It shouldn't be all that different from what you
do in your patch-series, but at least this way it's an easy
conditional, and people who don't have your kind of special latency
issues would never see it (and the code would be maintainable).

And do explain in the commit messages what your real load is (the
loopback case can still be interesting to show numbers in a controlled
environment, but it's not very compelling from a "this is the _reason_
for the change", if you see what I mean).

How does that sound? I think it would be much more likely to be
acceptable, and it doesn't sound like a big change.

But I may have missed something.

             Linus