Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable SYSLOG_DEFAULT by default #14695

Closed
wants to merge 1 commit into from
Closed

Disable SYSLOG_DEFAULT by default #14695

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions drivers/syslog/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@yamt yamt Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i submitted a (hopefully) less controversial alternative: #14722

depends on DEV_CONSOLE
select SYSLOG_REGISTER
---help---
Use the system console as a SYSLOG output device.

config SYSLOG_DEFAULT
bool "Default SYSLOG device"
default ARCH_LOWPUTC && !SYSLOG_CHAR && !RAMLOG_SYSLOG && !SYSLOG_RPMSG && !SYSLOG_RTT && !SYSLOG_CONSOLE
default n
depends on EXPERIMENTAL
---help---
syslog() interfaces will be present, but all output will go to the
up_putc(ARCH_LOWPUTC == y) or bit-bucket(ARCH_LOWPUTC == n).

CAVEAT: The current implementation of this functionality might
interfere the other activities on the console device, especially
with SMP. (Thus marked EXPERIMENTAL.)
See https://github.com/apache/nuttx/issues/14662.

endif

if RAMLOG_SYSLOG
Expand Down
Loading