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.

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

ostest:add sem protocl to priority inheritance #1358

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

anjiahao1
Copy link
Contributor

@anjiahao1 anjiahao1 commented Oct 18, 2022

Summary

ostest,sem use lock need set protocl SEM_PRIO_INHERIT
patch

impact

use sem as lock,need do is change

Testing

ostest

Comment on lines 532 to +533
sem_init(&g_sem, 0, NLOWPRI_THREADS);
sem_setprotocol(&g_sem, SEM_PRIO_INHERIT);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not change to nxmutex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ostest is app,nxmutex is kernal,so we need add mutex for app?

Copy link
Contributor

Choose a reason for hiding this comment

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

since this is used to test the api usage, it's better to call the core api directly instead of wrapper.

@masayuki2009
Copy link
Contributor

masayuki2009 commented Oct 18, 2022

@anjiahao1 @xiaoxiang781216
Do we need such a change if we enable the priority inheritance feature?

ostest testcase use sem as lock
need set protocl
Signed-off-by: anjiahao <[email protected]>
@anjiahao1
Copy link
Contributor Author

@anjiahao1 @xiaoxiang781216 Do we need such a change if we enable the priority inheritance feature?

if sem used as lock,need set protocl to priority inheritance, others do not need

@xiaoxiang781216
Copy link
Contributor

@masayuki2009 @pkarashchenko do you have more concern about this change?

@pkarashchenko
Copy link
Contributor

@masayuki2009 @pkarashchenko do you have more concern about this change?

This is a case of counting semaphore with priority inheritance enabled. We discussed this apache/nuttx#6965 (comment) . So the question that I raise long time in the past comes again: Should priority inheritance be an option of semaphore or mutex only. In this case I'll rephrase it: Should we allow enabling of priority inheritance for non-binary semaphores. If yes, then we probably need to update many places in code with similar changes.

@xiaoxiang781216
Copy link
Contributor

@masayuki2009 @pkarashchenko do you have more concern about this change?

This is a case of counting semaphore with priority inheritance enabled.

Yes, but ostest is very special one, because it verify the internal detail of OS implementation, and understanding the real behavior change between the enable and disable of priority inheritance.

We discussed this apache/incubator-nuttx#6965 (comment) . So the question that I raise long time in the past comes again: Should priority inheritance be an option of semaphore or mutex only. In this case I'll rephrase it: Should we allow enabling of priority inheritance for non-binary semaphores. If yes, then we probably need to update many places in code with similar changes.

I don't believe that the non-binary semaphore can work well with the priority inheritance in general. The priority inheritance is meaningful only when:

  1. The semaphore has the binary value(0 or 1)
  2. The semaphore is used as lock

@pkarashchenko
Copy link
Contributor

Yeah, currently with CONFIG_SEM_PREALLOCHOLDERS > 2 this will work. I mean that the current implementation with some configuration allows priority inheritence on counting semaphores and really do not makes sense to me, but to unblock further changes.
IMO anyway we need to make a kernel implementation of mutex objects instead of making a semaphore wrapper, so application will use pthread_mutex and kernel will use nxmutex, but that may never happen since many things like mutex usage in libc in FLAT build are not clear.

@masayuki2009
Copy link
Contributor

Let me merge this PR since apache/nuttx#5070 has been merged.

@masayuki2009 masayuki2009 merged commit 65668b3 into apache:master Oct 22, 2022
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.

4 participants