linux-kernel - Re: locking changes in tty broke low latency feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140219130308.GC1851@redhat.com>
Date:	Wed, 19 Feb 2014 14:03:09 +0100
From:	Stanislaw Gruszka <sgruszka@...hat.com>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-rt-users@...r.kernel.org,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Hal Murray <murray+fedora@...64-139-1-69.sjc.megapath.net>
Subject: Re: locking changes in tty broke low latency feature

Hello,

On Tue, Feb 18, 2014 at 05:12:13PM -0500, Peter Hurley wrote:
> On 02/18/2014 04:38 AM, Stanislaw Gruszka wrote:
> >Hi,
> >
> >setserial has low_latency option which should minimize receive latency
> >(scheduler delay). AFAICT it is used if someone talk to external device
> >via RS-485/RS-232 and need to have quick requests and responses . On
> >kernel this feature was implemented by direct tty processing from
> >interrupt context:
> >
> >void tty_flip_buffer_push(struct tty_port *port)
> >{
> >         struct tty_bufhead *buf = &port->buf;
> >
> >         buf->tail->commit = buf->tail->used;
> >
> >         if (port->low_latency)
> >                 flush_to_ldisc(&buf->work);
> >         else
> >                 schedule_work(&buf->work);
> >}
> >
> >But after 3.12 tty locking changes, calling flush_to_ldisc() from
> >interrupt context is a bug (we got scheduling while atomic bug report
> >here: https://bugzilla.redhat.com/show_bug.cgi?id=1065087 )
> >
> >I'm not sure how this should be solved. After Peter get rid all of those
> >race condition in tty layer, we probably don't want go back to use
> >spin_lock's there. Maybe we can create WQ_HIGHPRI workqueue and schedule
> >flush_to_ldisc() work there. Or perhaps users that need to low latency,
> >should switch to thread irq and prioritize serial irq to meat
> >retirements. Anyway setserial low_latency is now broken and all who use
> >this feature in the past can not do this any longer on 3.12+ kernels.
> >
> >Thoughts ?
> 
> Can you give me an idea of your device's average and minimum required
> latency (please be specific)?  Is your target arch x86 [so I can evaluate the
> the impact of bus-locked instructions relative to your expected]?
> 
> Also, how painful would it be if unsupported termios changes were rejected
> if the port was in low_latency mode and/or if low_latency setting was
> disallowed because of termios state?
> 
> It would be pointless to throttle low_latency, yes?
> 
> What would be an acceptable outcome of being unable to accept input?
> Corrupted overrun? Dropped i/o? Queued for later? Please explain with
> comparison to the outcome of missed minimum latency.

I'm sorry, I can not answer your questions.

For what I googled it looks like users wanted to get rid of 10ms jitter
caused by scheduler. Unfortunately that is all I know. I'm not user of
this feature and I don't know what are exact expectations. I'm adding Hal,
who reported bug to RH bugzilla, perhaps he can tell more about what
expectations are.

For now, since issue is not easy fixable, perhaps we should just fix
scheduling while atomic bug and add warning like on below patch ?

Thanks
Stanislaw

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 765125d..0fe6d65 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -504,9 +504,13 @@ void tty_flip_buffer_push(struct tty_port *port)
 
 	buf->tail->commit = buf->tail->used;
 
-	if (port->low_latency)
-		flush_to_ldisc(&buf->work);
-	else
+	if (port->low_latency) {
+		if (WARN_ONCE(in_irq() || irqs_disabled(),
+			      "serial low_latency feature can not be used in interrupt context"))
+			schedule_work(&buf->work);
+		else
+			flush_to_ldisc(&buf->work);
+	} else
 		schedule_work(&buf->work);
 }
 EXPORT_SYMBOL(tty_flip_buffer_push);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/