Disable SYSLOG_DEFAULT by default #14695
Conversation
Because the implementation is problematic. See apache#14662 for the discussion. Also, make it depend on EXPERIMENTAL to discourage it. Instead, enable SYSLOG_CONSOLE for a bit more cases. An alternative would be to introduce/extend some serialization mechanism (eg. enter_critical_section, which is currently used by the serial driver to interact with interrupts) to cover up_putc users including SYSLOG_DEFAULT. While it works, it doesn't sound attractive to me to introduce this kind of complexity to up_putc, which is supposed to be a very low-level machinary used early in the boot. Also, the implementation of such serialization can be complex because how up_putc works is basically device-specific and how it corresponds to other devices on the system (eg. uarts) isn't obvious to the upper layers.
github-actions
bot
added
Area: Drivers
|
[Experimental Bot, please feedback here] No, this PR does not fully meet the NuttX requirements. Here's why:
In short, the PR needs significantly more detail in the Impact and Testing sections to meet the requirements. The Summary could also benefit from more specifics about the implementation of the |
| @@ -267,19 +267,25 @@ config SYSLOG_STREAM | |||
|
|
|||
| config SYSLOG_CONSOLE | |||
| bool "Log to /dev/console" | |||
| default !ARCH_LOWPUTC && !SYSLOG_CHAR && !RAMLOG_SYSLOG && !SYSLOG_RPMSG && !SYSLOG_RTT | |||
| default !SYSLOG_CHAR && !RAMLOG_SYSLOG && !SYSLOG_RPMSG && !SYSLOG_RTT | |||
There was a problem hiding this comment.
but console channel doesn't support the output from interrupt. This change will break most panic logic before console can be called from interrupt get fixed.
There was a problem hiding this comment.
it's a valid concern.
IMO, using syslog from interrupt is a design mistake.
anyway, it won't be a disaster as syslog_device.c already has a check for that case. (syslog_dev_outputready)
for panic, maybe it's enough to make it fall back to up_putc if g_nx_initstate == OSINIT_PANIC.
as the panic logic should have already stopped moving parts (eg. pause_all_cpu) at that point, it should be ok to use up_putc in that case.
how do you think?
There was a problem hiding this comment.
it's a valid concern.
IMO, using syslog from interrupt is a design mistake.
Why is it mistake? If syslog can't support the interrupt, what's other kernel facility could do?
anyway, it won't be a disaster as syslog_device.c already has a check for that case. (syslog_dev_outputready)
Different syslog channel has the different capability: uputc/ramlog/rpmsg support interrupt, console/device/file doesn't support. Since the default syslog(uputc) support the interrupt before, the change need keep this behavior, otherwise all board will break.
for panic, maybe it's enough to make it fall back to up_putc if g_nx_initstate == OSINIT_PANIC. as the panic logic should have already stopped moving parts (eg. pause_all_cpu) at that point, it should be ok to use up_putc in that case.
panic is just one case; driver may output log in interrupt too.
how do you think?
I think the default syslog channel need support the output from interrupt to handle all possible case. So, I would suggest improving syslog console to support the interrupt before making it as default. serial driver framework already contains the special code to handle the interrupt case, so there is no block issue to improve it.
There was a problem hiding this comment.
it's a valid concern.
IMO, using syslog from interrupt is a design mistake.Why is it mistake? If syslog can't support the interrupt, what's other kernel facility could do?
syslog is an api for ordinary applications.
it isn't free for an api to be prepared to be used by interrupts. eg. complex locking.
yes, it's simpler to have a separate api for interrupts, IMO.
anyway, it won't be a disaster as syslog_device.c already has a check for that case. (syslog_dev_outputready)
Different syslog channel has the different capability: uputc/ramlog/rpmsg support interrupt, console/device/file doesn't support. Since the default syslog(uputc) support the interrupt before, the change need keep this behavior, otherwise all board will break.
for panic, maybe it's enough to make it fall back to up_putc if g_nx_initstate == OSINIT_PANIC. as the panic logic should have already stopped moving parts (eg. pause_all_cpu) at that point, it should be ok to use up_putc in that case.
panic is just one case; driver may output log in interrupt too.
how do you think?
I think the default syslog channel need support the output from interrupt to handle all possible case. So, I would suggest improving syslog console to support the interrupt before making it as default. serial driver framework already contains the special code to handle the interrupt case, so there is no block issue to improve it.
have you read #14662?
do you prefer to sprinkle more serialization in syslog and/or up_putc?
There was a problem hiding this comment.
it's a valid concern.
IMO, using syslog from interrupt is a design mistake.Why is it mistake? If syslog can't support the interrupt, what's other kernel facility could do?
syslog is an api for ordinary applications. it isn't free for an api to be prepared to be used by interrupts. eg. complex locking.
if so, do you plan to remove the capability that sem can be posted and mqueue can be sent from interrupt handler?
These extensions which make POSIX API can work from interrupt context are compatible extension in OS context.
yes, it's simpler to have a separate api for interrupts, IMO.
You still need fulfill the requirement I said before that the new function need work in the interrupt context.
anyway, it won't be a disaster as syslog_device.c already has a check for that case. (syslog_dev_outputready)
Different syslog channel has the different capability: uputc/ramlog/rpmsg support interrupt, console/device/file doesn't support. Since the default syslog(uputc) support the interrupt before, the change need keep this behavior, otherwise all board will break.
for panic, maybe it's enough to make it fall back to up_putc if g_nx_initstate == OSINIT_PANIC. as the panic logic should have already stopped moving parts (eg. pause_all_cpu) at that point, it should be ok to use up_putc in that case.
panic is just one case; driver may output log in interrupt too.
how do you think?
I think the default syslog channel need support the output from interrupt to handle all possible case. So, I would suggest improving syslog console to support the interrupt before making it as default. serial driver framework already contains the special code to handle the interrupt case, so there is no block issue to improve it.
have you read #14662?
You can fix up_putc in what you want, but syslog can be called from the interrupt context must been keep as before until you provide a new api(e.g. printk) which can be called from the interrupt context and replace all syslog call in the code base.
do you prefer to sprinkle more serialization in syslog and/or up_putc?
I also want to drop up_putc from syslog too, but I have to point out that you can't simply switch the syslog channel to console by default since it breaks ALL log from panic and interrupt.
So, let me change the patch to draft until you find a way to fix this problem.
There was a problem hiding this comment.
syslog can be called from the interrupt context must been keep as before
why are you pretending like we have a working implementation of syslog which meets your requirements?
In production, we normally use ramlog or rpmsg syslog. In development, we use the default syslog with interrupt buffer.
you just don't care occasional deadlock during development? or, you are lucky and have not been suffered by the issue?
We don't hit the deadlock since the driver already do the protection.
because you don't care SMP?
No, we care SMP a lot (actually, all recent SMP improvement come from us, and more will come in), since we already chip more than 400 million products.
if so, do you plan to remove the capability that sem can be posted and mqueue can be sent from interrupt handler?
no. (at least for now.)
this PR just disables a functionality which is broken. (just because leaving it enabled hurts users. i received complaints from users.) if you want to use it and you don't care the breakage, you can still enable it.But the syslog console which can't be called from panic/assert/interrupt need be fixed or more another approach is provided before we merge this patch.
why? you can use ramlog or whatever as you said, can't you?
Yes, I can change to ramlog in my defconfig, and you can change to console in your defconfig too, but you can't change the default setting in master branch since all defconfig expect syslog can be called from panic/assert/interrupt.
but panic/assert need work in production.
it's a valid concern and i already proposed a possible solution earlier in this thread: #14695 (comment)
But we need an implementation, not just a proposal before switching.
your response to my proposal was just "panic is just one case; driver may output log in interrupt too." i thought you meant the approach was not acceptable at all for you. what's the point to write an implementation just to be dismissed?
I don't oppose this patch, but all current syslog in kernel need migrate to the interrupt safe approach before we merge this one.
There was a problem hiding this comment.
this PR just disables a functionality which is broken. (just because leaving it enabled hurts users. i received complaints from users.)
if you want to use it and you don't care the breakage, you can still enable it.
With this change you will have other users complaining why logging from interrupts doesn't work by default. Satisfying some users at the expense of others is not the solution, especially considering that there are more platform without SMP.
There was a problem hiding this comment.
syslog can be called from the interrupt context must been keep as before
why are you pretending like we have a working implementation of syslog which meets your requirements?
In production, we normally use ramlog or rpmsg syslog. In development, we use the default syslog with interrupt buffer.
you just don't care occasional deadlock during development? or, you are lucky and have not been suffered by the issue?
We don't hit the deadlock since the driver already do the protection.
which driver are you talking about?
you have a fix in your local tree, you mean?
or, in up_putc implementation for your target? (which target is it?)
because you don't care SMP?
No, we care SMP a lot (actually, all recent SMP improvement come from us, and more will come in), since we already chip more than 400 million products.
if so, do you plan to remove the capability that sem can be posted and mqueue can be sent from interrupt handler?
no. (at least for now.)
this PR just disables a functionality which is broken. (just because leaving it enabled hurts users. i received complaints from users.) if you want to use it and you don't care the breakage, you can still enable it.But the syslog console which can't be called from panic/assert/interrupt need be fixed or more another approach is provided before we merge this patch.
why? you can use ramlog or whatever as you said, can't you?
Yes, I can change to ramlog in my defconfig, and you can change to console in your defconfig too, but you can't change the default setting in master branch since all defconfig expect syslog can be called from panic/assert/interrupt.
IMO, the default should be something safe.
There was a problem hiding this comment.
We don't hit the deadlock since the driver already do the protection.
which driver are you talking about? you have a fix in your local tree, you mean? or, in up_putc implementation for your target? (which target is it?)
Yes, it's in the local tree, since the chip vendor doesn't want to upstream their code to community.
Yes, I can change to ramlog in my defconfig, and you can change to console in your defconfig too, but you can't change the default setting in master branch since all defconfig expect syslog can be called from panic/assert/interrupt.
IMO, the default should be something safe.
it depends on the context: SMP safe or interrupt safe. Since the default config is interrupt safe for a long time ago, we need keep this default behavior. So, I suggest making console syslog work in the interrupt context, which isn't hard to do, so we can switch to it and remove the default syslog channel directly.
There was a problem hiding this comment.
i submitted a (hopefully) less controversial alternative: #14722
|
@yamt do you want to continue:
or close this PR directly? |
for a longer term, i still tend to think we should retire up_putc at least for the syslog purpose. |
Summary
Disable SYSLOG_DEFAULT by default because the implementation is problematic.
See #14662 for the discussion. Also, make it depend on EXPERIMENTAL to discourage it.
Instead, enable SYSLOG_CONSOLE for a bit more cases.
An alternative would be to introduce/extend some serialization mechanism (eg. enter_critical_section, which is currently used by the serial driver to interact with interrupts) to cover up_putc users including SYSLOG_DEFAULT. While it works, it doesn't sound attractive to me to introduce this kind of complexity to up_putc, which is supposed to be a very low-level machinary used early in the boot. Also, the implementation of such serialization can be complex because how up_putc works is basically device-specific and how it corresponds to other devices on the system (eg. uarts) isn't obvious to the upper layers.
Impact
this would affect many board configurations.
Testing
tested for esp32s3-devkitc, with a few unrelated patches.