Hello, Migration to nxmutex is quite a big effort and unfortunately recently I didn't have much time to deep dive into this. In general I support an initiative and do not see a use case for priority inheritance for regular semaphores, so I think we should clean-up priority inheritance code for the regular signalling semaphores and introduce a new kernel object (mutex) instead. This is surely valid for the kernel, but not for the user space that already has pthread_mutex with priority inheritance option, so I do not see anything is needed for user space.
I see a few areas where the problems may appear and just want to make sure that next areas are analyzed and tested with each migration step: 1. Cancellation points and FLAT mode. Since we are having only one copy of a LIBC here we need to decide what object to use, so all interfaces that are a cancellation point will still work as before 2. If we start to base pthread objects on nxmutex then we need to make sure that cancellation point operation is not broken, For example rw_lock API are cancellation points. The optimization of priority inheritance operation is welcomed. Best regards, Petro пт, 31 бер. 2023 р. о 07:10 Tomek CEDRO <to...@cedro.info> пише: > On Fri, Mar 31, 2023 at 12:23 AM Gregory Nutt wrote: > > > > > In his Confluence paper on "Signaling Semaphores and Priority > > Inheritance”, Brennan Ashton’s analysis is both thorough and accurate; > ... > > > > Minor fix. I wrote the paper, Brennan converted the Confluence page > > from an older DocuWiki page > > Respect :-) > > > > > The solution Brennan suggests is to initialize semaphores used as > > signaling events as follows: > > > sem_init(&sem, 0, 0); > > > sem_setprotocol(&sem, SEM_PRIO_NONE); > > > > > > this is, of course, correct, but retains the dual purpose of > > sem_wait() and sem_post(). I believe this can be confusing and will > > continue to be a source of subtle errors. I suggest going a step > > further and isolate the two use cases. Let the current sem_init, > > sem_wait, sem_post be used only for the resource locking use case. > > > > > > For the signaling use case, create a new API for event signaling > > within the kernel: nxev_init, nxev_wait, nxev_post where: nxev_init is > > simply: > > > sem_init(&nxev, 0, 0); > > > sem_setprotocol(&nxev, SEM_PRIO_NONE); > > > > > > and: > > > #define nxev_wait sem_wait > > > #define nxev_post sem_post > > > > > > In the case were PRIORITY_INHERITANCE is not configured, > > sem_setprotocol() does nothing and the nxev_*() API is still used for > > event notification. > > > > > > This may seem a trivial change, but having specific API function > > names for the two specific use cases would, I believe, all but eliminate > > future confusion; especially given that most people look to existing > > drivers to use as a template. Finally, enabling or disabling > > PRIORITY_INHERITANCE would not introduce the subtle error Brennan > > documented. > > > > Your suggestion would clarify the usage. > > > > I was thinking out a conceptually simple solution that should also > > resolve the risk in usage: Just change the default state to > > SEM_PRIO_NONE for all semaphores. That would make the default protocol > > for semaphore be no priority inheritance. > > > > This would be a lot of work, however. All occurrences of sem_init() > > would have to be examined: > > > > For the signaling use case, you would do nothing. We would have to > > remove all sem_setprotocol(&nxev, SEM_PRIO_NONE); > > For the resource locking use case, you would have to add > > sem_setprotocol(&nxev, SEM_PRIO_INHERIT); assuming priority inheritance > > is enabled. > > > > The eliminates the risk of inappropriately using priority inheritance on > > a locking semaphore. The consequences of doing that are very bad: > > > https://nuttx.yahoogroups.narkive.com/3hggphCi/problem-related-semaphore-and-priority-inheritance > > > > Then the only error that the user can make is to fail to select priority > > inheritance when it would do good. That is a lesser error and, as you > > note, usually OK. > > +1 :-) > > -- > CeDeROM, SQ7MHZ, http://www.tomek.cedro.info >
