[PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

In case of PREEMPT_RT or when low_latency flag is set by the serial driver
the TTY receive flip buffer is copied to the line discipline directly
instead of using a work queue in the background. Therefor only in case a
workqueue is actually used for copying data to the line discipline
we'll have to check & wait for the workqueue to finish.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 drivers/tty/tty_buffer.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5cfa548..b8e90b8 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -492,7 +492,16 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	/*
+	 * Only in case a workqueue is actually used for copying data to the
+	 * line discipline, we'll have to wait for the workqueue to finish. In
+	 * other cases this prevents us from unnecessary blocking by the
+	 * workqueue spin lock.
+	 */
+#ifndef CONFIG_PREEMPT_RT_FULL
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
+#endif
 }
 
 /**
-- 
1.7.9.5

[PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

In case of PREEMPT_RT or when low_latency flag is set by the serial driver
the TTY receive flip buffer is copied to the line discipline directly
instead of using a work queue in the background. Therefor only in case a
workqueue is actually used for copying data to the line discipline
we'll have to check & wait for the workqueue to finish.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 drivers/tty/tty_buffer.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5cfa548..b8e90b8 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -492,7 +492,16 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	/*
+	 * Only in case a workqueue is actually used for copying data to the
+	 * line discipline, we'll have to wait for the workqueue to finish. In
+	 * other cases this prevents us from unnecessary blocking by the
+	 * workqueue spin lock.
+	 */
+#ifndef CONFIG_PREEMPT_RT_FULL
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
+#endif
 }
 
 /**
-- 
1.7.9.5

Re: [PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> This prevents unnecessary spin lock/unlock on the workqueue spin lock
> that can cause additional scheduling overhead on a PREEMPT_RT system.
> On a 240 MHz AT91SAM9261 processor setup this fixes about 100us of
> scheduling overhead on the TTY read call.

This seems reasonable, but given its also relevant for upstream it
would be nice to get it without the ifdef in upstream.

The corner case is when the tty->low_latency flag is flipped but the
drivers should handle that gracefully and if not we should fix them so
you can get your 100uS.

Alan

Re: [PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> This prevents unnecessary spin lock/unlock on the workqueue spin lock
> that can cause additional scheduling overhead on a PREEMPT_RT system.
> On a 240 MHz AT91SAM9261 processor setup this fixes about 100us of
> scheduling overhead on the TTY read call.

This seems reasonable, but given its also relevant for upstream it
would be nice to get it without the ifdef in upstream.

The corner case is when the tty->low_latency flag is flipped but the
drivers should handle that gracefully and if not we should fix them so
you can get your 100uS.

Alan

Re: [PATCH] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

Hi,

2012/9/20 Alan Cox <alan@linux.intel.com>:
>
> This seems reasonable, but given its also relevant for upstream it
> would be nice to get it without the ifdef in upstream.
>
> The corner case is when the tty->low_latency flag is flipped but the
> drivers should handle that gracefully and if not we should fix them so
> you can get your 100uS.
>
> Alan

Actually this patch is related to my previous patch "tty: cleanup
duplicate functions in tty_buffer". Some drivers call the
tty_schedule_flip() function instead of tty_flip_buffer_push(). In
that case the work queue is always used, regardless the
tty->low_latency flag. That's why tried to get rid of the
tty_schedule_flip() function.

But from the first review remarks on the "tty: cleanup duplicate
functions in tty_buffer" patch, that patch seems to be invalid. So in
that case this patch is invalid also.

Regards,
Ivo Sieben

[PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

In case of PREEMPT_RT or when low_latency flag is set by the serial driver
the TTY receive flip buffer is copied to the line discipline directly
instead of using a work queue in the background. Therefor only in case a
workqueue is actually used for copying data to the line discipline
we'll have to check & wait for the workqueue to finish.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---

 v2:
 Patch v1 was based on the fact that only the tty_flip_buffer_push() function
 was used to copy dat to the line discipline because I removed the
 tty_schedule_flip() in my previous patch "[PATCH] tty: cleanup duplicate
 functions in tty_buffer". Since that patched turned out to be invalid, I had
 to implement this functionality differently.

 drivers/tty/tty_buffer.c |   17 ++++++++++++++---
 include/linux/tty.h      |    3 ++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..dee77b4 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -354,6 +354,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	schedule_work(&tty->buf.work);
+	atomic_set(&tty->buf.flush_work, 1);
 }
 EXPORT_SYMBOL(tty_schedule_flip);
 
@@ -514,7 +515,14 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	/*
+	 * The work queue is not always used to move data from the flip buffer
+	 * to the line discipline: the tty_flip_buffer_push() will call the
+	 * flush_to_ldisc() routine directly when low_latency flag is set.
+	 * Therefor only flush the work queue when required.
+	 */
+	if (atomic_xchg(&tty->buf.flush_work, 0))
+		flush_work(&tty->buf.work);
 }
 
 /**
@@ -538,10 +546,12 @@ void tty_flip_buffer_push(struct tty_struct *tty)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	raw_spin_unlock_irqrestore(&tty->buf.lock, flags);
 
-	if (tty->low_latency)
+	if (tty->low_latency) {
 		flush_to_ldisc(&tty->buf.work);
-	else
+	} else {
 		schedule_work(&tty->buf.work);
+		atomic_set(&tty->buf.flush_work, 1);
+	}
 }
 EXPORT_SYMBOL(tty_flip_buffer_push);
 
@@ -563,5 +573,6 @@ void tty_buffer_init(struct tty_struct *tty)
 	tty->buf.free = NULL;
 	tty->buf.memory_used = 0;
 	INIT_WORK(&tty->buf.work, flush_to_ldisc);
+	atomic_set(&tty->buf.flush_work, 0);
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 21bceef..f76ac5e 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -43,7 +43,7 @@
 #include <linux/tty_driver.h>
 #include <linux/tty_ldisc.h>
 #include <linux/mutex.h>
-
+#include <linux/atomic.h>
 
 
 /*
@@ -86,6 +86,7 @@ struct tty_buffer {
 
 struct tty_bufhead {
 	struct work_struct work;
+	atomic_t flush_work;
 	raw_spinlock_t lock;
 	struct tty_buffer *head;	/* Queue head */
 	struct tty_buffer *tail;	/* Active buffer */
-- 
1.7.9.5

[PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

In case of PREEMPT_RT or when low_latency flag is set by the serial driver
the TTY receive flip buffer is copied to the line discipline directly
instead of using a work queue in the background. Therefor only in case a
workqueue is actually used for copying data to the line discipline
we'll have to check & wait for the workqueue to finish.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---

 v2:
 Patch v1 was based on the fact that only the tty_flip_buffer_push() function
 was used to copy dat to the line discipline because I removed the
 tty_schedule_flip() in my previous patch "[PATCH] tty: cleanup duplicate
 functions in tty_buffer". Since that patched turned out to be invalid, I had
 to implement this functionality differently.

 drivers/tty/tty_buffer.c |   17 ++++++++++++++---
 include/linux/tty.h      |    3 ++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..dee77b4 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -354,6 +354,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	schedule_work(&tty->buf.work);
+	atomic_set(&tty->buf.flush_work, 1);
 }
 EXPORT_SYMBOL(tty_schedule_flip);
 
@@ -514,7 +515,14 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	/*
+	 * The work queue is not always used to move data from the flip buffer
+	 * to the line discipline: the tty_flip_buffer_push() will call the
+	 * flush_to_ldisc() routine directly when low_latency flag is set.
+	 * Therefor only flush the work queue when required.
+	 */
+	if (atomic_xchg(&tty->buf.flush_work, 0))
+		flush_work(&tty->buf.work);
 }
 
 /**
@@ -538,10 +546,12 @@ void tty_flip_buffer_push(struct tty_struct *tty)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	raw_spin_unlock_irqrestore(&tty->buf.lock, flags);
 
-	if (tty->low_latency)
+	if (tty->low_latency) {
 		flush_to_ldisc(&tty->buf.work);
-	else
+	} else {
 		schedule_work(&tty->buf.work);
+		atomic_set(&tty->buf.flush_work, 1);
+	}
 }
 EXPORT_SYMBOL(tty_flip_buffer_push);
 
@@ -563,5 +573,6 @@ void tty_buffer_init(struct tty_struct *tty)
 	tty->buf.free = NULL;
 	tty->buf.memory_used = 0;
 	INIT_WORK(&tty->buf.work, flush_to_ldisc);
+	atomic_set(&tty->buf.flush_work, 0);
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 21bceef..f76ac5e 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -43,7 +43,7 @@
 #include <linux/tty_driver.h>
 #include <linux/tty_ldisc.h>
 #include <linux/mutex.h>
-
+#include <linux/atomic.h>
 
 
 /*
@@ -86,6 +86,7 @@ struct tty_buffer {
 
 struct tty_bufhead {
 	struct work_struct work;
+	atomic_t flush_work;
 	raw_spinlock_t lock;
 	struct tty_buffer *head;	/* Queue head */
 	struct tty_buffer *tail;	/* Active buffer */
-- 
1.7.9.5

Re: [PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> instead of using a work queue in the background. Therefor only in case a
> workqueue is actually used for copying data to the line discipline
> we'll have to check & wait for the workqueue to finish.
> 
> This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
> cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
> AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
> TTY read call.

This seems incredibly convoluted and complicated as well as asking for
races and weirdness with the divisibility between the schedule_work and
the atomic operation.

If the only cases are:

	tty->low_latency = 0
				call flush_work
	tty->low_latency = 1
				don't call

	tty->low_latency changes
				call flush_work

then surely the right fix is

	if (tty->low_latency == 0)
		flush_work(&tty->buf.work);

and making the couple of spots we set/unset low latency on a running port
somewhat smarter ?

That avoids an expensive atomic operation as well (and atomic ops are
very expensive on some platforms).

Alan

Re: [PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> instead of using a work queue in the background. Therefor only in case a
> workqueue is actually used for copying data to the line discipline
> we'll have to check & wait for the workqueue to finish.
> 
> This prevents unnecessary spin lock/unlock on the workqueue spin lock that can
> cause additional scheduling overhead on a PREEMPT_RT system. On a 240 MHz
> AT91SAM9261 processor setup this fixes about 100us of scheduling overhead on the
> TTY read call.

This seems incredibly convoluted and complicated as well as asking for
races and weirdness with the divisibility between the schedule_work and
the atomic operation.

If the only cases are:

	tty->low_latency = 0
				call flush_work
	tty->low_latency = 1
				don't call

	tty->low_latency changes
				call flush_work

then surely the right fix is

	if (tty->low_latency == 0)
		flush_work(&tty->buf.work);

and making the couple of spots we set/unset low latency on a running port
somewhat smarter ?

That avoids an expensive atomic operation as well (and atomic ops are
very expensive on some platforms).

Alan

Re: [PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

Hi,

2012/9/25 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>
>
> then surely the right fix is
>
>         if (tty->low_latency == 0)
>                 flush_work(&tty->buf.work);
>

Your are right, that is indeed more straightforward & logical.
But what if a TTY driver uses the tty_schedule_flip() function instead
of tty_flip_buffer_push() and has low_latency set to 1?
In that case the work queue will never be flushed...

Or should drivers that use tty_schedule_flip() function never set the
low_latency flag? (a quick scan indeed shows that all drivers that use
the low_latency flag indeed make use of the tty_flip_buffer_push()
function).

Best regards,
Ivo Sieben

Re: [PATCH-v2] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> Or should drivers that use tty_schedule_flip() function never set the
> low_latency flag? (a quick scan indeed shows that all drivers that use
> the low_latency flag indeed make use of the tty_flip_buffer_push()
> function).

They should never do that - and adding a WARN_ON() will ensure we catch
anyone who offends.

I think thats worth doing as the resulting code is clearer and faster.

Alan

[PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

When low_latency flag is set the TTY receive flip buffer is copied to the
line discipline directly instead of using a work queue in the background.
Therefor only in case a workqueue is actually used for copying data to the
line discipline we'll have to flush the workqueue.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that
can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
overhead on the TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 v3:
 todo

 drivers/tty/tty_buffer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..b952de1 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -342,6 +342,8 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
  *	Takes any pending buffers and transfers their ownership to the
  *	ldisc side of the queue. It then schedules those characters for
  *	processing by the line discipline.
+ *	Note that this function can only be used when the low_latency flag
+ *	is unset. Otherwise the workqueue won't be flushed.
  *
  *	Locking: Takes tty->buf.lock
  */
@@ -349,6 +351,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
 void tty_schedule_flip(struct tty_struct *tty)
 {
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 	spin_lock_irqsave(&tty->buf.lock, flags);
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
@@ -514,7 +517,8 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
 }
 
 /**
-- 
1.7.9.5

[PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

When low_latency flag is set the TTY receive flip buffer is copied to the
line discipline directly instead of using a work queue in the background.
Therefor only in case a workqueue is actually used for copying data to the
line discipline we'll have to flush the workqueue.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that
can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
overhead on the TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 v3:
 todo

 drivers/tty/tty_buffer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..b952de1 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -342,6 +342,8 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
  *	Takes any pending buffers and transfers their ownership to the
  *	ldisc side of the queue. It then schedules those characters for
  *	processing by the line discipline.
+ *	Note that this function can only be used when the low_latency flag
+ *	is unset. Otherwise the workqueue won't be flushed.
  *
  *	Locking: Takes tty->buf.lock
  */
@@ -349,6 +351,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
 void tty_schedule_flip(struct tty_struct *tty)
 {
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 	spin_lock_irqsave(&tty->buf.lock, flags);
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
@@ -514,7 +517,8 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
 }
 
 /**
-- 
1.7.9.5

[PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

When low_latency flag is set the TTY receive flip buffer is copied to the
line discipline directly instead of using a work queue in the background.
Therefor only in case a workqueue is actually used for copying data to the
line discipline we'll have to flush the workqueue.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that
can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
overhead on the TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 v3:
 Prevented the flush the easy way: workque is not flused when low latency
 flag is set. This means that drivers that use the tty_schedule_flip()
 functions instead of the tty_flip_buffer_push() should never have the
 low_latency flag set, otherwise the workqueue won't be flushed. Added a
 WARN_ON() to detect those drivers.

 For PREEMPT_RT systems: this optimization will only work when the user has
 actually enabled the ASYNC_LOW_LATENCY option for the serial driver, otherwise
 the workqueue will still be flushed although it is not used since the PREMPT_RT
 patch makes the tty_flip_buffer_push() function to always bypass the workqueue.

 drivers/tty/tty_buffer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..b952de1 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -342,6 +342,8 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
  *	Takes any pending buffers and transfers their ownership to the
  *	ldisc side of the queue. It then schedules those characters for
  *	processing by the line discipline.
+ *	Note that this function can only be used when the low_latency flag
+ *	is unset. Otherwise the workqueue won't be flushed.
  *
  *	Locking: Takes tty->buf.lock
  */
@@ -349,6 +351,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
 void tty_schedule_flip(struct tty_struct *tty)
 {
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 	spin_lock_irqsave(&tty->buf.lock, flags);
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
@@ -514,7 +517,8 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
 }
 
 /**
-- 
1.7.9.5

[PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Ivo Sieben]

When low_latency flag is set the TTY receive flip buffer is copied to the
line discipline directly instead of using a work queue in the background.
Therefor only in case a workqueue is actually used for copying data to the
line discipline we'll have to flush the workqueue.

This prevents unnecessary spin lock/unlock on the workqueue spin lock that
can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
overhead on the TTY read call.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 v3:
 Prevented the flush the easy way: workque is not flused when low latency
 flag is set. This means that drivers that use the tty_schedule_flip()
 functions instead of the tty_flip_buffer_push() should never have the
 low_latency flag set, otherwise the workqueue won't be flushed. Added a
 WARN_ON() to detect those drivers.

 For PREEMPT_RT systems: this optimization will only work when the user has
 actually enabled the ASYNC_LOW_LATENCY option for the serial driver, otherwise
 the workqueue will still be flushed although it is not used since the PREMPT_RT
 patch makes the tty_flip_buffer_push() function to always bypass the workqueue.

 drivers/tty/tty_buffer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6146e8b..b952de1 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -342,6 +342,8 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
  *	Takes any pending buffers and transfers their ownership to the
  *	ldisc side of the queue. It then schedules those characters for
  *	processing by the line discipline.
+ *	Note that this function can only be used when the low_latency flag
+ *	is unset. Otherwise the workqueue won't be flushed.
  *
  *	Locking: Takes tty->buf.lock
  */
@@ -349,6 +351,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
 void tty_schedule_flip(struct tty_struct *tty)
 {
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 	spin_lock_irqsave(&tty->buf.lock, flags);
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
@@ -514,7 +517,8 @@ static void flush_to_ldisc(struct work_struct *work)
  */
 void tty_flush_to_ldisc(struct tty_struct *tty)
 {
-	flush_work(&tty->buf.work);
+	if (!tty->low_latency)
+		flush_work(&tty->buf.work);
 }
 
 /**
-- 
1.7.9.5

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

On Thu, 27 Sep 2012 13:58:21 +0200
Ivo Sieben <meltedpianoman@gmail.com> wrote:

> When low_latency flag is set the TTY receive flip buffer is copied to the
> line discipline directly instead of using a work queue in the background.
> Therefor only in case a workqueue is actually used for copying data to the
> line discipline we'll have to flush the workqueue.
> 
> This prevents unnecessary spin lock/unlock on the workqueue spin lock that
> can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
> MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
> overhead on the TTY read call.
> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

Acked-by: Alan Cox <alan@linux.intel.com>

(but for 3.8 not 3.7 ...)

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

On Thu, 27 Sep 2012 13:58:21 +0200
Ivo Sieben <meltedpianoman@gmail.com> wrote:

> When low_latency flag is set the TTY receive flip buffer is copied to the
> line discipline directly instead of using a work queue in the background.
> Therefor only in case a workqueue is actually used for copying data to the
> line discipline we'll have to flush the workqueue.
> 
> This prevents unnecessary spin lock/unlock on the workqueue spin lock that
> can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
> MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
> overhead on the TTY read call.
> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

Acked-by: Alan Cox <alan@linux.intel.com>

(but for 3.8 not 3.7 ...)

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Greg KH]

On Thu, Sep 27, 2012 at 02:15:22PM +0100, Alan Cox wrote:
> On Thu, 27 Sep 2012 13:58:21 +0200
> Ivo Sieben <meltedpianoman@gmail.com> wrote:
> 
> > When low_latency flag is set the TTY receive flip buffer is copied to the
> > line discipline directly instead of using a work queue in the background.
> > Therefor only in case a workqueue is actually used for copying data to the
> > line discipline we'll have to flush the workqueue.
> > 
> > This prevents unnecessary spin lock/unlock on the workqueue spin lock that
> > can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
> > MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
> > overhead on the TTY read call.
> > 
> > Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
> 
> Acked-by: Alan Cox <alan@linux.intel.com>
> 
> (but for 3.8 not 3.7 ...)

Yes, I will hold onto this until 3.7-rc1 is out, thanks.

greg k-h

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Greg KH]

On Thu, Sep 27, 2012 at 02:02:05PM +0200, Ivo Sieben wrote:
> When low_latency flag is set the TTY receive flip buffer is copied to the
> line discipline directly instead of using a work queue in the background.
> Therefor only in case a workqueue is actually used for copying data to the
> line discipline we'll have to flush the workqueue.
> 
> This prevents unnecessary spin lock/unlock on the workqueue spin lock that
> can cause additional scheduling overhead on a PREEMPT_RT system. On a 200
> MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
> overhead on the TTY read call.
> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

Note, I took out the WARN_ON() in this patch, as what is that really
going to help here?  It also will conflict with a patch from Jiri I'll
ba applying after this one, so if you think it's needed, care to send a
follow-on patch based on linux-next in a few days?

thanks,

greg k-h

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Alan Cox]

> Note, I took out the WARN_ON() in this patch, as what is that really
> going to help here?  It also will conflict with a patch from Jiri I'll
> ba applying after this one, so if you think it's needed, care to send
> a follow-on patch based on linux-next in a few days?

It catches anyone flipping low_latency wrongly or in other places. It's
fairly important as WARN_ON() goes !

[PATCH] [tty]: Report warning when low_latency flag is wrongly used

[Ivo Sieben]

When a driver has the low_latency flag set and uses the schedule_flip()
function to initiate copying data to the line discipline, a workqueue is
scheduled in but never actually flushed. This is incorrect use of the
low_latency flag (driver should not support the low_latency flag, or use
the tty_flip_buffer_push() function instead). Make sure a warning is
reported to catch incorrect use of the low_latency flag.

This patch goes with: cee4ad1ed90a0959fc29f9d30a2526e5e9522cfa

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

---
 drivers/tty/tty_buffer.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 06725f5..6cf87d7 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -365,6 +365,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 {
 	struct tty_bufhead *buf = &tty->port->buf;
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 
 	spin_lock_irqsave(&buf->lock, flags);
 	if (buf->tail != NULL)
-- 
1.7.9.5

[PATCH] [tty]: Report warning when low_latency flag is wrongly used

[Ivo Sieben]

When a driver has the low_latency flag set and uses the schedule_flip()
function to initiate copying data to the line discipline, a workqueue is
scheduled in but never actually flushed. This is incorrect use of the
low_latency flag (driver should not support the low_latency flag, or use
the tty_flip_buffer_push() function instead). Make sure a warning is
reported to catch incorrect use of the low_latency flag.

This patch goes with: cee4ad1ed90a0959fc29f9d30a2526e5e9522cfa

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

---
 drivers/tty/tty_buffer.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 06725f5..6cf87d7 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -365,6 +365,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 {
 	struct tty_bufhead *buf = &tty->port->buf;
 	unsigned long flags;
+	WARN_ON(tty->low_latency);
 
 	spin_lock_irqsave(&buf->lock, flags);
 	if (buf->tail != NULL)
-- 
1.7.9.5

Re: [PATCH] [tty]: Report warning when low_latency flag is wrongly used

[Paul Gortmaker]

On 12-10-24 08:35 AM, Ivo Sieben wrote:
> When a driver has the low_latency flag set and uses the schedule_flip()
> function to initiate copying data to the line discipline, a workqueue is
> scheduled in but never actually flushed. This is incorrect use of the
> low_latency flag (driver should not support the low_latency flag, or use
> the tty_flip_buffer_push() function instead). Make sure a warning is
> reported to catch incorrect use of the low_latency flag.
> 
> This patch goes with: cee4ad1ed90a0959fc29f9d30a2526e5e9522cfa

Ideally you shouldn't put commit IDs in the commit log when they
are not "permanent" (i.e. already part of mainline).  For example,
I have no idea what cee4ad1e contains, or what tree it lives in,
and I can't even search for it, since you've not also included
the short log.

Paul.
--

> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
> 
> ---
>  drivers/tty/tty_buffer.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 06725f5..6cf87d7 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -365,6 +365,7 @@ void tty_schedule_flip(struct tty_struct *tty)
>  {
>  	struct tty_bufhead *buf = &tty->port->buf;
>  	unsigned long flags;
> +	WARN_ON(tty->low_latency);
>  
>  	spin_lock_irqsave(&buf->lock, flags);
>  	if (buf->tail != NULL)
> 

Re: [PATCH] [tty]: Report warning when low_latency flag is wrongly used

[Paul Gortmaker]

On 12-10-24 08:35 AM, Ivo Sieben wrote:
> When a driver has the low_latency flag set and uses the schedule_flip()
> function to initiate copying data to the line discipline, a workqueue is
> scheduled in but never actually flushed. This is incorrect use of the
> low_latency flag (driver should not support the low_latency flag, or use
> the tty_flip_buffer_push() function instead). Make sure a warning is
> reported to catch incorrect use of the low_latency flag.
> 
> This patch goes with: cee4ad1ed90a0959fc29f9d30a2526e5e9522cfa

Ideally you shouldn't put commit IDs in the commit log when they
are not "permanent" (i.e. already part of mainline).  For example,
I have no idea what cee4ad1e contains, or what tree it lives in,
and I can't even search for it, since you've not also included
the short log.

Paul.
--

> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
> 
> ---
>  drivers/tty/tty_buffer.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 06725f5..6cf87d7 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -365,6 +365,7 @@ void tty_schedule_flip(struct tty_struct *tty)
>  {
>  	struct tty_bufhead *buf = &tty->port->buf;
>  	unsigned long flags;
> +	WARN_ON(tty->low_latency);
>  
>  	spin_lock_irqsave(&buf->lock, flags);
>  	if (buf->tail != NULL)
> 

Re: [PATCH-v3] tty: prevent unnecessary work queue lock checking on flip buffer copy

[Greg KH]

On Tue, Oct 23, 2012 at 11:16:59AM +0100, Alan Cox wrote:
> > Note, I took out the WARN_ON() in this patch, as what is that really
> > going to help here?  It also will conflict with a patch from Jiri I'll
> > ba applying after this one, so if you think it's needed, care to send
> > a follow-on patch based on linux-next in a few days?
> 
> It catches anyone flipping low_latency wrongly or in other places. It's
> fairly important as WARN_ON() goes !

Ok, I've applied the patch that Ivo sent doing this.

greg k-h