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

Jump to bottom

arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled #1709

Merged
merged 2 commits into from
Sep 5, 2020
Merged

arch/sim: Fix the up_cpu_start initialisation on OSX with SMP config enabled #1709

merged 2 commits into from
Sep 5, 2020

Conversation

sebastianene07
Copy link

@sebastianene07 sebastianene07 commented Sep 4, 2020 •
edited
Loading

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 pthread_cond_t signalling instead of sempahores.

@sebastianene07 sebastianene07 force-pushed the sene/arch_sim_bugfix_unsupported_unnamed_semaphore_osx branch from 22090af to 31bc19b Compare September 4, 2020 09:33
xiaoxiang781216
xiaoxiang781216 reviewed Sep 4, 2020
View reviewed changes
arch/sim/src/nuttx-names.in Outdated
@@ -149,6 +149,9 @@ NXSYMBOLS(sched_yield)
NXSYMBOLS(seekdir)
NXSYMBOLS(select)
NXSYMBOLS(sem_destroy)
NXSYMBOLS(sem_open)
NXSYMBOLS(sem_close)
NXSYMBOLS(sem_unlink)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's order the list.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will order the list

arch/sim/src/sim/up_simsmp.c Outdated
@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

should we switch to named semp for all platform? it's hard to maintain two codebase.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's a good idea

arch/sim/src/sim/up_simsmp.c Outdated
ret = sem_init(&cpuinfo.done, 0, 0);

#ifdef CONFIG_HOST_MACOS
done_sema = sem_open(g_sema_name_done, O_CREAT, 0644, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The name need different for each cpu and for multiple nuttx instance

Copy link
Author

@sebastianene07 sebastianene07 Sep 4, 2020 •
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 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.

@xiaoxiang781216
Copy link
Contributor

After more reviewing, I think we can remove the semphare from the code since the synchronization isn't really needed.

@sebastianene07
Copy link
Author

sebastianene07 commented Sep 4, 2020 •
edited
Loading

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.

@sebastianene07 sebastianene07 force-pushed the sene/arch_sim_bugfix_unsupported_unnamed_semaphore_osx branch 2 times, most recently from 6495dad to 13a9893 Compare September 4, 2020 11:31
@yamt
Copy link
Contributor

yamt commented Sep 4, 2020

i suspect it's simpler to use pthread mutex/condvar.

@sebastianene07 sebastianene07 force-pushed the sene/arch_sim_bugfix_unsupported_unnamed_semaphore_osx branch from 13a9893 to 82e69d2 Compare September 4, 2020 12:30
@sebastianene07
Copy link
Author

i suspect it's simpler to use pthread mutex/condvar.

I updated the patch to use pthread_con_t signalling mechanism

@sebastianene07 sebastianene07 force-pushed the sene/arch_sim_bugfix_unsupported_unnamed_semaphore_osx branch 2 times, most recently from 9ea6eb4 to 8a880cd Compare September 4, 2020 12:39
Sebastian Ene added 2 commits September 4, 2020 16:06
arch/sim: Use pthread_cond for signalling CPU initialisation done
7ecd869 
  ## Summary of changes

On OSX with CONFIG_SMP=y the semaphore which notifies that the CPU is
initialised, is not created and the up_cpu_start() returns with error
from sem_init(). This patch fixes the problem by using pthread_cond_t
signalling mechanism which is supported on Mac.

Signed-off-by: Sebastian Ene <[email protected]>
arch/sim: Add the pthread_cond_* API to the nuttx-names.in list
a266317 
 ## Summary of changes

The pthread_cond_* API is also present as part of libfs.a and we want
to avoid colisions and link with the correct implementation.

Signed-off-by: Sebastian Ene <[email protected]>
@sebastianene07 sebastianene07 force-pushed the sene/arch_sim_bugfix_unsupported_unnamed_semaphore_osx branch from 8a880cd to a266317 Compare September 4, 2020 13:07
@xiaoxiang781216
Copy link
Contributor

LGTM.

@xiaoxiang781216 xiaoxiang781216 linked an issue Sep 4, 2020 that may be closed by this pull request
arch/sim: On OSX with SMP enabled the ostest hangs #1708
Closed
@xiaoxiang781216 xiaoxiang781216 merged commit 18b47f9 into apache:master Sep 5, 2020
@btashton btashton added this to To-Add in Release Notes - 10.0.0 Oct 14, 2020
@btashton btashton moved this from To-Add to Added in Release Notes - 10.0.0 Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Release Notes - 10.0.0
Added
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

arch/sim: On OSX with SMP enabled the ostest hangs
3 participants
@sebastianene07 @xiaoxiang781216 @yamt