You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/24 12:17:11 UTC
[GitHub] [incubator-nuttx] XinStellaris opened a new pull request, #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
XinStellaris opened a new pull request, #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512
driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Change-Id: I7e08763790f226ed3ebca5567ed77d698c258ca4
Signed-off-by: 田昕 <ti...@xiaomi.com>
## Summary
In our application, a long input string (approximately 1000bytes) will be read and printed. I need to figure out a way to make sure the prints are not garbled.
Since the input is long, I don't want to use CONFIG_SYSLOG_BUFFER and increase syslog buffer size to 1000+ bytes.
After some thought, sched_lock is my solution.
FYI, we chose Round-Robin schedule.
This pr also relates to https://github.com/apache/incubator-nuttx/issues/3599. I can't get answer from the discussion, but it is interesting and helpful to read it.
## Impact
Scheduler behavior when sysloging is influenced.
## Testing
Tested on Vela
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166234177
If the system does not have hard memory limitations, then maybe it is better to configure `CONFIG_SYSLOG_BUFFER=y` and set `CONFIG_IOB_BUFSIZE` to the maximum size of the output message. This I hope should solve an issue with multi-threaded syslog usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#discussion_r906701940
##########
drivers/syslog/vsyslog.c:
##########
@@ -79,6 +79,10 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
#endif
#endif
+#ifdef CONFIG_SYSLOG_SCHEDLOCK
+ sched_lock();
Review Comment:
sched_lock can't protect in SMP case, so you have to use critical section here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1165954738
> @acassis - it defaults off and has a note. Maybe a stronger note is needed to make it blatant for the uninformed user.
Yes, but this note is in the Help section only that few people read (exactly because NuttX is missing Help for many symbols). It is better to put some "Poka-yoke"-like protection to avoid some user/developer who will enables it thinking they are improving their system. This way the user only will enable it if he/she to fit in it to right way! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1274214376
after all the discussion, i think maybe its better to change how the application code uses syslog, by splitting long string into several lines and limiting the line size less than iob buffer size.
---Original---
From: "Xiang ***@***.***>
Date: Sun, Jun 26, 2022 00:39 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [apache/incubator-nuttx] driver/syslog:sched lock in syslog toprevent garbled print from multi-thread (PR #6512)
@xiaoxiang781216 commented on this pull request.
In drivers/syslog/vsyslog.c:
> @@ -79,6 +79,10 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap) #endif #endif +#ifdef CONFIG_SYSLOG_SCHEDLOCK + sched_lock();
sched_lock can't protect in SMP case, so you have to use critical section here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166307610
> Or maybe I can change our way of using syslog, rather than print 1000+ symbols per syslog, I can split a long string into several syslog calls. Maybe this is a better way?
>
> @pkarashchenko @acassis @davids5 what do you guys think?
If those prints should be read but the human I would print not more than 80 chars per line in total.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1165935446
@XinStellaris @davids5 I think this PR will jeopardize the deterministic behavior of NuttX and will prioritize log debug message. In this case NuttX is not long real-time. Maybe in this case you need to highlight to end user/developer that the system now is a cooperative OS. This PR is very dangerous, maybe it should depends on EXPERIMENTAL flag or some new (to be created COOPERATIVE_OS or NO_REALTIME).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris closed pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris closed pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
URL: https://github.com/apache/incubator-nuttx/pull/6512
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1168281601
After all the discussion, sched_lock or critical section in this pr will impact real-time behavior.
So I will change how the application calls syslog, by spliting long string into several lines and enable CONFIG_SYSLOG_BUFFER. At least I can make sure each line is not garbled when the printed line is not too long.
I will close this pr, hope this discussion benefits those who encounter similar problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166292225
> If the system does not have hard memory limitations, then maybe it is better to configure `CONFIG_SYSLOG_BUFFER=y` and set `CONFIG_IOB_BUFSIZE` to the maximum size of the output message. This I hope should solve an issue with multi-threaded syslog usage. without compromising real-time performance
We have 6K bytes for total IOB size. So it is too brutal to set CONFIG_IOB_BUFSIZE to 1000+. 1000 bytes of CONFIG_IOB_BUFSIZE means only about 6 IOBs can be allocated. 6 IOBs are not enough for the system, especially when we have TCP stack enabled and 2~3 connections are necessary at the same time.
Thats why I don't want to use CONFIG_SYSLOG_BUFFER and increase syslog buffer size to 1000+ bytes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1165944686
@acassis - it defaults off and has a note. Maybe a stronger note is needed to make it blatant for the uninformed user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166292513
> > >
> >
> >
> > Thanks for your review. Yes this will have impact on scheduler and needs to be dealt with carefully. But I am not sure under which case can this make the system not real-time. Sched_lock will not disable interrupts, though some work in hpwork will be delayed, they will be done after the print is finished. Maybe the delay of hpwork is what you are talking about? In RR schedule, this pr surely will bring in some delay for higher priority task, but the delay can be predicted from uart baud-rate and print length.
> > Am I understanding the system and real-time correctly?
>
> With `sched_lock` if the printing is done from a lower priority task, then higher priority will not start execution until the scheduler is unlocked so here we are loosing the real-time.
Exactly!!! I think even with priority inheritance the issue will happen!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166234940
> >
>
> Thanks for your review. Yes this will have impact on scheduler and needs to be dealt with carefully. But I am not sure under which case can this make the system not real-time. Sched_lock will not disable interrupts, though some work in hpwork will be delayed, they will be done after the print is finished. Maybe the delay of hpwork is what you are talking about? In RR schedule, this pr surely will bring in some delay for higher priority task, but the delay can be predicted from uart baud-rate and print length.
>
> Am I understanding the system and real-time correctly?
With `sched_lock` if the printing is done from a lower priority task, then higher priority will not start execution until the scheduler is unlocked so here we are loosing the real-time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166293727
> So you are printing 1000+ symbols per syslog call?
From time to time, syslog in out application will print 1000+ symbols. It happens when:
1. We recv a long string from wireless. About once in a minute.
2. We recv a long string from uart. This can happen at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a diff in pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on code in PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#discussion_r907397184
##########
drivers/syslog/Kconfig:
##########
@@ -373,4 +373,13 @@ config SYSLOG_CHARDEV
byte output with no time-stamping or any other SYSLOG features
supported.
+config SYSLOG_SCHEDLOCK
+ bool "SYSLOG schedlock (DANGER: may degrade NuttX Realtime Performance)"
Review Comment:
s/schedlock/sched_lock/ to make this greppable?
##########
drivers/syslog/Kconfig:
##########
@@ -373,4 +373,13 @@ config SYSLOG_CHARDEV
byte output with no time-stamping or any other SYSLOG features
supported.
+config SYSLOG_SCHEDLOCK
+ bool "SYSLOG schedlock (DANGER: may degrade NuttX Realtime Performance)"
+ default n
+ ---help---
+ Use schedlock to make sure syslogs from multiple threads are not garbled.
Review Comment:
s/schedlock/sched_lock/ here as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166292487
So you are printing 1000+ symbols per syslog call?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#discussion_r906422773
##########
drivers/syslog/Kconfig:
##########
@@ -373,4 +373,13 @@ config SYSLOG_CHARDEV
byte output with no time-stamping or any other SYSLOG features
supported.
+config SYSLOG_SCHEDLOCK
+ bool "SYSLOG schedlock"
Review Comment:
```suggestion
bool "SYSLOG schedlock (DANGER: may degrade NuttX Realtime Performance)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166298333
Or maybe I can change our way of using syslog, rather than print 1000+ symbols per syslog, I can split a long string into several syslog calls. Maybe this is a better way?
@pkarashchenko @acassis @davids5 what do you guys think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#discussion_r906701940
##########
drivers/syslog/vsyslog.c:
##########
@@ -79,6 +79,10 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
#endif
#endif
+#ifdef CONFIG_SYSLOG_SCHEDLOCK
+ sched_lock();
Review Comment:
sched_lock can't protect in SMP case, so you have to use critical section here: https://github.com/apache/incubator-nuttx/issues/3600
basically, all sched_lock which is used for lock need replace with critical section or spinlock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] XinStellaris commented on pull request #6512: driver/syslog:sched lock in syslog to prevent garbled print from multi-thread
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on PR #6512:
URL: https://github.com/apache/incubator-nuttx/pull/6512#issuecomment-1166159209
>
Thanks for your review. Yes this will have impact on scheduler and needs to be dealt with carefully.
But I am not sure under which case can this make the system not real-time. Sched_lock will not disable interrupts, though some work in hpwork will be delayed, they will be done after the print is finished. Maybe the delay of hpwork is what you are talking about?
In RR schedule, this pr surely will bring in some delay for higher priority task, but the delay can be predicted from uart baud-rate and print length.
Am I understanding the system and real-time correctly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org