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 2020/09/04 09:25:25 UTC
[GitHub] [incubator-nuttx] sebastianene07 opened a new pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
sebastianene07 opened a new pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709
## Summary
This PR fixes the issue described in #1708 . On Mac the `sem_init(..)` host call returns with error as unnamed semaphores are not supported. Because of this, the call to `up_cpu_start` fails and the simulation hangs.
The patch fixes this issue by using named semaphores when `CONFIG_HOST_MACOS=y`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687069166
> After more reviewing, I think we can remove the semphare from the code since the synchronization isn't really needed.
Ok I will remove it, in that case I will also drop the changes from `nuttx-names.in`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687065502
After more reviewing, I think we can remove the semphare from the code since the synchronization isn't really needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#discussion_r483552249
##########
File path: arch/sim/src/nuttx-names.in
##########
@@ -149,6 +149,9 @@ NXSYMBOLS(sched_yield)
NXSYMBOLS(seekdir)
NXSYMBOLS(select)
NXSYMBOLS(sem_destroy)
+NXSYMBOLS(sem_open)
+NXSYMBOLS(sem_close)
+NXSYMBOLS(sem_unlink)
Review comment:
Ok I will order the list
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687113913
> i suspect it's simpler to use pthread mutex/condvar.
I updated the patch to use pthread_con_t signalling mechanism
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#discussion_r483530463
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -53,7 +54,11 @@
struct sim_cpuinfo_s
{
int cpu; /* CPU number */
+#ifdef CONFIG_HOST_MACOS
+ sem_t *done;
+#else
sem_t done; /* For synchronization */
Review comment:
should we switch to named semp for all platform? it's hard to maintain two codebase.
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -324,11 +344,22 @@ int up_cpu_start(int cpu)
/* Initialize the CPU info */
cpuinfo.cpu = cpu;
- ret = sem_init(&cpuinfo.done, 0, 0);
+
+#ifdef CONFIG_HOST_MACOS
+ done_sema = sem_open(g_sema_name_done, O_CREAT, 0644, 0);
Review comment:
The name need different for each cpu and for multiple nuttx instance
##########
File path: arch/sim/src/nuttx-names.in
##########
@@ -149,6 +149,9 @@ NXSYMBOLS(sched_yield)
NXSYMBOLS(seekdir)
NXSYMBOLS(select)
NXSYMBOLS(sem_destroy)
+NXSYMBOLS(sem_open)
+NXSYMBOLS(sem_close)
+NXSYMBOLS(sem_unlink)
Review comment:
let's order the list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#discussion_r483552797
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -324,11 +344,22 @@ int up_cpu_start(int cpu)
/* Initialize the CPU info */
cpuinfo.cpu = cpu;
- ret = sem_init(&cpuinfo.done, 0, 0);
+
+#ifdef CONFIG_HOST_MACOS
+ done_sema = sem_open(g_sema_name_done, O_CREAT, 0644, 0);
Review comment:
It's a good catch, thanks !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 edited a comment on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 edited a comment on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687069166
> After more reviewing, I think we can remove the semphare from the code since the synchronization isn't really needed.
~~Ok I will remove it, in that case I will also drop the changes from `nuttx-names.in`~~ I think we need this synchronization after a careful look I ended up in a bad situation where : I received a signal(SIGUSR1) in a thread that didn't end up setting the g_cpu_key.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687135148
LGTM.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#issuecomment-687090262
i suspect it's simpler to use pthread mutex/condvar.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#discussion_r483552625
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -53,7 +54,11 @@
struct sim_cpuinfo_s
{
int cpu; /* CPU number */
+#ifdef CONFIG_HOST_MACOS
+ sem_t *done;
+#else
sem_t done; /* For synchronization */
Review comment:
I think it's a good idea
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1709: arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1709:
URL: https://github.com/apache/incubator-nuttx/pull/1709#discussion_r483552797
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -324,11 +344,22 @@ int up_cpu_start(int cpu)
/* Initialize the CPU info */
cpuinfo.cpu = cpu;
- ret = sem_init(&cpuinfo.done, 0, 0);
+
+#ifdef CONFIG_HOST_MACOS
+ done_sema = sem_open(g_sema_name_done, O_CREAT, 0644, 0);
Review comment:
It's a good catch, thanks ! This will not work when we have multiple NuttX instances running. Currently it will work because the thread creation is sequential and we destroy the named semaphore after each CPU initialisation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org