You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "masayuki2009 (via GitHub)" <gi...@apache.org> on 2023/01/31 15:00:54 UTC
[GitHub] [nuttx] masayuki2009 opened a new pull request, #8383: fs: mqueue: Fix file_mq_open() for SMP
masayuki2009 opened a new pull request, #8383:
URL: https://github.com/apache/nuttx/pull/8383
## Summary
- I noticed that sometimes mqueue_test failed with rv-virt:smp.
- This commit fixes this issue.
## Impact
- SMP only
## Testing
- Tested with ostest on QEMU-7.1
--
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] [nuttx] xiaoxiang781216 merged pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8383:
URL: https://github.com/apache/nuttx/pull/8383
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8383:
URL: https://github.com/apache/nuttx/pull/8383#discussion_r1092122101
##########
fs/mqueue/mq_open.c:
##########
@@ -211,10 +214,15 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
/* Make sure that the check for the existence of the message queue
* and the creation of the message queue are atomic with respect to
* other processes executing mq_open(). A simple sched_lock() should
- * be sufficient.
+ * be sufficient for non-SMP case but critical section is needed for
+ * SMP case.
*/
+#ifdef CONFIG_SMP
+ flags = enter_critical_section();
+#else
Review Comment:
let' remove shed_lock and always use enter_critical_section?
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8383:
URL: https://github.com/apache/nuttx/pull/8383#discussion_r1092714339
##########
fs/mqueue/mq_open.c:
##########
@@ -211,10 +214,15 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
/* Make sure that the check for the existence of the message queue
* and the creation of the message queue are atomic with respect to
* other processes executing mq_open(). A simple sched_lock() should
- * be sufficient.
+ * be sufficient for non-SMP case but critical section is needed for
+ * SMP case.
*/
+#ifdef CONFIG_SMP
+ flags = enter_critical_section();
+#else
Review Comment:
BTW, I found that some place under fs/ still use sched_lock/sched_unlock, all should convert to inode_lock/inode_unlock I 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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8383:
URL: https://github.com/apache/nuttx/pull/8383#discussion_r1092712360
##########
fs/mqueue/mq_open.c:
##########
@@ -211,10 +214,15 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
/* Make sure that the check for the existence of the message queue
* and the creation of the message queue are atomic with respect to
* other processes executing mq_open(). A simple sched_lock() should
- * be sufficient.
+ * be sufficient for non-SMP case but critical section is needed for
+ * SMP case.
*/
+#ifdef CONFIG_SMP
+ flags = enter_critical_section();
+#else
Review Comment:
If the latency is a concern, the better approach is switching to lock by mutex(inode_lock/inode_unlock).
--
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] [nuttx] masayuki2009 commented on a diff in pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on code in PR #8383:
URL: https://github.com/apache/nuttx/pull/8383#discussion_r1092547460
##########
fs/mqueue/mq_open.c:
##########
@@ -211,10 +214,15 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
/* Make sure that the check for the existence of the message queue
* and the creation of the message queue are atomic with respect to
* other processes executing mq_open(). A simple sched_lock() should
- * be sufficient.
+ * be sufficient for non-SMP case but critical section is needed for
+ * SMP case.
*/
+#ifdef CONFIG_SMP
+ flags = enter_critical_section();
+#else
Review Comment:
@xiaoxiang781216
To reduce code maintenance costs, always using enter_critical_setion() would be better. However, using enter_critical_section() for non-SMP cases would increase interrupt latency since it disables interrupts. So sched_lock() would be better for non-SMP cases. What do you 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] [nuttx] masayuki2009 commented on a diff in pull request #8383: fs: mqueue: Fix file_mq_open() for SMP
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on code in PR #8383:
URL: https://github.com/apache/nuttx/pull/8383#discussion_r1092771782
##########
fs/mqueue/mq_open.c:
##########
@@ -211,10 +214,15 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
/* Make sure that the check for the existence of the message queue
* and the creation of the message queue are atomic with respect to
* other processes executing mq_open(). A simple sched_lock() should
- * be sufficient.
+ * be sufficient for non-SMP case but critical section is needed for
+ * SMP case.
*/
+#ifdef CONFIG_SMP
+ flags = enter_critical_section();
+#else
Review Comment:
@xiaoxiang781216
> let' remove shed_lock and always use enter_critical_section?
I will try this one first in this PR.
>BTW, I found that some place under fs/ still use sched_lock/sched_unlock, all should convert to inode_lock/inode_unlock I think.
Then I will study how to remove sched_lock under fs later.
--
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