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/12 19:15:57 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request, #6414: mutex: Revert part of the changes introduced by cea7953a5623bd48ce1188ce7db9017f92dc0504
pkarashchenko opened a new pull request, #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414
## Summary
Revert part of the changes introduced by cea7953a5623bd48ce1188ce7db9017f92dc0504 dues to deadlock in syslog and introduction of cancellation points in kernel for FLAT mode
## Impact
Fix syslog
## Testing
Pass CI
--
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 #6414: drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#issuecomment-1153861541
@davids5 done. Anyway I was trying to address another part in https://github.com/apache/incubator-nuttx/pull/6376
I think we need to revert libc related changes anyway and then think about better solution.
I found out that currently we have even bigger gap since in case of FLAT build the `_SEM_` macro interface is used also for dynamic memory allocation, so `malloc` and `free` become cancellation points. I do not have a solid solution for that, but think that we should not rock the boat with changes like https://github.com/apache/incubator-nuttx/pull/6320 introducing even more inconsistency.
IMO we need to revert another part of the changes (that I removed from this PR) and spend some time on a design review so then it will be possible to come up with a solid solution.
--
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 #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#issuecomment-1153796273
>Let's restore things to a working state and then do a single change for the syslog update.
I was going to suggest spitting this into 2 PR. A revert and a fix for syslog is that what you are saying?
It appears to me that some of the sweeping changes on master have destabilized the project. I would revert all the changes on master and work on an upstream branch until the all the issues are addressed and tested. I would also strongly suggest that future changes like these be done on upstream branches and tested before merging.
@pkarashchenko - you are closer to the problem than I am so I leave it up to you make the call on this.
--
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 #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#issuecomment-1153757846
@davids5 the deadlock is only caused by changes in `drivers/syslog/syslog_device.c`.
Initially the syslog take semaphore was returning error code if semaphore is already held.
Other changes are related to introduction of cancellation points in kernel for FLAT build
--
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 merged pull request #6414: drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414
--
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 #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#issuecomment-1153752476
@pkarashchenko On looking at this revert and the replacement. At first blush it looks like semaphore based code does exactly what the rmutex added. Would you please enumerate the difference and the root cause of the deadlock?
--
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 a diff in pull request #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#discussion_r895594941
##########
drivers/syslog/syslog_device.c:
##########
@@ -120,6 +123,63 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: syslog_dev_takesem
+ ****************************************************************************/
+
+static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
Review Comment:
Let's restore things to a working state and then do a single change for the syslog update.
--
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 #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#issuecomment-1153805071
Ok. I will split the change into 2 PRs
--
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 a diff in pull request #6414: drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#discussion_r895667526
##########
drivers/syslog/syslog_device.c:
##########
@@ -120,6 +123,63 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: syslog_dev_takesem
+ ****************************************************************************/
+
+static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
Review Comment:
@xiaoxiang781216 I'm just reverting a part that was changed. No new logic is introduced 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] xiaoxiang781216 commented on a diff in pull request #6414: mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6414:
URL: https://github.com/apache/incubator-nuttx/pull/6414#discussion_r895593423
##########
drivers/syslog/syslog_device.c:
##########
@@ -120,6 +123,63 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: syslog_dev_takesem
+ ****************************************************************************/
+
+static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
Review Comment:
it's better to add new function to check whether the caller already hold the lock and then return the error directly. Since syslog_device.c is part of kernel, it isn't good to dup the same logic again.
--
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