IRQF_TIMER | IRQF_SHARED ?

[Jens Rottmann]

Hi Andres,

one of our customers tripped over the fact that the MFGPT driver won't share its
IRQ with anyone else. (MFGPT defaulted to same IRQ as audio, MFGPT driver loaded
first, audio fails.) *No big deal!* They don't actually need MFGPT and will
simply disable it. It just made me wonder ...

Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED (see patch
below)? mfgpt_tick() already does properly return IRQ_NONE when it feels
unresponsible. I tested it with either driver loaded first and it seemed to work
(well, at least audio worked, don't know how to explicitly test cs5535-clockevt).

I thought about latencies of IRQ sharing being unacceptable for a timer, but ...
- If MFGPT is loaded first there is no additional latency, is there? Audio
recieves its IRQs only as 2nd in list but that's not a problem.
- If MFGPT is loaded second - well, there is a latency, but without sharing the
IRQ the driver failed to load at all, so that's still an improvement.

But I did not fail to notice that _none_ of the code in drivers/clocksource/
uses IRQF_SHARED, obviously this must be deliberate.

So, what's so bad about IRQF_TIMER | IRQF_SHARED?

Any education would be welcome, even if combined with flame. :-)

Thanks and best regards,
Jens

--- linux-3.2-rc4/drivers/clocksource/cs5535-clockevt.c
+++ shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c
@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v

static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER | IRQF_SHARED,
.name = DRV_NAME,
};

_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Andres Salomon]

On Mon, 12 Dec 2011 16:41:25 +0100
Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:

> Hi Andres,
>
> one of our customers tripped over the fact that the MFGPT driver
> won't share its IRQ with anyone else. (MFGPT defaulted to same IRQ as
> audio, MFGPT driver loaded first, audio fails.) *No big deal!* They
> don't actually need MFGPT and will simply disable it. It just made me
> wonder ...
>
> Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED (see
> patch below)? mfgpt_tick() already does properly return IRQ_NONE when
> it feels unresponsible. I tested it with either driver loaded first
> and it seemed to work (well, at least audio worked, don't know how to
> explicitly test cs5535-clockevt).

Just loading cs5535-clockevt should start the periodic timer. On my
XO-1, IRQ 7 starts firing immediately.

>
> I thought about latencies of IRQ sharing being unacceptable for a
> timer, but ...
> - If MFGPT is loaded first there is no additional latency, is there?
> Audio recieves its IRQs only as 2nd in list but that's not a problem.
> - If MFGPT is loaded second - well, there is a latency, but without
> sharing the IRQ the driver failed to load at all, so that's still an
> improvement.
>
> But I did not fail to notice that _none_ of the code in
> drivers/clocksource/ uses IRQF_SHARED, obviously this must be
> deliberate.

Hm, maybe tglx knows? For my part, I don't think it would be a
problem, but I can imagine the reason for not sharing being clock drift
or something to that effect.

[Martin-Éric Racine]

12. joulukuuta 2011 22.31 Andres Salomon <dili...@queued.net> kirjoitti:
> On Mon, 12 Dec 2011 16:41:25 +0100
> Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:
>> one of our customers tripped over the fact that the MFGPT driver
>> won't share its IRQ with anyone else. (MFGPT defaulted to same IRQ as
>> audio, MFGPT driver loaded first, audio fails.) *No big deal!* They
>> don't actually need MFGPT and will simply disable it. It just made me
>> wonder ...
>>
>> Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED (see
>> patch below)? mfgpt_tick() already does properly return IRQ_NONE when
>> it feels unresponsible. I tested it with either driver loaded first
>> and it seemed to work (well, at least audio worked, don't know how to
>> explicitly test cs5535-clockevt).
>
> Just loading cs5535-clockevt should start the periodic timer.  On my
> XO-1, IRQ 7 starts firing immediately.

Could it be a good idea to inform udev maintainers of this?

Martin-Éric

[Andres Salomon]

On Mon, 12 Dec 2011 23:00:01 +0200
Martin-Éric Racine <martin-er...@iki.fi> wrote:

> 12. joulukuuta 2011 22.31 Andres Salomon <dili...@queued.net>
> kirjoitti:
> > On Mon, 12 Dec 2011 16:41:25 +0100
> > Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:
> >> one of our customers tripped over the fact that the MFGPT driver
> >> won't share its IRQ with anyone else. (MFGPT defaulted to same IRQ
> >> as audio, MFGPT driver loaded first, audio fails.) *No big deal!*
> >> They don't actually need MFGPT and will simply disable it. It just
> >> made me wonder ...
> >>
> >> Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED
> >> (see patch below)? mfgpt_tick() already does properly return
> >> IRQ_NONE when it feels unresponsible. I tested it with either
> >> driver loaded first and it seemed to work (well, at least audio
> >> worked, don't know how to explicitly test cs5535-clockevt).
> >
> > Just loading cs5535-clockevt should start the periodic timer.  On my
> > XO-1, IRQ 7 starts firing immediately.
>
> Could it be a good idea to inform udev maintainers of this?
>

It *would* be nice to get it auto-loading..

[Thomas Gleixner]

On Mon, 12 Dec 2011, Andres Salomon wrote:

> On Mon, 12 Dec 2011 16:41:25 +0100
> Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:
>
> > But I did not fail to notice that _none_ of the code in
> > drivers/clocksource/ uses IRQF_SHARED, obviously this must be
> > deliberate.
>
> Hm, maybe tglx knows? For my part, I don't think it would be a
> problem, but I can imagine the reason for not sharing being clock drift
> or something to that effect.

No, you can share a timer irq. The other drivers don't have the SHARED
flag set because they are on exclusive irq lines, which is the sane
thing to do. shared irqs suck and you figure that out once you try to
use that shared timer irq on a preempt-rt enabled kernel.

Thanks,

tglx

[Jens Rottmann]

Andres Salomon schrieb:

> Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:
>> and it seemed to work (well, at least audio worked, don't know how to
>> explicitly test cs5535-clockevt).
> Just loading cs5535-clockevt should start the periodic timer. On my
> XO-1, IRQ 7 starts firing immediately.

Hmm, no, doesn't work. :-| In /proc/interrupts IRQ0 gets increased, not (in
my case) IRQ 11. And also in /proc/timer_list next_event gets increased for
the pit device, not cs5535-clockevt.

But that's even without sharing enabled ... I guess I'll have to figure out
first what's going wrong here and then re-test. I'll get back to you ...

Thanks and regards,
Jens

[Jens Rottmann]

Jens Rottmann schrieb:
> Andres Salomon schrieb:

>> Just loading cs5535-clockevt should start the periodic timer.
>> On my XO-1, IRQ 7 starts firing immediately.

> Hmm, no, doesn't work. :-| In /proc/interrupts IRQ 0 gets increased,

> not (in my case) IRQ 11.

Update: I found that SMP-enabled kernels (like the generic one I was using) do
load cs5535-clockevt fine but ignore it and keep using the pit timer instead.

Responsible seems kernel/time/tick-common.c: tick_check_new_device()
/*
* If the cpu affinity of the device interrupt can not
* be set, ignore it.
*/
if (!irq_can_set_affinity(newdev->irq))
goto out_bc;

Looks like it has been that way for quite some time, maybe cs5535-clockevt
hasn't ever worked on SMP kernels.

If I turn off SMP, cs5535-clockevt replaces the pit timer and the MFGPT IRQ
starts firing just as you said - at least on 3.0.9. 3.2-rc5 prefers to hang
instead, looks like no timer events get generated. Sigh.

I'll try to narrow it down tomorrow, but now I'm calling it a day.

Cheers

[Andres Salomon]

On Wed, 14 Dec 2011 19:37:39 +0100
Jens Rottmann <JRot...@LiPPERTEmbedded.de> wrote:

> Jens Rottmann schrieb:
> > Andres Salomon schrieb:
> >> Just loading cs5535-clockevt should start the periodic timer.
> >> On my XO-1, IRQ 7 starts firing immediately.
> > Hmm, no, doesn't work. :-| In /proc/interrupts IRQ 0 gets
> > increased, not (in my case) IRQ 11.
>
> Update: I found that SMP-enabled kernels (like the generic one I was
> using) do load cs5535-clockevt fine but ignore it and keep using the
> pit timer instead.

Note that there are two subsystems at work here; clockevents and
clocksource. cs5535-clockevt uses clockevents (despite being in
drivers/clocksource/). For you to switch away from pit (via,
/sys/devices/system/clocksource/clocksource0/current_clocksource), we'd
have to implement cs5535_clocksource. Something that would be worth
doing, but I haven't looked into it..

>
> Responsible seems kernel/time/tick-common.c: tick_check_new_device()
> /*
> * If the cpu affinity of the device interrupt can not
> * be set, ignore it.
> */
> if (!irq_can_set_affinity(newdev->irq))
> goto out_bc;
>
> Looks like it has been that way for quite some time, maybe
> cs5535-clockevt hasn't ever worked on SMP kernels.

I've only ever tested it on UP kernels. I didn't know SMP CS5536
boards existed. Gosh, I hope all of those cs5535 drivers that hadn't
had their locking primitives tested aren't racy! ;)

>
> If I turn off SMP, cs5535-clockevt replaces the pit timer and the
> MFGPT IRQ starts firing just as you said - at least on 3.0.9. 3.2-rc5
> prefers to hang instead, looks like no timer events get generated.
> Sigh.
>

3.2-rc4 hung on my XO-1 due to a bug that's been fixed on rc5, but I
haven't tested rc5 yet.

[Jens Rottmann]

Hi Thomas,

I wrote (to Andres):

> If I turn off SMP, cs5535-clockevt replaces the pit timer and the MFGPT IRQ
> starts firing just as you said - at least on 3.0.9. 3.2-rc5 prefers to hang
> instead, looks like no timer events get generated.

Commit de28f25e8244c7353abed8de0c7792f5f883588c (Linus's tree) makes the
kernel hang during boot if the cs5535-clockevt driver is used. The (silent)
hang occurs shortly after the cs5535-clockevt becomes active. The keyboard
(incl. SysRq) stays responsive but the kernel seems starved of timer events
and does nothing on its own accord any more. Happens on 3.0.13, 3.1.5 and
3.2-rc6. Commit went into 2.6.32.50, too, but haven't tested this.

I don't know the code so I can't tell what's really going wrong or provide a
fix. I played around with it a bit:
Tried to change the order, putting "old->event_handler =" behind the
list_add() doesn't help. Neither does adding "if (new)" immediately before
"old->event_handler =". But "if (!new)" does.

As you see, I'm aimlessly guessing around, sorry. Just tell me if you need
anything tested.

Thanks and best regards,
Jens Rottmann

[Jens Rottmann]

cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

On SMP-capable kernels (e.g. generic distro kernel) the cs5535-clockevt driver
loads but is not actually used.

Setting cpumask to cpu_all_mask works for UP-only kernels, but if compiled for
SMP - though still running on the same UP hardware -
kernel/time/tick-common.c:tick_check_new_device() reads this as "non-cpu-local"
and silently ignores the device.

If we leave cpumask unset clockevents_register_device() will initialize it and
the cs5535-clockevt driver will be used no matter how the kernel was compiled.
Should anyone ever manage to stick a CS553x in an SMP system (is this even
possible?) then a warning will be printed. This is fine as the cs5535-clockevt
driver was never written/tested for SMP.

If bisecting led you here this patch may have exposed a pre-existing MFGPT
problem. Configure for UP-only and re-check.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>
---

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
@@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
.set_mode = mfgpt_set_mode,
.set_next_event = mfgpt_next_event,
.rating = 250,
- .cpumask = cpu_all_mask,
.shift = 32
};

_

[Jens Rottmann]

Hi Andres,

>> I found that SMP-enabled kernels (like the generic one I was
>> using) do load cs5535-clockevt fine but ignore it and keep using
>> the pit timer instead.

Solved, hope my patch is ok.

> I didn't know SMP CS5536 boards existed.

They don't. I was talking about SMP-capable kernels, e.g. _generic_ distro
kernels which were compiled with SMP support enabled, even though this feature
is not needed for a UP-only Geode. Sorry if I wasn't making myself clear.

Cheers,

[Jens Rottmann]

cs5535-clockevt: allow the MFGPT IRQ to be shared

Shared timer IRQs are not a good solution, however the Geode platform has no
APIC, IRQs are a scarce resource and there is no technical reason to forbid it
rightaway. Increased latencies and overhead due to sharing are still better
than a driver refusing to load.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>
---

Hi,

I tested this a bit longer, this time with MFGPT IRQ actually being triggered,
with cs5535-clockevt driver loaded first or second when sharing the IRQ, with
some CPU load or without.

I didn't encounter any negative effects of this change. I did have suspend
problems with cs5535-clockevt, but that was in no way different from before I
applied this patch, it's an unrelated BIOS issue.

Thomas Gleixner wrote:
> No, you can share a timer irq. The other drivers don't have the SHARED

> flag set because they are on exclusive irq lines, ...

Is this an ACK?

> shared irqs suck and you figure that out once you try to
> use that shared timer irq on a preempt-rt enabled kernel.

Or a NACK? :-| (I did address this in the commit log.)

The kernel I tested with was 3.2-rc6, CONFIG_SMP=y and CONFIG_PREEMPT=y. As I
said, I didn't notice anything bad happening.

Thanks and have a nice christmas holiday,
Jens

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ allow_shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c

@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v

static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER | IRQF_SHARED,
.name = DRV_NAME,
};

_

[Andres Salomon]

Hm, have you tried setting cpumask to cpumask_of(0), like a bunch of
the other clock_event_device drivers do?

[Andres Salomon]

Seems fine to me

Acked-by: Andres Salomon <dili...@queued.net>

[Jens Rottmann]

Hi Andres,

and a happy new year to you. Sorry for the delay.

Yes, I've seen that, and it was my initial approach. cpumask_of(0)
isn't regarded constant, so moved it from the struct init to
cs5535_mfgpt_init() ... but then I saw

clockevents.c:clockevents_register_device():
if (!dev->cpumask) {
WARN_ON(num_possible_cpus() > 1);
dev->cpumask = cpumask_of(smp_processor_id());
}

Looks to me like meant exactly for this purpose. This will set cpumask to
cpumask_of(0), i.e. does exactly what you're suggesting.

Both do work fine, but I understand cpumask=cpumask_of(0) to mean "this timer
is hardware-tied to a specific CPU", whereas cpumask unset means more like
"wow, we never expected anybody putting this hardware in an SMP system".

And you've said it yourself:

> I've only ever tested it on UP kernels. [...] I hope all of those cs5535

> drivers that hadn't had their locking primitives tested aren't racy!

So I see the WARN_ON as a bonus.

Cheers
Jens

[Andres Salomon]

Ah, great! In that case,

Acked-by: Andres Salomon <dili...@queued.net>

[Jens Rottmann]

cs5535-clockevt: allow the MFGPT IRQ to be shared

Shared timer IRQs are not a good solution, however the Geode platform has no
APIC, IRQs are a scarce resource and there is no technical reason to forbid it
rightaway. Increased latencies and overhead due to sharing are still better
than a driver refusing to load.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>

Acked-by: Andres Salomon <dili...@queued.net>
---

Hi,

could you please take this, for linux-next?

Thanks,

Jens

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ allow_shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c

@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v

static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER | IRQF_SHARED,
.name = DRV_NAME,
};

_

[Jens Rottmann]

cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

On SMP-capable kernels (e.g. generic distro kernel) the cs5535-clockevt driver
loads but is not actually used.

Setting cpumask to cpu_all_mask works for UP-only kernels, but if compiled for
SMP - though still running on the same UP hardware -
kernel/time/tick-common.c:tick_check_new_device() reads this as "non-cpu-local"
and silently ignores the device.

If we leave cpumask unset clockevents_register_device() will initialize it and
the cs5535-clockevt driver will be used no matter how the kernel was compiled.
Should anyone ever manage to stick a CS553x in an SMP system (is this even
possible?) then a warning will be printed. This is fine as the cs5535-clockevt
driver was never written/tested for SMP.

If bisecting led you here this patch may have exposed a pre-existing MFGPT
problem. Configure for UP-only and re-check.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>
Acked-by: Andres Salomon <dili...@queued.net>
---

Hi,

could you please take this, for linux-next?

Thanks,
Jens

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c

+++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
@@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
.set_mode = mfgpt_set_mode,
.set_next_event = mfgpt_next_event,
.rating = 250,
- .cpumask = cpu_all_mask,
.shift = 32
};

_

[Jens Rottmann]

cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

On SMP-capable kernels (e.g. generic distro kernel) the cs5535-clockevt driver
loads but is not actually used.

Setting cpumask to cpu_all_mask works for UP-only kernels, but if compiled for
SMP - though still running on the same UP hardware -
kernel/time/tick-common.c:tick_check_new_device() reads this as "non-cpu-local"
and silently ignores the device.

If we leave cpumask unset clockevents_register_device() will initialize it and
the cs5535-clockevt driver will be used no matter how the kernel was compiled.
Should anyone ever manage to stick a CS553x in an SMP system (is this even
possible?) then a warning will be printed. This is fine as the cs5535-clockevt
driver was never written/tested for SMP.

If bisecting led you here this patch may have exposed a pre-existing MFGPT
problem. Configure for UP-only and re-check.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>
Acked-by: Andres Salomon <dili...@queued.net>
---

Hi again,

could you please take this, for linux-next?

Thanks,
Jens

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
@@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
.set_mode = mfgpt_set_mode,
.set_next_event = mfgpt_next_event,
.rating = 250,
- .cpumask = cpu_all_mask,
.shift = 32
};

_

[Jens Rottmann]

cs5535-clockevt: allow the MFGPT IRQ to be shared

Shared timer IRQs are not a good solution, however the Geode platform has no
APIC, IRQs are a scarce resource and there is no technical reason to forbid it
rightaway. Increased latencies and overhead due to sharing are still better
than a driver refusing to load.

Signed-off-by: Jens Rottmann <JRot...@LiPPERTEmbedded.de>
Acked-by: Andres Salomon <dili...@queued.net>
---

Hi again,

could you please take this, for linux-next?

Thanks,
Jens

--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c

+++ allow_shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c

@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v

static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER | IRQF_SHARED,
.name = DRV_NAME,
};

_