* [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3)
@ 2012-05-03 12:37 Ivo Sieben
2012-05-03 12:37 ` [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) Ivo Sieben
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben
Solved unnecessary schedule latency in the TTY layer when used in
realtime context:
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 workqueue in the background. Therefor 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. For a PREEMPT system this
prevents us from unnecessary blocking by the workqueue spin lock.
Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and
no interrupts (and therefor no scheduling) is disabled.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
drivers/tty/tty_buffer.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6c9b7cd..ed86359 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -317,12 +317,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
void tty_schedule_flip(struct tty_struct *tty)
{
- unsigned long flags;
- spin_lock_irqsave(&tty->buf.lock, flags);
- if (tty->buf.tail != NULL)
- tty->buf.tail->commit = tty->buf.tail->used;
- spin_unlock_irqrestore(&tty->buf.lock, flags);
- schedule_work(&tty->buf.work);
+ tty_flip_buffer_push(tty);
}
EXPORT_SYMBOL(tty_schedule_flip);
@@ -469,7 +464,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.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) 2012-05-03 12:37 [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben @ 2012-05-03 12:37 ` Ivo Sieben 2012-05-03 16:25 ` Greg KH 2012-05-03 12:37 ` [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) Ivo Sieben ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw) To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben Solved unnecessary schedule latency in the TTY layer when used in realtime context: The global "normal" spin lock that guards the line discipline administration is replaced by a raw spin lock. Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and no interrupts (and therefor no scheduling) is disabled. Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com> Conflicts: drivers/tty/tty_ldisc.c --- drivers/tty/tty_ldisc.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index 24b95db..30836cc 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -26,7 +26,7 @@ * callers who will do ldisc lookups and cannot sleep. */ -static DEFINE_SPINLOCK(tty_ldisc_lock); +static DEFINE_RAW_SPINLOCK(tty_ldisc_lock); static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait); static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle); /* Line disc dispatch table */ @@ -53,18 +53,18 @@ static void put_ldisc(struct tty_ldisc *ld) * We really want an "atomic_dec_and_lock_irqsave()", * but we don't have it, so this does it by hand. */ - local_irq_save(flags); - if (atomic_dec_and_lock(&ld->users, &tty_ldisc_lock)) { + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); + if (atomic_dec_and_test(&ld->users)) { struct tty_ldisc_ops *ldo = ld->ops; ldo->refcount--; module_put(ldo->owner); - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); kfree(ld); return; } - local_irq_restore(flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); wake_up(&tty_ldisc_idle); } @@ -89,11 +89,11 @@ int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc) if (disc < N_TTY || disc >= NR_LDISCS) return -EINVAL; - spin_lock_irqsave(&tty_ldisc_lock, flags); + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); tty_ldiscs[disc] = new_ldisc; new_ldisc->num = disc; new_ldisc->refcount = 0; - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); return ret; } @@ -119,12 +119,12 @@ int tty_unregister_ldisc(int disc) if (disc < N_TTY || disc >= NR_LDISCS) return -EINVAL; - spin_lock_irqsave(&tty_ldisc_lock, flags); + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); if (tty_ldiscs[disc]->refcount) ret = -EBUSY; else tty_ldiscs[disc] = NULL; - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); return ret; } @@ -135,7 +135,7 @@ static struct tty_ldisc_ops *get_ldops(int disc) unsigned long flags; struct tty_ldisc_ops *ldops, *ret; - spin_lock_irqsave(&tty_ldisc_lock, flags); + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); ret = ERR_PTR(-EINVAL); ldops = tty_ldiscs[disc]; if (ldops) { @@ -145,7 +145,7 @@ static struct tty_ldisc_ops *get_ldops(int disc) ret = ldops; } } - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); return ret; } @@ -153,10 +153,10 @@ static void put_ldops(struct tty_ldisc_ops *ldops) { unsigned long flags; - spin_lock_irqsave(&tty_ldisc_lock, flags); + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); ldops->refcount--; module_put(ldops->owner); - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); } /** @@ -286,11 +286,11 @@ static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty) unsigned long flags; struct tty_ldisc *ld; - spin_lock_irqsave(&tty_ldisc_lock, flags); + raw_spin_lock_irqsave(&tty_ldisc_lock, flags); ld = NULL; if (test_bit(TTY_LDISC, &tty->flags)) ld = get_ldisc(tty->ldisc); - spin_unlock_irqrestore(&tty_ldisc_lock, flags); + raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); return ld; } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) 2012-05-03 12:37 ` [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) Ivo Sieben @ 2012-05-03 16:25 ` Greg KH 2012-05-07 7:45 ` Ivo Sieben 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2012-05-03 16:25 UTC (permalink / raw) To: Ivo Sieben; +Cc: linux-serial, Alan Cox, RT On Thu, May 03, 2012 at 02:37:42PM +0200, Ivo Sieben wrote: > Solved unnecessary schedule latency in the TTY layer when used in > realtime context: > The global "normal" spin lock that guards the line discipline > administration is replaced by a raw spin lock. Why, what does this cause to have happen? What's the difference here that causes any speedups or latency fixes? thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) 2012-05-03 16:25 ` Greg KH @ 2012-05-07 7:45 ` Ivo Sieben 0 siblings, 0 replies; 11+ messages in thread From: Ivo Sieben @ 2012-05-07 7:45 UTC (permalink / raw) To: Greg KH; +Cc: linux-serial, Alan Cox, RT Hi, 2012/5/3 Greg KH <gregkh@linuxfoundation.org>: > > Why, what does this cause to have happen? What's the difference here > that causes any speedups or latency fixes? > This patch solves the following scenario: - A low priority process starts to send data to serial port A - In order to do this, the low priority process calls the tty_ldisc_ref_wait() function to retrieve a reference to its line discipline. - In the tty_ldisc_ref_wait() function, the global ldisc spinlock is locked. - While the spin lock is still locked, a context switch takes place and a second process on high real-time priority starts reading data from another serial port B (this is a possible scenario on a PREEMPT_RT system where a "normal" spin lock behaves like a mutex and no interrupts are actually disabled, although the "irqsave" postfix in the spinlock function name suggests otherwise) - The second project also calls the tty_ldisc_ref_wait() function, and therefor gets blocked on the global ldisc spin lock. - As a result priority inversion takes place, and the second process is scheduled out, the first process is scheduled in on high priority. After the first process has released the spin lock, the second process is scheduled again. The above scenario is not wrong, it works fine... but the scheduling & mutex handling requires a lot of extra processing time what makes that a normal read operation of about 50us sometimes can last up to 230us. By using raw spin locks this situation is prevented. I think it is legitimate to use raw spin locks because the critical sections in ldisc are small. Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) 2012-05-03 12:37 [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben 2012-05-03 12:37 ` [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) Ivo Sieben @ 2012-05-03 12:37 ` Ivo Sieben 2012-05-03 16:24 ` Greg KH 2012-05-10 15:28 ` Alan Cox 2012-05-07 14:10 ` [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben 2012-05-10 15:26 ` Alan Cox 3 siblings, 2 replies; 11+ messages in thread From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw) To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben Solved unnecessary schedule latency in the TTY layer when used in realtime context: The global wait_queue that is used for line discipline idle handling is moved to a separate wait_queue for each line instance. This prevents unnecessary blocking on one line, because of idle handling on another line. Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and no interrupts (and therefor no scheduling) is disabled. Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com> --- drivers/tty/tty_ldisc.c | 7 ++++--- include/linux/tty_ldisc.h | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index 30836cc..77eb299 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -28,7 +28,6 @@ static DEFINE_RAW_SPINLOCK(tty_ldisc_lock); static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait); -static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle); /* Line disc dispatch table */ static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS]; @@ -65,7 +64,7 @@ static void put_ldisc(struct tty_ldisc *ld) return; } raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); - wake_up(&tty_ldisc_idle); + wake_up(&ld->wq_idle); } /** @@ -200,6 +199,8 @@ static struct tty_ldisc *tty_ldisc_get(int disc) ld->ops = ldops; atomic_set(&ld->users, 1); + init_waitqueue_head(&ld->wq_idle); + return ld; } @@ -538,7 +539,7 @@ static void tty_ldisc_flush_works(struct tty_struct *tty) static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout) { long ret; - ret = wait_event_timeout(tty_ldisc_idle, + ret = wait_event_timeout(tty->ldisc->wq_idle, atomic_read(&tty->ldisc->users) == 1, timeout); return ret > 0 ? 0 : -EBUSY; } diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h index ff7dc08..fb79dd8 100644 --- a/include/linux/tty_ldisc.h +++ b/include/linux/tty_ldisc.h @@ -110,6 +110,7 @@ #include <linux/fs.h> #include <linux/wait.h> #include <linux/pps_kernel.h> +#include <linux/wait.h> struct tty_ldisc_ops { int magic; @@ -154,6 +155,7 @@ struct tty_ldisc_ops { struct tty_ldisc { struct tty_ldisc_ops *ops; atomic_t users; + wait_queue_head_t wq_idle; }; #define TTY_LDISC_MAGIC 0x5403 -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) 2012-05-03 12:37 ` [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) Ivo Sieben @ 2012-05-03 16:24 ` Greg KH 2012-05-10 15:28 ` Alan Cox 1 sibling, 0 replies; 11+ messages in thread From: Greg KH @ 2012-05-03 16:24 UTC (permalink / raw) To: Ivo Sieben; +Cc: linux-serial, Alan Cox, RT On Thu, May 03, 2012 at 02:37:43PM +0200, Ivo Sieben wrote: > Solved unnecessary schedule latency in the TTY layer when used in > realtime context: > The global wait_queue that is used for line discipline idle handling is > moved to a separate wait_queue for each line instance. This prevents > unnecessary blocking on one line, because of idle handling on another > line. This patch makes sense even in a non-RT case, as it localizes the lock to the specific ldisc. Alan, any objection to me applying this one to the tree now? thanks, greg k-h > Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and > no interrupts (and therefor no scheduling) is disabled. > > Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com> > --- > drivers/tty/tty_ldisc.c | 7 ++++--- > include/linux/tty_ldisc.h | 2 ++ > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c > index 30836cc..77eb299 100644 > --- a/drivers/tty/tty_ldisc.c > +++ b/drivers/tty/tty_ldisc.c > @@ -28,7 +28,6 @@ > > static DEFINE_RAW_SPINLOCK(tty_ldisc_lock); > static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait); > -static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle); > /* Line disc dispatch table */ > static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS]; > > @@ -65,7 +64,7 @@ static void put_ldisc(struct tty_ldisc *ld) > return; > } > raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags); > - wake_up(&tty_ldisc_idle); > + wake_up(&ld->wq_idle); > } > > /** > @@ -200,6 +199,8 @@ static struct tty_ldisc *tty_ldisc_get(int disc) > > ld->ops = ldops; > atomic_set(&ld->users, 1); > + init_waitqueue_head(&ld->wq_idle); > + > return ld; > } > > @@ -538,7 +539,7 @@ static void tty_ldisc_flush_works(struct tty_struct *tty) > static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout) > { > long ret; > - ret = wait_event_timeout(tty_ldisc_idle, > + ret = wait_event_timeout(tty->ldisc->wq_idle, > atomic_read(&tty->ldisc->users) == 1, timeout); > return ret > 0 ? 0 : -EBUSY; > } > diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h > index ff7dc08..fb79dd8 100644 > --- a/include/linux/tty_ldisc.h > +++ b/include/linux/tty_ldisc.h > @@ -110,6 +110,7 @@ > #include <linux/fs.h> > #include <linux/wait.h> > #include <linux/pps_kernel.h> > +#include <linux/wait.h> > > struct tty_ldisc_ops { > int magic; > @@ -154,6 +155,7 @@ struct tty_ldisc_ops { > struct tty_ldisc { > struct tty_ldisc_ops *ops; > atomic_t users; > + wait_queue_head_t wq_idle; > }; > > #define TTY_LDISC_MAGIC 0x5403 > -- > 1.7.0.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-serial" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) 2012-05-03 12:37 ` [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) Ivo Sieben 2012-05-03 16:24 ` Greg KH @ 2012-05-10 15:28 ` Alan Cox 1 sibling, 0 replies; 11+ messages in thread From: Alan Cox @ 2012-05-10 15:28 UTC (permalink / raw) To: Ivo Sieben; +Cc: Greg KH, linux-serial, RT On Thu, 3 May 2012 14:37:43 +0200 Ivo Sieben <meltedpianoman@gmail.com> wrote: > Solved unnecessary schedule latency in the TTY layer when used in > realtime context: > The global wait_queue that is used for line discipline idle handling > is moved to a separate wait_queue for each line instance. This > prevents unnecessary blocking on one line, because of idle handling > on another line. > > Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes > and no interrupts (and therefor no scheduling) is disabled. > > Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com> And this one as a standalone I think we should merge. I'm surprised it makes a difference but its small, clean and localises a lock which is a good thing. Alan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) 2012-05-03 12:37 [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben 2012-05-03 12:37 ` [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) Ivo Sieben 2012-05-03 12:37 ` [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) Ivo Sieben @ 2012-05-07 14:10 ` Ivo Sieben 2012-05-10 15:26 ` Alan Cox 3 siblings, 0 replies; 11+ messages in thread From: Ivo Sieben @ 2012-05-07 14:10 UTC (permalink / raw) To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben Hi, 2012/5/3 Ivo Sieben <meltedpianoman@gmail.com>: > --- a/drivers/tty/tty_buffer.c > +++ b/drivers/tty/tty_buffer.c > @@ -317,12 +317,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags); > > void tty_schedule_flip(struct tty_struct *tty) > { > - unsigned long flags; > - spin_lock_irqsave(&tty->buf.lock, flags); > - if (tty->buf.tail != NULL) > - tty->buf.tail->commit = tty->buf.tail->used; > - spin_unlock_irqrestore(&tty->buf.lock, flags); > - schedule_work(&tty->buf.work); > + tty_flip_buffer_push(tty); > } > EXPORT_SYMBOL(tty_schedule_flip); > I have an additional question on the above change in the patch that I've send: I found that two functions in drivers/tty/tty_buffer.c implement almost the same functionality: - tty_schedule_flip - tty_flip_buffer_push Only difference was that tty_schedule_flip() always uses the work queue, while the tty_flip_buffer_push only uses the work queue in case of a non prempt_rt system and low_latency flag unset. But is my change correct? I see that most serial drivers use the tty_flip_buffer_push() function. But still a number of drivers use the tty_schedule_flip() function. I even found one driver that uses both (drivers/staging/serqt_usb2/serqt_usb2.c). Does my patch introduce bugs to these drivers? Or is the tty_schedule_flip() a legacy function, and would it be better to remove it completely? Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) 2012-05-03 12:37 [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben ` (2 preceding siblings ...) 2012-05-07 14:10 ` [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben @ 2012-05-10 15:26 ` Alan Cox 2012-05-14 12:25 ` Ivo Sieben 3 siblings, 1 reply; 11+ messages in thread From: Alan Cox @ 2012-05-10 15:26 UTC (permalink / raw) To: Ivo Sieben; +Cc: Greg KH, linux-serial, RT > Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes > and no interrupts (and therefor no scheduling) is disabled. > > Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com> (Coming back round to these patches now the urgent stuff is buried for a bit) > void tty_schedule_flip(struct tty_struct *tty) > { > - unsigned long flags; > - spin_lock_irqsave(&tty->buf.lock, flags); > - if (tty->buf.tail != NULL) > - tty->buf.tail->commit = tty->buf.tail->used; > - spin_unlock_irqrestore(&tty->buf.lock, flags); > - schedule_work(&tty->buf.work); > + tty_flip_buffer_push(tty); > } You'd need to ifdef both of these for non RT cases. I think it may be right for RT although I'm not 100% sure on the locking. At this point I think we'd be better off sorting out our tty locking in general before tackling RT optimisations. However as an RT patch it looks good and its interesting to see RT simplifying code not making it more complex. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) 2012-05-10 15:26 ` Alan Cox @ 2012-05-14 12:25 ` Ivo Sieben 2012-05-15 15:04 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Ivo Sieben @ 2012-05-14 12:25 UTC (permalink / raw) To: Alan Cox; +Cc: Greg KH, linux-serial, RT Hi, 2012/5/10 Alan Cox <alan@linux.intel.com>: > > You'd need to ifdef both of these for non RT cases. I think it may be > right for RT although I'm not 100% sure on the locking. Alan, I don't completely understand what you mean with "ifdef both". Regarding the following two functions defined in drivers/tty/tty_buffer.c - tty_schedule_flip - tty_flip_buffer_push They both implement almost the same functionality: void tty_schedule_flip(struct tty_struct *tty) { unsigned long flags; spin_lock_irqsave(&tty->buf.lock, flags); if (tty->buf.tail != NULL) tty->buf.tail->commit = tty->buf.tail->used; spin_unlock_irqrestore(&tty->buf.lock, flags); schedule_work(&tty->buf.work); } void tty_flip_buffer_push(struct tty_struct *tty) { unsigned long flags; spin_lock_irqsave(&tty->buf.lock, flags); if (tty->buf.tail != NULL) tty->buf.tail->commit = tty->buf.tail->used; spin_unlock_irqrestore(&tty->buf.lock, flags); #ifndef CONFIG_PREEMPT_RT_FULL if (tty->low_latency) flush_to_ldisc(&tty->buf.work); else schedule_work(&tty->buf.work); #else flush_to_ldisc(&tty->buf.work); #endif } Only difference is that tty_schedule_flip() always uses the work queue, while the tty_flip_buffer_push only uses the work queue in case of a non prempt_rt system and low_latency flag unset. I see that most serial drivers use the tty_flip_buffer_push() function. But still a number of drivers use the tty_schedule_flip() function. I even found one driver that uses both (drivers/staging/serqt_usb2/serqt_usb2.c). Is is there a reason for these two functions implementing slightly different behavior? If not, I think it would be better to remove the tty_schedule_flip() function completely in a separate patch... Regards, Ivo Sieben ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) 2012-05-14 12:25 ` Ivo Sieben @ 2012-05-15 15:04 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2012-05-15 15:04 UTC (permalink / raw) To: Ivo Sieben; +Cc: Alan Cox, Greg KH, linux-serial, RT On Mon, 2012-05-14 at 14:25 +0200, Ivo Sieben wrote: > void tty_flip_buffer_push(struct tty_struct *tty) > { > unsigned long flags; > spin_lock_irqsave(&tty->buf.lock, flags); > if (tty->buf.tail != NULL) > tty->buf.tail->commit = tty->buf.tail->used; > spin_unlock_irqrestore(&tty->buf.lock, flags); > > #ifndef CONFIG_PREEMPT_RT_FULL > if (tty->low_latency) > flush_to_ldisc(&tty->buf.work); > else > schedule_work(&tty->buf.work); > #else > flush_to_ldisc(&tty->buf.work); > #endif > } > Note, just to keep ugly #ifdefs out of C code, or at least C functions, it is usually better to wrap the above in something like: #ifdef CONFIG_PREEMPT_RT_FULL static inline void tty_flush_work(struct tty_struct *tty) { flush_to_ldisc(&tty->buf.work); } #else static inline void tty_flush_work(struct tty_struct *tty) { if (tty->low_latency) flush_to_ldisc(&tty->buf.work); else schedule_work(&tty->buf.work); } #endif Then the C function would just look like: void tty_flip_buffer_push(struct tty_struct *tty) { unsigned long flags; spin_lock_irqsave(&tty->buf.lock, flags); if (tty->buf.tail != NULL) tty->buf.tail->commit = tty->buf.tail->used; spin_unlock_irqrestore(&tty->buf.lock, flags); tty_flush_work(tty); } -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-05-15 15:04 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-03 12:37 [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben 2012-05-03 12:37 ` [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3) Ivo Sieben 2012-05-03 16:25 ` Greg KH 2012-05-07 7:45 ` Ivo Sieben 2012-05-03 12:37 ` [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3) Ivo Sieben 2012-05-03 16:24 ` Greg KH 2012-05-10 15:28 ` Alan Cox 2012-05-07 14:10 ` [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3) Ivo Sieben 2012-05-10 15:26 ` Alan Cox 2012-05-14 12:25 ` Ivo Sieben 2012-05-15 15:04 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).