[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol

[GitBox <gi...@apache.org>]

zhaoxiu-zeng commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1002965955


   
   
   
   > > This will also break many things for downstream projects using NuttX.
   > 
   > Only when the project enable CONFIG_PRIORITY_INHERITANCE, otherwise no difference before and after apply this patch.
   > 
   > > We need to make sure the wider community notices this proposed change so it can be reviewed properly. I will post on the mailing list, but there are other venues, so to everyone frequenting them, please help draw attention to this there, too.
   > 
   > Sure.
   > 
   > > To be clear, I am in favor of standards-compliance, provided this is in line with NuttX's [Inviolables](https://github.com/hartmannathan/incubator-nuttx/blob/master/INVIOLABLES.md):
   > > 
   > > * If it is standard POSIX behavior to do as this change proposes, then we should do so, but only after making sure it is reviewed properly.
   > 
   > As far as I know, POSIX never mention the priority inheritance in spec, so it's the special behavior implemented by NuttX to improve the real time capability. But, sem_init without nxsem_set_protocol(SEM_PRIO_NONE) activate the priority inheritance by default which make the follow simple POSIX compliant program crash after the second sem_post:
   > 
   > ```
   > #include <pthread.h>
   > #include <semaphore.h>
   > #include <unistd.h>
   > 
   > static sem_t g_sem;
   > 
   > static void *thread1_cb(void *arg)
   > {
   >   sem_wait(&g_sem);
   >   return NULL;
   > }
   > 
   > static void *thread2_cb(void *arg)
   > {
   >   sleep(2);
   >   sem_wait(&g_sem);
   >   return NULL;
   > }
   > 
   > static void *thread3_cb(void *arg)
   > {
   >   sleep(1);
   >   sem_post(&g_sem);
   >   sleep(2);
   >   sem_post(&g_sem);
   >   return NULL;
   > }
   > 
   > int main(int argc, char *argv[])
   > {
   >   sem_init(&g_sem);
   > 
   >   thread1 = pthread_create(thread1_cb...);
   >   thread2 = pthread_create(thread2_cb...);
   >   thread3 = pthread_create(thread3_cb...);
   > 
   >   pthread_join(&thread1);
   >   pthread_join(&thread2);
   >   pthread_join(&thread3);
   > 
   >   sem_destroy(&g_sem);
   >   return 0;
   > }
   > ```
   
   After successfully sem_wait, the task becomes the semphore's holder. Then the task exit, but not be removed from the semphore's holder list, so it will crash if visit this holder in the future.
   I will do a commit to fix this problem.
   
   > 
   > So, it's very dangerous to enable priority inheritance without explicit indicator(e.g. a function call or an initial flag).
   > 
   > > * If this is Linux behavior and is in contradiction to POSIX, then we should **not** do it.
   > 
   > Of course.
   > 
   > > Also, if this change is implemented, we need to increment the major version number. It will have to be NuttX 11.x.
   > 
   > Sure.
   > 
   > > As mentioned elsewhere, there are likely other parts of our tree that need review/changes.
   > > Regarding whether we should do this or not, I cannot offer an opinion until I re-read POSIX details regarding semaphores to see if this is correct per our Inviolable Principles.
   > 
   > Ok, let's wait your research.
   
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org