Skip to content

nuttx/mutex: do not use non-nx interface in kernel except libs#6376

Closed
pkarashchenko wants to merge 1 commit into
apache:masterfrom
pkarashchenko:fix_nxmutex
Closed

nuttx/mutex: do not use non-nx interface in kernel except libs#6376
pkarashchenko wants to merge 1 commit into
apache:masterfrom
pkarashchenko:fix_nxmutex

Conversation

@pkarashchenko

Copy link
Copy Markdown
Contributor

Summary

The #6320 redirected nxmutex used in kernel to sem_ API family in case of CONFIG_BUILD_FLAT. That is not correct. This PR is created just for discussion and just highlight the issue.

Impact

Testing

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 could you please take a look. I noticed during my work that mutexes use sem_ API inside the kernel in case of FLAT build. Seems that we missed that point during the review of #6320 by trying to overcome the libc build error and switching from nxsem_ to _SEM_. This introduced a side effect in case of FLAT build.

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

One of the possible ways may be to get back original code for the files that I've listed in this PR and switching nxmutex_ APIs to use nxsem_ APIs only.

@pkarashchenko pkarashchenko force-pushed the fix_nxmutex branch 2 times, most recently from c1774ec to 40943e8 Compare June 7, 2022 05:18
@xiaoxiang781216

xiaoxiang781216 commented Jun 7, 2022

Copy link
Copy Markdown
Contributor

One of the possible ways may be to get back original code for the files that I've listed in this PR and switching nxmutex_ APIs to use nxsem_ APIs only.

How about we follow sem_t approach:

  • include/mutex.h which call sem_xxx and set errno
  • include/nuttx/mutex.h which call nxsem_xxx but not set errno
  • add MUTEX_XXXX macro in include/nuttx/mutex.h which call first or second version by check __KERNEL__

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

Yes. This can work. Similar approach like _SEM_XXX

@pkarashchenko pkarashchenko force-pushed the fix_nxmutex branch 3 times, most recently from 7df8128 to 5bd3a5a Compare June 7, 2022 09:05
@pkarashchenko

Copy link
Copy Markdown
Contributor Author

One of the possible ways may be to get back original code for the files that I've listed in this PR and switching nxmutex_ APIs to use nxsem_ APIs only.

How about we follow sem_t approach:

* include/mutex.h which call sem_xxx and set errno

* include/nuttx/mutex.h which call nxsem_xxx but not set errno

* add MUTEX_XXXX macro in include/nuttx/mutex.h which call first or second version by check `__KERNEL__`

Done, but IMO we should merge mutex_t and rmutex_t (as well as APIs) and just add bool recursive; flag to mutex_t and just keep one set of APIs

Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
@pkarashchenko

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 seems that mutex_ notation conflicts with 3P component naming. Since mutex_ is not a POSIX APIs I'm not sure what is the best way to solve the naming conflict. mtx_ prefix is already occupied (include/threads.h). What do you think, how we can resolve the naming conflict?

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

The issue of naming conflict comes from: #include <stdio.h> -> #include <nuttx/fs/fs.h> -> #include <nuttx/mutex.h> because struct file_struct has member rmutex_t fs_lock;

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 ping

Comment thread include/mutex.h

/* Initializers */

#define MUTEX_INITIALIZER {SEM_INITIALIZER(1),MUTEX_NO_HOLDER}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add space after ,

Comment thread include/mutex.h
/* Initializers */

#define MUTEX_INITIALIZER {SEM_INITIALIZER(1),MUTEX_NO_HOLDER}
#define RMUTEX_INITIALIZER {MUTEX_INITIALIZER,0}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add space after ,

Comment thread include/mutex.h
struct mutex_s
{
sem_t sem;
pid_t holder;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why need save pid in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In general mutex should have the holder stored and do not allow unlock not from a holder thread. We can even DEBUGASSERT if we try to unlock from not a mutex holder

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since mutex is used very often in the code base. How about we enable this for debugging only?

@pkarashchenko pkarashchenko Jun 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean check for normal mutex? The recursive need to store a holder

@xiaoxiang781216

Copy link
Copy Markdown
Contributor

The issue of naming conflict comes from: #include <stdio.h> -> #include <nuttx/fs/fs.h> -> #include <nuttx/mutex.h> because struct file_struct has member rmutex_t fs_lock;

It's hard to fix:(

@zyfeier

zyfeier commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

@pkarashchenko Hi, pkarashchenko, is this PR still being modified?

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

I will review and most probably close this PR as it is outdated

@zyfeier

zyfeier commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

@pkarashchenko, i have a similar modify, please help to review. #8645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants