Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)
Dmitry Adamushko wrote:
For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.
Good. Then let's go for
HANDLED, UNHANDLED - we may consider them even as 2 scalar values
+
NOENABLE, CHAINED - additional bits.
They are not encouraged to be used with shared interrupts
(explained in docs + debug messages when XENO_OPT_DEBUG is on).
Any ISR on the shared irq line should understand that it's
just one among the equals. That said, it should not do anything
that may affect other ISRs and not require any special treatment
(like CHAINED or NOENABLE).
If it wants it indeed, then don't declare itself as SHARED.
We may come back to the topic about possible return values of ISRs
a bit later maybe having got more feedback (hm.. hopefully)
on shared irq support.
But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).
And to support HANDLED_NOENABLE properly, those 2 have to be
decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt
asserted, hence we can't EOI at this point (unless NO_ENABLE means
DISABLE).
I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks.
Almost.
Let's consider the following only as anorther way of doing some things;
I don't propose to implement it, it's just to illustrate my thoughts.
So one may simply ski-skip-skip it :o)
Let's keep in mind that what is behind .end-ing the IRQ line depends on
archs.
Sometimes end == enable (EOI was sent on .ack step), while in other cases
end == send_EOI [+ re-enabling].
But anyway, all ISRs are running with a given IRQ line disabled.
Supported values : HANDLED, UNHANDLED, PROPAGATE.
nucleus :: xnintr_irq_handler()
{
ret = 0;
...
for each isr in isr_list[ IRQ ]
{
temp = isr-handler();
if (temp ret)
ret = temp;
}
if (ret == PROPAGATE)
{
// quite dengerous with shared interrupts, be sure you understand
// what you are doing!
xnarch_chain_irq(irq); // will be .end-ed in Linux domain
}
else
{
// HANDLED or UNHANDLED
xnarch_end_irq();
}
...
}
ENABLE or NOENABLE is missing? Nop.
let's say we have 2 rt-ISRs :
isr1 : HANDLED
isr2 : HANDLED + WISH
WISH == I want the IRQ line remain disabled until later
(e.g. bottom half in rt_task will enable it)
How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
support an internal counter that allows them to be called in a nested way.
So e.g. if there are 2 consecutive calls to disable_irq(), then
2 calls to enable_irq() are needed to really enable the IRQ line.
This way, the WISH is only about directly calling xnarch_irq_disable()
in isr2
and there is no need in ENABLE or NOENABLE flags.
This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in
Linux domain;
while WISH==NOENABLE - has nothing to do with sending EOI, but only with
enabling/disabling the given IRQ line.
Yes, if one isr (or a few) defers the IRQ line enabling until later, it
will affect
all others ISR = all interrupts are temporary not accepted on this line.
This scenario is possible under Linux, but should be used with even more
care in real-time system. At least, this way HANDLED_NOENABLE case works
and doesn't lead to lost interrupts on some archs.
Moreover, it avoids the need for ENABLE flag even in non-shared
interrupt case.
Ok, general bottom line regarding IRQ support (and the rest):
1- we want the rules applicable to the common case to be simple, well-defined and
straightforward. This applies to the ISR return value as exposed.
2- some requirements might fall outside of the common case; to support them, we
need to refrain from imposing a strong policy, but only provide sound mechanisms
to implement that. Specifically, let's keep the basic ability to control the Adeos
pipelining from Xenomai intact.
3- however, let's be true to the good old UNIX way-of-life: there is no point in
penalizing 99% of the common user base to please only 1% of them who happen to
have very specific needs. E.g., don't slow down the fast path everyone takes,
don't impose hard rules most of the users won't benefit from. [I'm not referring
to the enable/disable nesting count here: this _is_ correct, and is currently
missing -- this should be done at Xeno's arch-level, not from the HAL which should
be kept the way it is to have a direct, unmoderated access to the PIC handling].
IOW, the patch, the way you discussed it recently, is ok for me too.
--
Philippe.
[Xenomai-core] Xenomai patch packaging concerns
Hi, I understand that the prepare-kernel.sh script is a kind of smart patch program. It is very nice, but for packaging for Debian we need a real diff file. So I am currently modifying prepare-kernel.sh to be even smarter, so that it can optionally generate a diff file instead of actually modifying the Linux tree. This is done efficiently in terms of disk space, i.e. it does not duplicate the Linux tree to generate the diff file. I will send a patch for prepare-kernel.sh once I have finished. However, I have a problem: some files are copied (linked) several times into the Linux tree. As a consequence, in a generated diff file those files would be duplicated. And I believe that such a duplication is unecessary. Here are the sets of links that are currently made by prepare-kernel.sh (on every line: Linux tree - Xeno tree): 1- arch/arch/xenomai/* - ksrc/arch/arch/* 2- kernel/xenomai/* - ksrc/* 3- kernel/xenomai/arch/* - ksrc/arch/arch/* 4- kernel/xenomai/drivers/*... - ksrc/drivers/*... 5- kernel/xenomai/nucleus/*... - ksrc/nucleus/*... 6- kernel/xenomai/skins/*... - ksrc/skins/*... 7- drivers/xenomai/*... - ksrc/drivers/*... 8- include/asm-arch/xenomai/* - include/asm-arch/* 9- include/asm-generic/xenomai/* - include/asm-generic/* 10- include/xenomai/* - include/* 11- include/xenomai/asm-arch/* - include/asm-arch/* 12- include/xenomai/asm-generic/* - include/asm-generic/* ... 1- and 3- are redundant: I guess that 3- is not required? 4- and 7- are redundant: I guess that 4- is not required? 8- and 11- are redundant: I guess that 11- is not required? 9- and 12- are redundant: I guess that 12- is not required? I everybody agrees, I will modify prepare-kernel.sh to not generate the 3-, 4-, 11- and 12- sets of links. Note that removing the 3- and 4- sets of links would also require to modify the ksrc/Makefile file, so that the line: obj-$(CONFIG_XENOMAI) += arch/ nucleus/ skins/ becomes: obj-$(CONFIG_XENOMAI) += nucleus/ skins/ and the following line should be suppressed: subdir-$(CONFIG_XENOMAI) += arch Is that OK? It would even be better and easier to modify the Xenomai source tree to match better the Linux tree: ksrc/ L arch L drivers L kernel L nucleus L skins And I also will make prepare-kernel.sh not link (or generate diff) for the asm-arch and arch/arch/ directories for architectures that are different from the built architecture. -- Romain LENGLET
Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)
Dmitry Adamushko wrote:
For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.
Good. Then let's go for
HANDLED, UNHANDLED - we may consider them even as 2 scalar values
+
NOENABLE, CHAINED - additional bits.
They are not encouraged to be used with shared interrupts
(explained in docs + debug messages when XENO_OPT_DEBUG is on).
Any ISR on the shared irq line should understand that it's
just one among the equals. That said, it should not do anything
that may affect other ISRs and not require any special treatment
(like CHAINED or NOENABLE).
If it wants it indeed, then don't declare itself as SHARED.
We may come back to the topic about possible return values of ISRs
a bit later maybe having got more feedback (hm.. hopefully)
on shared irq support.
But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).
And to support HANDLED_NOENABLE properly, those 2 have to be
decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt
asserted, hence we can't EOI at this point (unless NO_ENABLE means
DISABLE).
I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks.
Almost.
Let's consider the following only as anorther way of doing some things;
I don't propose to implement it, it's just to illustrate my thoughts.
So one may simply ski-skip-skip it :o)
Let's keep in mind that what is behind .end-ing the IRQ line depends on
archs.
Sometimes end == enable (EOI was sent on .ack step), while in other cases
end == send_EOI [+ re-enabling].
But anyway, all ISRs are running with a given IRQ line disabled.
Supported values : HANDLED, UNHANDLED, PROPAGATE.
nucleus :: xnintr_irq_handler()
{
ret = 0;
...
for each isr in isr_list[ IRQ ]
{
temp = isr-handler();
if (temp ret)
ret = temp;
}
if (ret == PROPAGATE)
{
// quite dengerous with shared interrupts, be sure you understand
// what you are doing!
xnarch_chain_irq(irq); // will be .end-ed in Linux domain
}
else
{
// HANDLED or UNHANDLED
xnarch_end_irq();
}
...
}
ENABLE or NOENABLE is missing? Nop.
let's say we have 2 rt-ISRs :
isr1 : HANDLED
isr2 : HANDLED + WISH
WISH == I want the IRQ line remain disabled until later
(e.g. bottom half in rt_task will enable it)
How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
support an internal counter that allows them to be called in a nested way.
So e.g. if there are 2 consecutive calls to disable_irq(), then
2 calls to enable_irq() are needed to really enable the IRQ line.
This way, the WISH is only about directly calling xnarch_irq_disable()
in isr2
and there is no need in ENABLE or NOENABLE flags.
This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in
Linux domain;
while WISH==NOENABLE - has nothing to do with sending EOI, but only with
enabling/disabling the given IRQ line.
Yes, if one isr (or a few) defers the IRQ line enabling until later, it
will affect
all others ISR = all interrupts are temporary not accepted on this line.
This scenario is possible under Linux, but should be used with even more
care in real-time system. At least, this way HANDLED_NOENABLE case works
and doesn't lead to lost interrupts on some archs.
Moreover, it avoids the need for ENABLE flag even in non-shared
interrupt case.
Ok, general bottom line regarding IRQ support (and the rest):
1- we want the rules applicable to the common case to be simple, well-defined and
straightforward. This applies to the ISR return value as exposed.
2- some requirements might fall outside of the common case; to support them, we
need to refrain from imposing a strong policy, but only provide sound mechanisms
to implement that. Specifically, let's keep the basic ability to control the Adeos
pipelining from Xenomai intact.
3- however, let's be true to the good old UNIX way-of-life: there is no point in
penalizing 99% of the common user base to please only 1% of them who happen to
have very specific needs. E.g., don't slow down the fast path everyone takes,
don't impose hard rules most of the users won't benefit from. [I'm not referring
to the enable/disable nesting count here: this _is_ correct, and is currently
missing -- this should be done at Xeno's arch-level, not from the HAL which should
be kept the way it is to have a direct, unmoderated access to the PIC handling].
IOW, the patch, the way you discussed it recently, is ok for me too.
--
Philippe.
Re: [Xenomai-core] Xenomai patch packaging concerns
Romain Lenglet wrote: Hi, I understand that the prepare-kernel.sh script is a kind of smart patch program. It is very nice, but for packaging for Debian we need a real diff file. So I am currently modifying prepare-kernel.sh to be even smarter, so that it can optionally generate a diff file instead of actually modifying the Linux tree. This is done efficiently in terms of disk space, i.e. it does not duplicate the Linux tree to generate the diff file. I will send a patch for prepare-kernel.sh once I have finished. However, I have a problem: some files are copied (linked) several times into the Linux tree. As a consequence, in a generated diff file those files would be duplicated. And I believe that such a duplication is unecessary. Here are the sets of links that are currently made by prepare-kernel.sh (on every line: Linux tree - Xeno tree): 1- arch/arch/xenomai/* - ksrc/arch/arch/* 2- kernel/xenomai/* - ksrc/* 3- kernel/xenomai/arch/* - ksrc/arch/arch/* 4- kernel/xenomai/drivers/*... - ksrc/drivers/*... 5- kernel/xenomai/nucleus/*... - ksrc/nucleus/*... 6- kernel/xenomai/skins/*... - ksrc/skins/*... 7- drivers/xenomai/*... - ksrc/drivers/*... 8- include/asm-arch/xenomai/* - include/asm-arch/* 9- include/asm-generic/xenomai/* - include/asm-generic/* 10- include/xenomai/* - include/* 11- include/xenomai/asm-arch/* - include/asm-arch/* 12- include/xenomai/asm-generic/* - include/asm-generic/* ... 1- and 3- are redundant: I guess that 3- is not required? 4- and 7- are redundant: I guess that 4- is not required? 8- and 11- are redundant: I guess that 11- is not required? 9- and 12- are redundant: I guess that 12- is not required? I everybody agrees, I will modify prepare-kernel.sh to not generate the 3-, 4-, 11- and 12- sets of links. Note that removing the 3- and 4- sets of links would also require to modify the ksrc/Makefile file, so that the line: obj-$(CONFIG_XENOMAI) += arch/ nucleus/ skins/ becomes: obj-$(CONFIG_XENOMAI) += nucleus/ skins/ and the following line should be suppressed: subdir-$(CONFIG_XENOMAI) += arch Is that OK? Entering arch/ from the kernel section is needed to compile the generic HAL part; this is why 3- is currently needed. How would this fit into your new layout? Filtering out the rest looks ok though. It would even be better and easier to modify the Xenomai source tree to match better the Linux tree: ksrc/ L arch L drivers L kernel L nucleus L skins We could do that for later versions, but the generic HAL issue needs to be addressed first. And I also will make prepare-kernel.sh not link (or generate diff) for the asm-arch and arch/arch/ directories for architectures that are different from the built architecture. -- Philippe.
