Re: [PATCH] serial: imx: reduce RX interrupt frequency

From: Sergey Organov <sorganov@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Tomasz Moń" <tomasz.mon@camlingroup.com>,
	linux-serial@vger.kernel.org, "Jiri Slaby" <jirislaby@kernel.org>,
	"Fabio Estevam" <festevam@gmail.com>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"NXP Linux Team" <linux-imx@nxp.com>,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Shawn Guo" <shawnguo@kernel.org>,
	k.drobinski@camlintechnologies.com
Subject: Re: [PATCH] serial: imx: reduce RX interrupt frequency
Date: Wed, 05 Jan 2022 16:34:21 +0300	[thread overview]
Message-ID: <87tuei4882.fsf@osv.gnss.ru> (raw)
In-Reply-To: <20220104224900.u3omfbilejx2jawr@pengutronix.de> ("Uwe =?utf-8?Q?Kleine-K=C3=B6nig=22's?= message of "Tue, 4 Jan 2022 23:49:00 +0100")

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Tue, Jan 04, 2022 at 12:38:01PM +0100, Greg Kroah-Hartman wrote:
>> On Tue, Jan 04, 2022 at 12:13:06PM +0100, Tomasz Moń wrote:
>> > On 04.01.2022 11:54, Greg Kroah-Hartman wrote:
>> > > Why can't you do this dynamically based on the baud rate so as to always
>> > > work properly for all speeds without increased delays for slower ones?
>> > 
>> > Could you please advise on which baud rates to consider as slow? Does it
>> > sound good to have the old trigger level for rates up to and including
>> > 115200 and the new one for faster ones?
>> 
>> You tell me, you are the one seeing this issue and are seeing delays on
>> slower values with your change.  Do some testing to see where the curve
>> is.
>
> Maybe it's more sensible to make this even more dynamic: e.g. keep it at
> 1 as default and increase the water level when a certain irq frequency
> is reached?

Too complex, and too many questions, I'm afraid. What is "irq
frequency", exactly? For this particular driver, or overall system?
Measured on what time interval? What is the threshold? Do we drop the
water level back to 1 when "irq frequency" is down again? Will we just
create re-configure storm at some conditions? Etc.....

Personally, I don't think this AI is worth the trouble. If at all, this
will need to be generic implementation on upper level of TTY subsystem.
I suspect a lot of UARTs have the feature nowadays, and it'd be a bad
idea to implement the same complex policy in every separate driver.

I'm not in favor of dependency on baud rate as well, but if any,
dependency on baud rate looks better for me, being more straightforward.

For what it's worth, I'm +1 for the original version of the patch. I
work with RS232 ports a lot and always set the threshold high in my
drivers. Where latency matters, there are usually methods to get proper
upper-level protocols to reduce it, though ioctl() to set the level
manually might be useful at some extreme conditions.

Thanks,
-- Sergey Organov