[PATCH] serial: imx: reduce RX interrupt frequency

[PATCH] serial: imx: reduce RX interrupt frequency

[Tomasz Moń]

Triggering RX interrupt for every byte defeats the purpose of aging
timer and leads to interrupt starvation at high baud rates.

Increase receiver trigger level to 8 to increase the minimum period
between RX interrupts to 8 characters time. The tradeoff is increased
latency. In the worst case scenario, where RX data has intercharacter
delay slightly less than aging timer (8 characters time), it can take
up to 63 characters time for the interrupt to be raised since the
reception of the first character.

Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 90f82e6c54e4..3c812c47ecc0 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1255,7 +1255,7 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport)
 }
 
 #define TXTL_DEFAULT 2 /* reset default */
-#define RXTL_DEFAULT 1 /* reset default */
+#define RXTL_DEFAULT 8 /* 8 characters or aging timer */
 #define TXTL_DMA 8 /* DMA burst setting */
 #define RXTL_DMA 9 /* DMA burst setting */
 
-- 
2.25.1

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

[Ahmad Fatoum]

On 04.01.22 11:32, Tomasz Moń wrote:
> Triggering RX interrupt for every byte defeats the purpose of aging
> timer and leads to interrupt starvation at high baud rates.

s/starvation/storm/ ?

> 
> Increase receiver trigger level to 8 to increase the minimum period
> between RX interrupts to 8 characters time. The tradeoff is increased
> latency. In the worst case scenario, where RX data has intercharacter
> delay slightly less than aging timer (8 characters time), it can take
> up to 63 characters time for the interrupt to be raised since the
> reception of the first character.
> 
> Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
> ---
>  drivers/tty/serial/imx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 90f82e6c54e4..3c812c47ecc0 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1255,7 +1255,7 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport)
>  }
>  
>  #define TXTL_DEFAULT 2 /* reset default */
> -#define RXTL_DEFAULT 1 /* reset default */
> +#define RXTL_DEFAULT 8 /* 8 characters or aging timer */
>  #define TXTL_DMA 8 /* DMA burst setting */
>  #define RXTL_DMA 9 /* DMA burst setting */
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Greg Kroah-Hartman]

On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
> Triggering RX interrupt for every byte defeats the purpose of aging
> timer and leads to interrupt starvation at high baud rates.
> 
> Increase receiver trigger level to 8 to increase the minimum period
> between RX interrupts to 8 characters time. The tradeoff is increased
> latency. In the worst case scenario, where RX data has intercharacter
> delay slightly less than aging timer (8 characters time), it can take
> up to 63 characters time for the interrupt to be raised since the
> reception of the first character.

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?

thanks,

greg k-h

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

[Tomasz Moń]

On 04.01.2022 11:43, Ahmad Fatoum wrote:
> On 04.01.22 11:32, Tomasz Moń wrote:
>> Triggering RX interrupt for every byte defeats the purpose of aging
>> timer and leads to interrupt starvation at high baud rates.
> 
> s/starvation/storm/ ?

Yes, absolutely. While it starves the system, the proper term here is
interrupt storm. Thank you for pointing this out!

Tomasz Mon

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

[Tomasz Moń]

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?

Thanks,
Tomasz Mon

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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tomasz Moń]

On 04.01.2022 23:49, Uwe Kleine-König wrote:
> 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.

While the increased latency due to this change is undeniable, it is
important to note that latency is not everything. There are applications
where the latency is crucial, however using Linux for such applications
is questionable. Linux is not a Real Time Operating System after all.

Latency is simple to measure and argue based on the reception of single
character. That is only a corner case, not fully describing real world.
In the real world, it is important to consider the overall performance
improvement. It is hard to determine how much does the performance of
the system improve thanks to less time spent in interrupt handling.

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

The more I think about this, the dynamic part feels less reasonable,
i.e. all the extra complexity needed seems enormous compared to the
potential advantage.

I have hacked in a GPIO toggle at the end of interrupt handling. This
allows me to measure the latency increase with the oscilloscope in the
simple one character reception case. Latency here is defined as "time
between the stop bit and GPIO toggle". Note that this test completely
ignores the user space impact (process scheduling).

With RXTL being set to 1 (aging timer effectively disabled), latency is
within 10 to 20 us range. This time does not depend on the baud rate,
but on the system, i.e. cpu frequency, memory layout, caching, compiler
optimization level, other interrupts and so on.

With RXTL being set to 8, baud rate set to 1M and 10 bits per character
(8N1), latency is within 90 to 100 us range. The shift by approximately
80 us matches the expectations as it is directly induced by aging timer
(interrupt was raised only after aging timer expired).

When DMA is enabled, baud rate set to 1M and 10 bits per character
(8N1), latency is within 100 to 110 us range. This is somewhat expected
as the main latency contributing factor is the aging timer. The extra
few us is likely due to the DMA overhead (not really important in real
world where more than one byte is being transferred).

Usually serial transmission happens in various length bursts (frames).
For such transmissions the latency induced by the aging timer is
generally equal to 8 characters time (when the frame length is exact
multiple of RXTL there is no latency induced by the aging timer).

Worst case scenario is when the intercharacter interval is slighly less
than the aging timer timeout. While this is possible, in my opinion it
is highly unlikely. Aging timer latency upper bound, mentioned in commit
message, is equal to:
  (RXTL - 1) * (8 character time timeout + received 1 character time)

The real advantage of higher RXTL is avoiding interrupt storm. With RXTL
being set to 1, I am able to trigger interrupt storm at 1M and higher
baud rates when intercharacter interval is virtually zero. This is
expected because, as noted earlier, interrupt handling time is within 10
to 20 us range. As the interrupt handling time is more than the time it
takes to transmit single character (10 us), this inevitably leads to
line discipline workqueue starvation, and in turn to dropping the
characters, because eventually:
  * tty flip buffer cannot take any more characters,
  * tty throttle does not happen due to interrupt storm,
  * RX interrupt is fast enough to keep RxFIFO below autoRTS level.

With RXTL being set to 8, minimum RX interrupt period is 8 characters
time. This happens when there is continuous data being received (no
delay due to the aging timer).

If changing the default RXTL value does not sound right, then maybe RXTL
could be configured via a device tree property? That way it would be up
to the user to tune it for the application.

Best Regards,
Tomasz Mon

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

[Greg Kroah-Hartman]

On Wed, Jan 05, 2022 at 08:59:09AM +0100, Tomasz Moń wrote:
> On 04.01.2022 23:49, Uwe Kleine-König wrote:
> > 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.
> 
> While the increased latency due to this change is undeniable, it is
> important to note that latency is not everything. There are applications
> where the latency is crucial, however using Linux for such applications
> is questionable. Linux is not a Real Time Operating System after all.

Yes, Linux can be used in real time situtations just fine, look at the
RT patchset for proof of that.

So let's not make things any worse for no good reason if at all
possible.

> Latency is simple to measure and argue based on the reception of single
> character. That is only a corner case, not fully describing real world.
> In the real world, it is important to consider the overall performance
> improvement. It is hard to determine how much does the performance of
> the system improve thanks to less time spent in interrupt handling.

If this can't be measured at all, why make this change in the first
place?

> If changing the default RXTL value does not sound right, then maybe RXTL
> could be configured via a device tree property? That way it would be up
> to the user to tune it for the application.

Device tree is not there to tune for applications :)

thanks,

greg k-h

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

[Tomasz Moń]

On 05.01.2022 11:37, Greg Kroah-Hartman wrote:
> On Wed, Jan 05, 2022 at 08:59:09AM +0100, Tomasz Moń wrote:
>> While the increased latency due to this change is undeniable, it is
>> important to note that latency is not everything. There are applications
>> where the latency is crucial, however using Linux for such applications
>> is questionable. Linux is not a Real Time Operating System after all.
> 
> Yes, Linux can be used in real time situtations just fine, look at the
> RT patchset for proof of that.
> 
> So let's not make things any worse for no good reason if at all
> possible.

Avoiding interrupt storm is a good reason for me.

>> Latency is simple to measure and argue based on the reception of single
>> character. That is only a corner case, not fully describing real world.
>> In the real world, it is important to consider the overall performance
>> improvement. It is hard to determine how much does the performance of
>> the system improve thanks to less time spent in interrupt handling.
> 
> If this can't be measured at all, why make this change in the first
> place?

It can be measured, just in general case, it is hard to tell exactly how
much does it help due to the complexity. All the time saved by not
wasting it in interrupt handler can have impact on many parts of the system.

However, there is a use case where it is easy to tell how much does this
change improve performance. In the situation where the interrupt storm
occurs with current mainline kernel (RXTL is set to 1) essentially
freezing everything until the sender stops transmitting, the patched
kernel (with RXTL is set to 8) handles the situation just fine. Change
from completely unresponsive to just fine is easy to notice.

Best Regards,
Tomasz Mon

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

On 05.01.2022 11:37:05, Greg Kroah-Hartman wrote:
> On Wed, Jan 05, 2022 at 08:59:09AM +0100, Tomasz Moń wrote:
> > On 04.01.2022 23:49, Uwe Kleine-König wrote:
> > > 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.
> > 
> > While the increased latency due to this change is undeniable, it is
> > important to note that latency is not everything. There are applications
> > where the latency is crucial, however using Linux for such applications
> > is questionable. Linux is not a Real Time Operating System after all.
> 
> Yes, Linux can be used in real time situtations just fine, look at the
> RT patchset for proof of that.
> 
> So let's not make things any worse for no good reason if at all
> possible.

+1

We have a $CUSTOMER, where serial latency is crucial. And we have a task
to cut down latencies and jitter even more.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Sergey Organov]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
>> Triggering RX interrupt for every byte defeats the purpose of aging
>> timer and leads to interrupt starvation at high baud rates.
>> 
>> Increase receiver trigger level to 8 to increase the minimum period
>> between RX interrupts to 8 characters time. The tradeoff is increased
>> latency. In the worst case scenario, where RX data has intercharacter
>> delay slightly less than aging timer (8 characters time), it can take
>> up to 63 characters time for the interrupt to be raised since the
>> reception of the first character.
>
> 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?

I don't like the idea of dynamic threshold as I don't think increased
complexity is worth it.

In fact the threshold works "properly" on any baud rate, as maximum
latency is proportional to the current baud rate, and if somebody does
care about *absolute* latency, increasing baud rate is the primary
solution. At most I'd think about some ioctl() to manually tweak the
threshold, for some exotic applications.

That said, are there examples in the kernel where this threshold is
dynamic? If not, then it's another argument against introducing it, and
if yes, then the OP will have reference implementation of exact
dependency curve.

Thanks,
-- Sergey Organov

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On 05.01.2022 16:00:34, Sergey Organov wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
> >> Triggering RX interrupt for every byte defeats the purpose of aging
> >> timer and leads to interrupt starvation at high baud rates.
> >> 
> >> Increase receiver trigger level to 8 to increase the minimum period
> >> between RX interrupts to 8 characters time. The tradeoff is increased
> >> latency. In the worst case scenario, where RX data has intercharacter
> >> delay slightly less than aging timer (8 characters time), it can take
> >> up to 63 characters time for the interrupt to be raised since the
> >> reception of the first character.
> >
> > 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?
> 
> I don't like the idea of dynamic threshold as I don't think increased
> complexity is worth it.
> 
> In fact the threshold works "properly" on any baud rate, as maximum
> latency is proportional to the current baud rate, and if somebody does
> care about *absolute* latency, increasing baud rate is the primary
> solution.

Nope - this only works if you have both sides under control.... Which is
not the case in our $CUSTROMER's use case.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Sergey Organov]

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

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

[Sergey Organov]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 05.01.2022 16:00:34, Sergey Organov wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
>> >> Triggering RX interrupt for every byte defeats the purpose of aging
>> >> timer and leads to interrupt starvation at high baud rates.
>> >> 
>> >> Increase receiver trigger level to 8 to increase the minimum period
>> >> between RX interrupts to 8 characters time. The tradeoff is increased
>> >> latency. In the worst case scenario, where RX data has intercharacter
>> >> delay slightly less than aging timer (8 characters time), it can take
>> >> up to 63 characters time for the interrupt to be raised since the
>> >> reception of the first character.
>> >
>> > 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?
>> 
>> I don't like the idea of dynamic threshold as I don't think increased
>> complexity is worth it.
>> 
>> In fact the threshold works "properly" on any baud rate, as maximum
>> latency is proportional to the current baud rate, and if somebody does
>> care about *absolute* latency, increasing baud rate is the primary
>> solution.
>
> Nope - this only works if you have both sides under control.... Which is
> not the case in our $CUSTROMER's use case.

Yep, if one can't use primary solution, they need to come up with
something else. Where is that historical "low-latency" bit, by the way?

Thanks,
-- Sergey Organov

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On 05.01.2022 16:37:22, Sergey Organov wrote:
> Marc Kleine-Budde <mkl@pengutronix.de> writes:
> 
> > On 05.01.2022 16:00:34, Sergey Organov wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> > On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
> >> >> Triggering RX interrupt for every byte defeats the purpose of aging
> >> >> timer and leads to interrupt starvation at high baud rates.
> >> >> 
> >> >> Increase receiver trigger level to 8 to increase the minimum period
> >> >> between RX interrupts to 8 characters time. The tradeoff is increased
> >> >> latency. In the worst case scenario, where RX data has intercharacter
> >> >> delay slightly less than aging timer (8 characters time), it can take
> >> >> up to 63 characters time for the interrupt to be raised since the
> >> >> reception of the first character.
> >> >
> >> > 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?
> >> 
> >> I don't like the idea of dynamic threshold as I don't think increased
> >> complexity is worth it.
> >> 
> >> In fact the threshold works "properly" on any baud rate, as maximum
> >> latency is proportional to the current baud rate, and if somebody does
> >> care about *absolute* latency, increasing baud rate is the primary
> >> solution.
> >
> > Nope - this only works if you have both sides under control.... Which is
> > not the case in our $CUSTROMER's use case.
> 
> Yep, if one can't use primary solution, they need to come up with
> something else.

Please don't break existing use cases while improving the kernel.

> Where is that historical "low-latency" bit, by the
> way?

...has been removed:

https://lore.kernel.org/all/20210105120239.28031-11-jslaby@suse.cz/

Is there an option to bring that back?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Sergey Organov]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 05.01.2022 16:37:22, Sergey Organov wrote:
>> Marc Kleine-Budde <mkl@pengutronix.de> writes:
>> 
>> > On 05.01.2022 16:00:34, Sergey Organov wrote:
>> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> >> 
>> >> > On Tue, Jan 04, 2022 at 11:32:03AM +0100, Tomasz Moń wrote:
>> >> >> Triggering RX interrupt for every byte defeats the purpose of aging
>> >> >> timer and leads to interrupt starvation at high baud rates.
>> >> >> 
>> >> >> Increase receiver trigger level to 8 to increase the minimum period
>> >> >> between RX interrupts to 8 characters time. The tradeoff is increased
>> >> >> latency. In the worst case scenario, where RX data has intercharacter
>> >> >> delay slightly less than aging timer (8 characters time), it can take
>> >> >> up to 63 characters time for the interrupt to be raised since the
>> >> >> reception of the first character.
>> >> >
>> >> > 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?
>> >> 
>> >> I don't like the idea of dynamic threshold as I don't think increased
>> >> complexity is worth it.
>> >> 
>> >> In fact the threshold works "properly" on any baud rate, as maximum
>> >> latency is proportional to the current baud rate, and if somebody does
>> >> care about *absolute* latency, increasing baud rate is the primary
>> >> solution.
>> >
>> > Nope - this only works if you have both sides under control.... Which is
>> > not the case in our $CUSTROMER's use case.
>> 
>> Yep, if one can't use primary solution, they need to come up with
>> something else.
>
> Please don't break existing use cases while improving the kernel.

If we aim at strict backward compatibility, the default value of water
level mark should not be changed, and I'm afraid it can't be changed at
any baud rate that is currently supported, as we can't be sure nobody
uses that feature of being "low latency" at any given baud rate.

>
>> Where is that historical "low-latency" bit, by the
>> way?
>
> ...has been removed:
>
> https://lore.kernel.org/all/20210105120239.28031-11-jslaby@suse.cz/
>
> Is there an option to bring that back?

It had different meaning as far as I recall, but as a way out of current
situation, I think something similar could be introduced as an
additional bit for tty parameters structure that is "true" by default,
meaning "minimize latency", and can be set to "false" through ioctl().

Alternatively, we might want to introduce "threshold baud rate"
parameter for tty, for drivers to be free to set high watermarks above
that baud rate.

Thanks,
-- Sergey Organov


>
> Marc

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

On Wed, Jan 05, 2022 at 04:34:21PM +0300, Sergey Organov wrote:
> 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.....

It could be as easy as increasing the waterlevel by one if an RX irq
happens with USR1.AGTIM = 0 and reset to 1 if USR1.AGTIM = 1.

This makes sure that receiving at a low frequency makes the hardware
interrupt the CPU early, and a burst doesn't starve the CPU.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Sergey Organov]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 05.01.2022 11:37:05, Greg Kroah-Hartman wrote:
>> On Wed, Jan 05, 2022 at 08:59:09AM +0100, Tomasz Moń wrote:
>> > On 04.01.2022 23:49, Uwe Kleine-König wrote:
>> > > 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.
>> > 
>> > While the increased latency due to this change is undeniable, it is
>> > important to note that latency is not everything. There are applications
>> > where the latency is crucial, however using Linux for such applications
>> > is questionable. Linux is not a Real Time Operating System after all.
>> 
>> Yes, Linux can be used in real time situtations just fine, look at the
>> RT patchset for proof of that.

Do you mean CONFIG_PREEMPT_RT patches? If so, you probably miss the
point. These patches, among other things, convert interrupt handlers
into preemptible kernel threads, that in turn may even result in
increase of latency of getting characters from RS232 as seen by user
applications.

These patches rather allow to write specific kernel IRQ handlers that
will have low latencies, too meet RT requirements.

Anyway, we are discussing this in the context of plain mainstream
kernel, and when latency vs throughput compromise is to be considered,
I'd expect throughput to win, not latency.

>> 
>> So let's not make things any worse for no good reason if at all
>> possible.
>
> +1
>
> We have a $CUSTOMER, where serial latency is crucial. And we have a task
> to cut down latencies and jitter even more.

Provided plain Linux kernel is far from being RT, it seems that to meet
the requirements you'd need to patch the kernel anyway. Like, say, apply
RT-patches and then turn this particular ISR back from threads to kernel
level. In which case adding one more tweak of setting the water mark
back to 1 is not a big deal, right?

Thanks,
-- Sergey Organov

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

[Tomasz Moń]

On 06.01.2022 16:05, Uwe Kleine-König wrote:
> On Wed, Jan 05, 2022 at 04:34:21PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>>> 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.....
> 
> It could be as easy as increasing the waterlevel by one if an RX irq
> happens with USR1.AGTIM = 0 and reset to 1 if USR1.AGTIM = 1.

This is indeed simple, but I fail to see the actual benefit of doing so.

> This makes sure that receiving at a low frequency makes the hardware
> interrupt the CPU early, and a burst doesn't starve the CPU.

If the communication involves multiple characters sent in burst (frame),
and includes some sort of CRC check (so only complete frames are useful,
as no partial processing can be done because checksum has to be checked
first), then it is the latency after the last character (not the first)
that matters.

RXTL upper value has to be capped. If RXTL goes above CTSTL then it
would limit throughput if hardware flow control is enabled. If hardware
flow control is not enabled, then the higher RXTL gets, the higher is
the risk of not reading RxFIFO in time. Setting RXTL to quarter of the
RxFIFO (8) seems to leave enough time for RX interrupt and at the same
time noticeably lowers the interrupt rate.

Best Regards,
Tomasz Mon

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]

On Mon, Jan 10, 2022 at 07:14:58AM +0100, Tomasz Moń wrote:
> On 06.01.2022 16:05, Uwe Kleine-König wrote:
> > On Wed, Jan 05, 2022 at 04:34:21PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >>> 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.....
> > 
> > It could be as easy as increasing the waterlevel by one if an RX irq
> > happens with USR1.AGTIM = 0 and reset to 1 if USR1.AGTIM = 1.
> 
> This is indeed simple, but I fail to see the actual benefit of doing so.
> 
> > This makes sure that receiving at a low frequency makes the hardware
> > interrupt the CPU early, and a burst doesn't starve the CPU.
> 
> If the communication involves multiple characters sent in burst (frame),
> and includes some sort of CRC check (so only complete frames are useful,
> as no partial processing can be done because checksum has to be checked
> first), then it is the latency after the last character (not the first)
> that matters.

Agreed, and if the setup is different it might be better if the
configured waterlevel is lower.

> RXTL upper value has to be capped. If RXTL goes above CTSTL then it
> would limit throughput if hardware flow control is enabled. If hardware
> flow control is not enabled, then the higher RXTL gets, the higher is
> the risk of not reading RxFIFO in time. Setting RXTL to quarter of the
> RxFIFO (8) seems to leave enough time for RX interrupt and at the same
> time noticeably lowers the interrupt rate.

I thought a bit about different usecases and the latency penalty for
increasing RXTL from 1 to 8 is at most 8 character frames (i.e. the time
until the aging irq triggers) which happens with a probability of 7/8.
For fast transfers it's a bit less. For a smaller RXTL the penalty is
still 8 character frames but the probablity is a bit lower.
(RXTL - 1 / RXTL, i.e. 0 in the status quo with RXTL = 1)

So the dynamic approach is only a tad better on avarage and the worst
case latency is the same. (Assuming a random message length.)

The worst case is always: There are between 1 and RXTL - 1 chars sitting
in the FIFO and waiting for 8 character frames until they are received.

So I agree to just increasing the RXTL value to 8, and if there is a
problem with that, we can still consider the dynamic approach then.

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]