Hello,
On 01/12/2013 12:50 PM, Shawn Guo wrote:
> The clockevent core is able to figure out the best mult and shift,
> calculate min_delta_ns and max_delta_ns, with the necessary info passed
> into clockevents_config_and_register(). Use this combined configure
> and register function where possible to make the codes less error prone
> and gain some positive diff stat.
>
> Signed-off-by: Shawn Guo <[email protected]>
> Cc: Andres Salomon <[email protected]>
> Cc: Nicolas Ferre <[email protected]>
> Acked-by: Maxime Ripard <[email protected]>
[...]
I just bisected a timer problem down to this commit.
> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> index 32cb929..8a61872 100644
> --- a/drivers/clocksource/tcb_clksrc.c
> +++ b/drivers/clocksource/tcb_clksrc.c
> @@ -157,7 +157,6 @@ static struct tc_clkevt_device clkevt = {
> .name = "tc_clkevt",
> .features = CLOCK_EVT_FEAT_PERIODIC
> | CLOCK_EVT_FEAT_ONESHOT,
> - .shift = 32,
> /* Should be lower than at91rm9200's system timer */
> .rating = 125,
> .set_next_event = tc_next_event,
> @@ -196,13 +195,9 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
>
> timer_clock = clk32k_divisor_idx;
>
> - clkevt.clkevt.mult = div_sc(32768, NSEC_PER_SEC, clkevt.clkevt.shift);
> - clkevt.clkevt.max_delta_ns
> - = clockevent_delta2ns(0xffff, &clkevt.clkevt);
> - clkevt.clkevt.min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
^^^^^^
With this patch, the "+ 1" gets lost, resulting in a different
"min_delta_ns".
good:
setup_clkevents: shift=32 mult=140737 min=30518 max=1999976422
bad:
setup_clkevents: shift=32 mult=140737 min=30517 max=1999976422
This leads to problems with schedule_delayed_work() with a delay of
"1". These worksqueues seem to be hanging for several seconds. I'll
send a RFC patch.
> clkevt.clkevt.cpumask = cpumask_of(0);
>
> - clockevents_register_device(&clkevt.clkevt);
> + clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
>
> setup_irq(irq, &tc_irqaction);
> }
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
The commit
77cc982 clocksource: use clockevents_config_and_register() where possible
switches from manually calculating min_delta_ns (and others) and
clockevents_register_device() to automatic calculation via
clockevents_config_and_register(). During this conversation the "+ 1" in
min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
was lost. This leads to problems with schedule_delayed_work() with a delay of
"1". Resulting in the work not scheduled in time.
This patch fixes the problem by increasing the min_delta to "2" ticks.
Signed-off-by: Marc Kleine-Budde <[email protected]>
---
drivers/clocksource/tcb_clksrc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 8a61872..7cf6dc7 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
clkevt.clkevt.cpumask = cpumask_of(0);
- clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
+ clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
setup_irq(irq, &tc_irqaction);
}
--
1.8.2.rc2
On 04/23/2013 03:08 PM, Marc Kleine-Budde wrote:
> The commit
>
> 77cc982 clocksource: use clockevents_config_and_register() where possible
>
> switches from manually calculating min_delta_ns (and others) and
> clockevents_register_device() to automatic calculation via
> clockevents_config_and_register(). During this conversation the "+ 1" in
>
> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>
> was lost. This leads to problems with schedule_delayed_work() with a delay of
> "1". Resulting in the work not scheduled in time.
>
> This patch fixes the problem by increasing the min_delta to "2" ticks.
>
> Signed-off-by: Marc Kleine-Budde <[email protected]>
The problem appears on at91sam9263. This patch successfully fixes the
problem and applies to current linus/master (post v3.9-rc8).
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On Tue, Apr 23, 2013 at 03:11:33PM +0200, Marc Kleine-Budde wrote:
> On 04/23/2013 03:08 PM, Marc Kleine-Budde wrote:
> > The commit
> >
> > 77cc982 clocksource: use clockevents_config_and_register() where possible
> >
> > switches from manually calculating min_delta_ns (and others) and
> > clockevents_register_device() to automatic calculation via
> > clockevents_config_and_register(). During this conversation the "+ 1" in
> >
> > min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
> >
> > was lost. This leads to problems with schedule_delayed_work() with a delay of
> > "1". Resulting in the work not scheduled in time.
> >
> > This patch fixes the problem by increasing the min_delta to "2" ticks.
> >
> > Signed-off-by: Marc Kleine-Budde <[email protected]>
>
> The problem appears on at91sam9263. This patch successfully fixes the
> problem and applies to current linus/master (post v3.9-rc8).
Thanks for the fixing, Marc.
Acked-by: Shawn Guo <[email protected]>
On 04/23/2013 03:44 PM, Shawn Guo wrote:
> On Tue, Apr 23, 2013 at 03:11:33PM +0200, Marc Kleine-Budde wrote:
>> On 04/23/2013 03:08 PM, Marc Kleine-Budde wrote:
>>> The commit
>>>
>>> 77cc982 clocksource: use clockevents_config_and_register() where possible
>>>
>>> switches from manually calculating min_delta_ns (and others) and
>>> clockevents_register_device() to automatic calculation via
>>> clockevents_config_and_register(). During this conversation the "+ 1" in
>>>
>>> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>>>
>>> was lost. This leads to problems with schedule_delayed_work() with a delay of
>>> "1". Resulting in the work not scheduled in time.
>>>
>>> This patch fixes the problem by increasing the min_delta to "2" ticks.
>>>
>>> Signed-off-by: Marc Kleine-Budde <[email protected]>
>>
>> The problem appears on at91sam9263. This patch successfully fixes the
>> problem and applies to current linus/master (post v3.9-rc8).
>
> Thanks for the fixing, Marc.
>
> Acked-by: Shawn Guo <[email protected]>
Thanks. The downside of my RFC Patch is, that min_delta_ns is increased
from 30518 to 61035:
good: setup_clkevents: shift=32 mult=140737 min=30518 max=1999976422
bad: setup_clkevents: shift=32 mult=140737 min=30517 max=1999976422
RFC: setup_clkevents: shift=32 mult=140737 min=61035 max=1999976422
To keep the original min_delta_ns value of 30518, a rather hackish
option would be this:
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 8a61872..30e0a68 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -198,6 +198,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
clkevt.clkevt.cpumask = cpumask_of(0);
clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
+ clkevt.clkevt.min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
setup_irq(irq, &tc_irqaction);
}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 04/23/2013 03:08 PM, Marc Kleine-Budde wrote:
> The commit
>
> 77cc982 clocksource: use clockevents_config_and_register() where possible
>
> switches from manually calculating min_delta_ns (and others) and
> clockevents_register_device() to automatic calculation via
> clockevents_config_and_register(). During this conversation the "+ 1" in
>
> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>
> was lost. This leads to problems with schedule_delayed_work() with a delay of
> "1". Resulting in the work not scheduled in time.
>
> This patch fixes the problem by increasing the min_delta to "2" ticks.
>
> Signed-off-by: Marc Kleine-Budde <[email protected]>
Who will take care of this patch?
Marc
> ---
> drivers/clocksource/tcb_clksrc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> index 8a61872..7cf6dc7 100644
> --- a/drivers/clocksource/tcb_clksrc.c
> +++ b/drivers/clocksource/tcb_clksrc.c
> @@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
>
> clkevt.clkevt.cpumask = cpumask_of(0);
>
> - clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
> + clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
>
> setup_irq(irq, &tc_irqaction);
> }
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On Thu, 25 Apr 2013, Marc Kleine-Budde wrote:
> On 04/23/2013 03:08 PM, Marc Kleine-Budde wrote:
> > The commit
> >
> > 77cc982 clocksource: use clockevents_config_and_register() where possible
> >
> > switches from manually calculating min_delta_ns (and others) and
> > clockevents_register_device() to automatic calculation via
> > clockevents_config_and_register(). During this conversation the "+ 1" in
> >
> > min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
> >
> > was lost. This leads to problems with schedule_delayed_work() with a delay of
> > "1". Resulting in the work not scheduled in time.
> >
> > This patch fixes the problem by increasing the min_delta to "2" ticks.
> >
> > Signed-off-by: Marc Kleine-Budde <[email protected]>
>
> Who will take care of this patch?
If you had cc'ed me in the first place ...
> Marc
>
> > ---
> > drivers/clocksource/tcb_clksrc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> > index 8a61872..7cf6dc7 100644
> > --- a/drivers/clocksource/tcb_clksrc.c
> > +++ b/drivers/clocksource/tcb_clksrc.c
> > @@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
> >
> > clkevt.clkevt.cpumask = cpumask_of(0);
> >
> > - clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
> > + clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
> >
> > setup_irq(irq, &tc_irqaction);
> > }
> >
>
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
>
On Tue, 23 Apr 2013, Marc Kleine-Budde wrote:
> The commit
>
> 77cc982 clocksource: use clockevents_config_and_register() where possible
>
> switches from manually calculating min_delta_ns (and others) and
> clockevents_register_device() to automatic calculation via
> clockevents_config_and_register(). During this conversation the "+ 1" in
>
> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>
> was lost. This leads to problems with schedule_delayed_work() with a delay of
> "1". Resulting in the work not scheduled in time.
Errm. How is schedule_delayed_work() related to this?
schedule_delayed_work() is jiffies based and has absolutely nothing to
do with clockevents.
> This patch fixes the problem by increasing the min_delta to "2" ticks.
This changelog sucks.
> Signed-off-by: Marc Kleine-Budde <[email protected]>
> ---
> drivers/clocksource/tcb_clksrc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> index 8a61872..7cf6dc7 100644
> --- a/drivers/clocksource/tcb_clksrc.c
> +++ b/drivers/clocksource/tcb_clksrc.c
> @@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
>
> clkevt.clkevt.cpumask = cpumask_of(0);
>
> - clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
> + clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
>
> setup_irq(irq, &tc_irqaction);
> }
> --
> 1.8.2.rc2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On 04/25/2013 04:21 PM, Thomas Gleixner wrote:
> On Tue, 23 Apr 2013, Marc Kleine-Budde wrote:
>> The commit
>>
>> 77cc982 clocksource: use clockevents_config_and_register() where possible
>>
>> switches from manually calculating min_delta_ns (and others) and
>> clockevents_register_device() to automatic calculation via
>> clockevents_config_and_register(). During this conversation the "+ 1" in
>>
>> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>>
>> was lost. This leads to problems with schedule_delayed_work() with a delay of
>> "1". Resulting in the work not scheduled in time.
>
> Errm. How is schedule_delayed_work() related to this?
Let me explain:
For other reasons [1], we reverted:
f23eb2b tty: stop using "delayed_work" in the tty layer
(for reference find the patch below [2]). Which basically means
tty_schedule_flip() uses schedule_delayed_work(&buf->work, 1) instead
of schedule_work(&buf->work) as mainline does.
This works pretty well until v3.8. With v3.9-rc kernel the serial
console gets sluggish. It takes several seconds to abort a "find /" on
a serial console with Ctrl+c. I've bisected the problem down to:
77cc982 clocksource: use clockevents_config_and_register() where
possible
A printk shows the subtile difference in the min_delta_ns:
good: setup_clkevents: shift=32 mult=140737 min=30518 max=1999976422
bad: setup_clkevents: shift=32 mult=140737 min=30517 max=1999976422
Restoring min_delta_ns to the original value, with this hackish patch,
"fixes" the serial problem, too:
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 8a61872..30e0a68 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -198,6 +198,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
clkevt.clkevt.cpumask = cpumask_of(0);
clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
+ clkevt.clkevt.min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
setup_irq(irq, &tc_irqaction);
}
> schedule_delayed_work() is jiffies based and has absolutely nothing to
> do with clockevents.
I'm puzzled, too.
>> This patch fixes the problem by increasing the min_delta to "2" ticks.
>
> This changelog sucks.
>
>> Signed-off-by: Marc Kleine-Budde <[email protected]>
>> ---
>> drivers/clocksource/tcb_clksrc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
>> index 8a61872..7cf6dc7 100644
>> --- a/drivers/clocksource/tcb_clksrc.c
>> +++ b/drivers/clocksource/tcb_clksrc.c
>> @@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
>>
>> clkevt.clkevt.cpumask = cpumask_of(0);
>>
>> - clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
>> + clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
>>
>> setup_irq(irq, &tc_irqaction);
>> }
[1] http://permalink.gmane.org/gmane.linux.kernel/1479280
[2]
From e4d43afca06724c6a59b8caac1f5dc07a9974aaa Mon Sep 17 00:00:00 2001
From: Steffen Trumtrar <[email protected]>
Date: Fri, 5 Apr 2013 16:38:55 +0200
Subject: [PATCH] HACK: tty: Revert stop using "delayed_work" in the tty layer
This patch reverts:
f23eb2b tty: stop using "delayed_work" in the tty layer
That patch changed the tty buffer flip from beeing handled in a "delayed_work"
to an immediately scheduled "work". The original motivation was to decrease the
latency, this however leads to 15-25 %-points of more load on certain
workloads.
Here, less load is more desired than low latency, thus the patch is reverted
until a mainline accepted solution is found.
See for a detailed discussion see:
http://permalink.gmane.org/gmane.linux.kernel/1479280
Signed-off-by: Steffen Trumtrar <[email protected]>
Signed-off-by: Marc Kleine-Budde <[email protected]>
---
drivers/tty/n_tty.c | 2 +-
drivers/tty/tty_buffer.c | 12 ++++++------
drivers/tty/tty_ldisc.c | 10 +++++-----
include/linux/tty.h | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 05e72be..ead46da 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -153,7 +153,7 @@ static void n_tty_set_room(struct tty_struct *tty)
if (left && !old_left) {
WARN_RATELIMIT(tty->port->itty == NULL,
"scheduling with invalid itty\n");
- schedule_work(&tty->port->buf.work);
+ schedule_delayed_work(&tty->port->buf.work, 1);
}
}
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 578aa75..187b874 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -339,7 +339,7 @@ void tty_schedule_flip(struct tty_port *port)
if (buf->tail != NULL)
buf->tail->commit = buf->tail->used;
spin_unlock_irqrestore(&buf->lock, flags);
- schedule_work(&buf->work);
+ schedule_delayed_work(&buf->work, 1);
}
EXPORT_SYMBOL(tty_schedule_flip);
@@ -418,7 +418,7 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string_flags);
static void flush_to_ldisc(struct work_struct *work)
{
- struct tty_port *port = container_of(work, struct tty_port, buf.work);
+ struct tty_port *port = container_of(work, struct tty_port, buf.work.work);
struct tty_bufhead *buf = &port->buf;
struct tty_struct *tty;
unsigned long flags;
@@ -492,7 +492,7 @@ static void flush_to_ldisc(struct work_struct *work)
void tty_flush_to_ldisc(struct tty_struct *tty)
{
if (!tty->port->low_latency)
- flush_work(&tty->port->buf.work);
+ flush_delayed_work(&tty->port->buf.work);
}
/**
@@ -520,9 +520,9 @@ void tty_flip_buffer_push(struct tty_port *port)
spin_unlock_irqrestore(&buf->lock, flags);
if (port->low_latency)
- flush_to_ldisc(&buf->work);
+ flush_to_ldisc(&buf->work.work);
else
- schedule_work(&buf->work);
+ schedule_delayed_work(&port->buf.work, 1);
}
EXPORT_SYMBOL(tty_flip_buffer_push);
@@ -545,6 +545,6 @@ void tty_buffer_init(struct tty_port *port)
buf->tail = NULL;
buf->free = NULL;
buf->memory_used = 0;
- INIT_WORK(&buf->work, flush_to_ldisc);
+ INIT_DELAYED_WORK(&port->buf.work, flush_to_ldisc);
}
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index d794087..1cb63f1 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -514,7 +514,7 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
static int tty_ldisc_halt(struct tty_struct *tty)
{
clear_bit(TTY_LDISC, &tty->flags);
- return cancel_work_sync(&tty->port->buf.work);
+ return cancel_delayed_work(&tty->port->buf.work);
}
/**
@@ -527,7 +527,7 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
{
flush_work(&tty->hangup_work);
flush_work(&tty->SAK_work);
- flush_work(&tty->port->buf.work);
+ flush_delayed_work(&tty->port->buf.work);
}
/**
@@ -706,9 +706,9 @@ enable:
/* Restart the work queue in case no characters kick it off. Safe if
already running */
if (work)
- schedule_work(&tty->port->buf.work);
+ schedule_delayed_work(&tty->port->buf.work, 1);
if (o_work)
- schedule_work(&o_tty->port->buf.work);
+ schedule_delayed_work(&o_tty->port->buf.work, 1);
mutex_unlock(&tty->ldisc_mutex);
tty_unlock(tty);
return retval;
@@ -819,7 +819,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
*/
clear_bit(TTY_LDISC, &tty->flags);
tty_unlock(tty);
- cancel_work_sync(&tty->port->buf.work);
+ cancel_delayed_work(&tty->port->buf.work);
mutex_unlock(&tty->ldisc_mutex);
retry:
tty_lock(tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c75d886..f7f6836 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -52,7 +52,7 @@ struct tty_buffer {
struct tty_bufhead {
- struct work_struct work;
+ struct delayed_work work;
spinlock_t lock;
struct tty_buffer *head; /* Queue head */
struct tty_buffer *tail; /* Active buffer */
--
1.8.2.rc2
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Hello Thomas,
On 04/25/2013 04:53 PM, Marc Kleine-Budde wrote:
> On 04/25/2013 04:21 PM, Thomas Gleixner wrote:
>> On Tue, 23 Apr 2013, Marc Kleine-Budde wrote:
>>> The commit
>>>
>>> 77cc982 clocksource: use clockevents_config_and_register() where possible
>>>
>>> switches from manually calculating min_delta_ns (and others) and
>>> clockevents_register_device() to automatic calculation via
>>> clockevents_config_and_register(). During this conversation the "+ 1" in
>>>
>>> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>>>
>>> was lost. This leads to problems with schedule_delayed_work() with a delay of
>>> "1". Resulting in the work not scheduled in time.
>>
>> Errm. How is schedule_delayed_work() related to this?
Coming back to the this problem after some time.
I'm on at91sam9263 (armv5) using latest linus/master (v3.10-rc6-84-gc069114).
I can illustrate the problem running cyclictest.
Here's the output of unpatched linus/master:
> root@halo22:~ cyclictest -i 10000 -n -t 4
> policy: other/other: loadavg: 0.10 0.19 0.10 2/31 133
>
> T: 0 ( 130) P: 0 I:10000 C: 30767 Min: 57 Act: 165 Avg:909453 Max: 201897444
> T: 1 ( 131) P: 0 I:10500 C: 29302 Min: 68 Act: 144 Avg:906651 Max: 204005666
> T: 2 ( 132) P: 0 I:11000 C: 27970 Min: 79 Act: 188 Avg:905068 Max: 204008666
> T: 3 ( 133) P: 0 I:11500 C: 26754 Min: 52 Act: 132 Avg:904438 Max: 203665111
Cyclictest runs for about two seconds, but then shows no output for
about two seconds, then it runs again, then stops and so on...
With my patch applied, the numbers are back to normal:
> root@halo22:~ cyclictest -i 10000 -n -t 4
> policy: other/other: loadavg: 0.34 0.15 0.06 2/32 109
>
> T: 0 ( 106) P: 0 I:10000 C: 292 Min: 112 Act: 208 Avg: 189 Max: 1715
> T: 1 ( 107) P: 0 I:10500 C: 277 Min: 107 Act: 198 Avg: 201 Max: 1464
> T: 2 ( 108) P: 0 I:11000 C: 265 Min: 104 Act: 148 Avg: 195 Max: 1530
> T: 3 ( 109) P: 0 I:11500 C: 252 Min: 113 Act: 205 Avg: 167 Max: 269
Cyclictest keeps running without 2 second stops.
Is my patch a valid solution to the problem? If so, I'll write more
specific commit message.
regards,
Marc
---
The commit
77cc982 clocksource: use clockevents_config_and_register() where possible
switches from manually calculating min_delta_ns (and others) and
clockevents_register_device() to automatic calculation via
clockevents_config_and_register(). During this conversation the "+ 1" in
min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
was lost. This leads to problems with schedule_delayed_work() with a delay of
"1". Resulting in the work not scheduled in time.
This patch fixes the problem by increasing the min_delta to "2" ticks.
Signed-off-by: Marc Kleine-Budde <[email protected]>
---
drivers/clocksource/tcb_clksrc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 8a61872..7cf6dc7 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -197,7 +197,7 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
clkevt.clkevt.cpumask = cpumask_of(0);
- clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff);
+ clockevents_config_and_register(&clkevt.clkevt, 32768, 2, 0xffff);
setup_irq(irq, &tc_irqaction);
}
-- 1.8.2.rc2
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 20.06.2013 17:00, Marc Kleine-Budde wrote:
> Hello Thomas,
>
> On 04/25/2013 04:53 PM, Marc Kleine-Budde wrote:
>> On 04/25/2013 04:21 PM, Thomas Gleixner wrote:
>>> On Tue, 23 Apr 2013, Marc Kleine-Budde wrote:
>>>> The commit
>>>>
>>>> 77cc982 clocksource: use clockevents_config_and_register() where possible
>>>>
>>>> switches from manually calculating min_delta_ns (and others) and
>>>> clockevents_register_device() to automatic calculation via
>>>> clockevents_config_and_register(). During this conversation the "+ 1" in
>>>>
>>>> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>>>>
>>>> was lost. This leads to problems with schedule_delayed_work() with a delay of
>>>> "1". Resulting in the work not scheduled in time.
>>>
>>> Errm. How is schedule_delayed_work() related to this?
I also stumbled over this issue now after upgrading from 3.4.57 to
3.10.10. Even sleep() is unreliable - sleeping for 2 seconds often
sleeps just more than 3 seconds.
The now lost +1 has actually been added in 2007 to solve a rounding issue:
http://permalink.gmane.org/gmane.linux.kernel/549744
So I think this should either be fixed by always rounding up in
clockevent_delta2ns() or by adding +1 to the min_delta_ns in the
clockevents_config_and_register() function. Dunno if rounding up will
cause problems with the max_delta_ns as it might be too big then so
probably the second approach is better.
Opinions?
- ron
--
Ronald Wahl - [email protected] - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes
On 09/09/2013 10:18 AM, Ronald Wahl wrote:
> On 20.06.2013 17:00, Marc Kleine-Budde wrote:
>> Hello Thomas,
>>
>> On 04/25/2013 04:53 PM, Marc Kleine-Budde wrote:
>>> On 04/25/2013 04:21 PM, Thomas Gleixner wrote:
>>>> On Tue, 23 Apr 2013, Marc Kleine-Budde wrote:
>>>>> The commit
>>>>>
>>>>> 77cc982 clocksource: use clockevents_config_and_register()
>>>>> where possible
>>>>>
>>>>> switches from manually calculating min_delta_ns (and others) and
>>>>> clockevents_register_device() to automatic calculation via
>>>>> clockevents_config_and_register(). During this conversation the "+
>>>>> 1" in
>>>>>
>>>>> min_delta_ns = clockevent_delta2ns(1, &clkevt.clkevt) + 1;
>>>>>
>>>>> was lost. This leads to problems with schedule_delayed_work() with
>>>>> a delay of
>>>>> "1". Resulting in the work not scheduled in time.
>>>>
>>>> Errm. How is schedule_delayed_work() related to this?
>
> I also stumbled over this issue now after upgrading from 3.4.57 to
> 3.10.10. Even sleep() is unreliable - sleeping for 2 seconds often
> sleeps just more than 3 seconds.
>
> The now lost +1 has actually been added in 2007 to solve a rounding issue:
>
> http://permalink.gmane.org/gmane.linux.kernel/549744
>
> So I think this should either be fixed by always rounding up in
> clockevent_delta2ns() or by adding +1 to the min_delta_ns in the
> clockevents_config_and_register() function. Dunno if rounding up will
> cause problems with the max_delta_ns as it might be too big then so
> probably the second approach is better.
>
> Opinions?
Thomas, any thoughts on this?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |