You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/12/24 02:58:01 UTC
[GitHub] [incubator-nuttx] anjiahao1 opened a new pull request #5070: libc/semaphore:sem_init change defult protocol
anjiahao1 opened a new pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070
Signed-off-by: anjiahao <an...@xiaomi.com>
Semaphores are generally used in two ways, one is to protect shared resources, the other is to use as a mutual exclusion lock
In Nuttx, the default behavior of semaphores is mutual exclusion lock. I think it is better to change the default behavior to protect shared resources, and it will be more general in Linux programs.
In nuttx, there are many calls to nxsem_init, this is an example.
Why would I do this:
In porting some Linux programs, the semaphore is used to protect resources, but its default signal is a mutual exclusion lock（nuttx）. When two threads are waiting for the semaphore, one of the threads gets the semaphore and exits, which will cause another A holder that accesses the semaphore will cause a crash, and the default behavior is used to protect resources, so the holder will not be added
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005555900
Here is a real case: https://github.com/apache/incubator-nuttx-apps/pull/960
--
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
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#discussion_r775057248
##########
File path: include/semaphore.h
##########
@@ -47,8 +47,8 @@
/* Bit definitions for the struct sem_s flags field */
-#define PRIOINHERIT_FLAGS_DISABLE (1 << 0) /* Bit 0: Priority inheritance
- * is disabled for this semaphore. */
+#define PRIOINHERIT_FLAGS_ENABLE (1 << 0) /* Bit 0: Priority inheritance
+ * is enable for this semaphore. */
Review comment:
s/enable/enabled/
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005555900
Here is a case: https://github.com/apache/incubator-nuttx-apps/pull/960
--
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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1002780625
Seems that disabling priority inheritance by default is the right way of doing things. My arguments are that by default priority inheritance is used to struggle with resource guarding issue (mutual exclusion use case) and does not have much sense in case of event signaling use case. The POSIX semaphores however have have more wide range then mutual exclusion use case and it is more a coincidence that `phtread_mutex`s are implemented on top of POSIX semaphores in NuttX (I just want to emphasize that `phtread_mutex`s could have their own implementation as an alternative and are not tight together with semaphores in general). `pthread_mutex`s will still have priority inheritance enabled by default and that is fine because I truly believe that when NuttX user enables priority inheritance that is the exactly use case that needs to be handled by this feature.
Summarizing above: my vote is to change the default protocol for POSIX semaphores and proceed with this change.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1002780625
Seems that disabling priority inheritance by default seems to be the right way of doing things. My arguments are that by default priority inheritance is used to struggle with resource guarding issue (mutual exclusion use case) and does not have much sense in case of event signaling use case. The POSIX semaphores however have have more wide range then mutual exclusion use case and it is more a coincidence that `phtread_mutex`s are implemented on top of POSIX semaphores in NuttX (I just want to emphasize that `phtread_mutex`s could have their own implementation as an alternative and are not tight together with semaphores in general). `pthread_mutex`s are will still have priority inheritance enabled by default and that is fine because I truly believe that when NuttX user enables priority inheritance that is the exactly use case that needs to be handled by this feature.
Summarizing above: my vote is to change the default protocol for POSIX semaphores and proceed with this change.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003017113
@anjiahao1 has a simple fix which change tcb_s to pid, so we can detect the abnormal case and remove the orphan node easily,
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005603855
Another real case: https://github.com/apache/incubator-nuttx/pull/5170, both case will generate hard fault.
--
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
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000898688
This will also break many things for downstream projects using NuttX.
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.
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.
- If this is Linux behavior and is in contradiction to POSIX, then we should **not** do it.
Also, if this change is implemented, we need to increment the major version number. It will have to be NuttX 11.x.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003000531
@zhaoxiu-zeng how do you plan to fix this? I was looking into the code as well and was looking for a solution, but didn't found a lightweight solution. The code only idea that I had was to create a global list of semaphores that have at least one holder and iterate that list on task exit in order to find a semaphore that contain task in the holder list or create a list of semaphores in task control block and add semaphore to that list when task is added as a semaphore holder. But both seems to me quite "heavy" in terms of both memory and performance.
--
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005623759
> Another real case: #5170, both case will generate hard fault.
I run ostest with https://github.com/apache/incubator-nuttx/pull/5171, no such assertions were found.
I think priority inheritance should only be valid for mutex, so there is only one holder per semphore at most.
The current implementation still cannot avoid priority flipping completely.
For example, there are 3 tasks t2 > t1 > t0, t0 held sem1, t1 held sem2 and wait sem1, then t2 wait sem2. In this case, only t1's priority is raised.
If multiple holders are supported, this problem is very difficult to solve, which is the O(n^2) level.
If only one holder is supported, many things can be simplified.
--
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005610175
>
I have pushed the modificaiton, let's discuss in the PR.
https://github.com/apache/incubator-nuttx/pull/5171
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#discussion_r774856005
##########
File path: libs/libc/semaphore/sem_init.c
##########
@@ -74,7 +74,7 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
/* Initialize to support priority inheritance */
#ifdef CONFIG_PRIORITY_INHERITANCE
- sem->flags = 0;
+ sem->flags |= PRIOINHERIT_FLAGS_DISABLE;
Review comment:
change to:
```
sem->flags = PRIOINHERIT_FLAGS_DISABLE;
```
or change `PRIOINHERIT_FLAGS_DISABLE` to `PRIOINHERIT_FLAGS_ENABLE`
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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 sem_set_protocol(SEM_NONE) activate the priority inheritance by default which make the follow common POSIX complaint program crash after the second post:
```
sem_t g_sem;
void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
void *thread2_cb(void *arg)
{
sleep(2);
set_wait(&g_sem);
return NULL;
}
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);
}
```
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000980875
> Drivers that use the semaphores for signaling should not be involved with priority inheritance (when enabled). That use case was assumed to be the non default.
Why non default? semaphore use as signaling is a normal practice in POSIX. On the other hand, most program use POSIX pthread mutex as lock, so semaphore seldom use as lock except NuttX kernel.
I have to say that it's very bad choice to enable priority inheritance by default when priority inheritance get implemented, because:
1. It's an optional feature, which mean that the code which use sem_t but not call nxsem_set_protocol(NuttX specific), the behavior will change with/without enable this feature.
2. Since priority inheritance is the default setting, which mean the wrong usage will crash the system. It's more serious than the priority inversion.
>
> It seams to me this change can be done with adding an Kconfig option for the default and leave it as it was.
Kconfig is good to disable/enable the optional feature, but it isn't a good solution here. Do you want sem_t change it's behavior by an option? And all place which call sem_init/nxsem_init need check the option and call nxsem_set_protocol with the different value.
> But allow the default to be changed if someone is using Linux code semantics.
First, it isn't Linux semantics. Second, my demo show that isn't a minor issue because the program will crash the system.
--
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
[GitHub] [incubator-nuttx] hartmannathan edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
hartmannathan edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000898688
This will also break many things for downstream projects using NuttX.
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.
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.
- If this is Linux behavior and is in contradiction to POSIX, then we should **not** do it.
Also, if this change is implemented, we need to increment the major version number. It will have to be NuttX 11.x.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#discussion_r774857229
##########
File path: drivers/input/tsc2007.c
##########
@@ -1228,6 +1228,7 @@ int tsc2007_register(FAR struct i2c_master_s *dev,
priv->config = config; /* Save the board configuration */
nxsem_init(&priv->devsem, 0, 1); /* Initialize device structure semaphore */
+ nxsem_set_protocol(&priv->devsem, SEM_PRIO_INHERIT);
Review comment:
let remove nxsem_set_protocol(&priv->waitsem, SEM_PRIO_NONE) at line 1238
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#discussion_r774944602
##########
File path: drivers/input/tsc2007.c
##########
@@ -1234,7 +1234,7 @@ int tsc2007_register(FAR struct i2c_master_s *dev,
* have priority inheritance enabled.
*/
Review comment:
/* The event dev semaphore is used for lock, hence, should have priority inheritance enabled. */
--
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by 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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003023972
@xiaoxiang781216 changing `tcb_s` to `pid` adds at least some reliability, but still seems to be not bulletproof. In such case I would suggest to store both `tcb_s` and `pid`, so we can use next mechanism to improve validity check: `if (pholder->htcb == nxsched_get_tcb(pholder->pid))`, so the probability that same memory block will be allocated for a task and same `pid` will be assigned is relatively low. In this case we can insert a sanity check and "dead" nodes clean-up into each semaphore operation API call.
--
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
[GitHub] [incubator-nuttx] hartmannathan edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
hartmannathan edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000898688
**This will also break many things for downstream projects using NuttX.**
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.
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.
- If this is Linux behavior and is in contradiction to POSIX, then we should **not** do it.
Also, if this change is implemented, we need to increment the major version number. It will have to be NuttX 11.x.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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 post:
```
sem_t g_sem;
void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
void *thread2_cb(void *arg)
{
sleep(2);
set_wait(&g_sem);
return NULL;
}
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);
}
```
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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 common POSIX complaint program crash after the second post:
```
sem_t g_sem;
void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
void *thread2_cb(void *arg)
{
sleep(2);
set_wait(&g_sem);
return NULL;
}
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);
}
```
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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 sem_set_protocol(SEM_NONE) activate the priority inheritance by default which make the follow common POSIX complaint program crash after the second post:
```
sem_t g_sem;
void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
void *thread2_cb(void *arg)
{
sleep(2);
set_wait(&g_sem);
return NULL;
}
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);
}
```
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.
--
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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003023972
@xiaoxiang781216 changing `tcb_s` to `pid` adds at least some reliability, but still seems to be not bulletproof. In such case I would suggest to store both `tcb_s` and `pid`, so we can use next mechanism to improve validity check: `if (pholder->htcb == nxsched_get_tcb(pholder->pid))`, so the probability that same memory block will be allocated for a task and same `pid` will be assigned is relatively low. In this case we can insert a sanity check and "dead" nodes clean-up to each semaphore operation.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003023972
@xiaoxiang781216 changing `tcb_s` to `pid` adds at least some reliability, but still seems to be not bulletproof. In such case I would suggest to store both `tcb_s` and `pid`, so we can use next mechanism to improve validity check: `if (pholder->htcb == nxsched_get_tcb(pholder->pid))`.
--
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1006219723
> > > Another real case: #5170, both case will generate hard fault.
> >
> >
> > I run ostest with #5171, no such assertions were found.
> > I think priority inheritance should only be valid for mutex, so there is only one holder per semphore at most.
> > The current implementation still cannot avoid priority flipping completely.
> > For example, there are 3 tasks t2 > t1 > t0, t0 held sem1, t1 held sem2 and wait sem1, then t2 wait sem2. In this case, only t1's priority is raised.
> > If multiple holders are supported, this problem is very difficult to solve, which is the O(n^2) level.
> > If only one holder is supported, many things can be simplified.
>
> The semaphores are the building blocks for the pthread mutexes and here is some description for pthread mutexes:
>
I mean when the semphore is used as mutex. If not, priority inheritance is not necessary.
> > When a thread makes a call to pthread_mutex_lock(), the mutex was initialized with the protocol attribute having the value PTHREAD_PRIO_INHERIT, when the calling thread is blocked because the mutex is owned by another thread, that owner thread shall inherit the priority level of the calling thread as long as it continues to own the mutex. The implementation shall update its execution priority to the maximum of its assigned priority and all its inherited priorities. Furthermore, if this owner thread itself becomes blocked on another mutex with the protocol attribute having the value PTHREAD_PRIO_INHERIT, the same priority inheritance effect shall be propagated to this other owner thread, in a recursive manner.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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:
```
sem_t g_sem;
void *thread1_cb(void *arg)
{
sem_wait(&g_sem);
return NULL;
}
void *thread2_cb(void *arg)
{
sleep(2);
sem_wait(&g_sem);
return NULL;
}
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);
}
```
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000980875
> Drivers that use the semaphores for signaling should not be involved with priority inheritance (when enabled). That use case was assumed to be the non default.
>
> It seams to me this change can be done with adding an Kconfig option for the default and leave it as it was.
Kconfig is good to disable/enable the optional feature, but it isn't a good solution here. Do you want sem_t change it's behavior by an option? And all place which call sem_init/nxsem_init need check the option and call nxsem_set_protocol with the different value.
> But allow the default to be changed if someone is using Linux code semantics.
First, it isn't Linux semantics. Second, my demo show that isn't a minor issue because the program will crash the system.
--
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000917175
Drivers that use the semaphores for signaling should not be involved with priority inheritance (when enabled). That use case was assumed to be the non default.
It seams to me this change can be done with adding an Kconfig option for the default and leave it as it was. But allow the default to be changed if someone is using Linux code semantics.
--
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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003023972
@xiaoxiang781216 changing `tcb_s` to `pid` adds at least some reliability, but still seems to be not bulletproof. In such case I would suggest to store both `tcb_s` and `pid`, so we can use next mechanism to improve validity check: `if (pholder->htcb == nxsched_get_tcb(pholder->pid))`, so the probability that same memory block will be allocated for a task and same `pid` will be assigned is relatively low. In this case we can insert a sanity check and "dead" nodes clean-up into each semaphore operation.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003017113
@anjiahao1 has a simple fix which change tcb_s to pid, so we can detect the abnormal case and remove the orphan node easily. But, we wan to fix the root cause first, and then add the safe guard in case of the wrong usage.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#discussion_r774948755
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -947,7 +947,7 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
* inheritance is effectively disabled.
*/
- if ((sem->flags & PRIOINHERIT_FLAGS_DISABLE) == 0)
+ if ((sem->flags & PRIOINHERIT_FLAGS_ENABLE) == 1)
Review comment:
!= 0
##########
File path: libs/libc/semaphore/sem_getprotocol.c
##########
@@ -56,7 +56,7 @@ int sem_getprotocol(FAR sem_t *sem, FAR int *protocol)
DEBUGASSERT(sem != NULL && protocol != NULL);
#ifdef CONFIG_PRIORITY_INHERITANCE
- if ((sem->flags & PRIOINHERIT_FLAGS_DISABLE) != 0)
+ if ((sem->flags & PRIOINHERIT_FLAGS_ENABLE) == 0)
Review comment:
if ((sem->flags & PRIOINHERIT_FLAGS_ENABLE) != 0)
{
*protocol = SEM_PRIO_INHERIT;
}
else
{
*protocol = SEM_PRIO_NONE;
}
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000980875
> Drivers that use the semaphores for signaling should not be involved with priority inheritance (when enabled). That use case was assumed to be the non default.
Why non default? semaphore use as signaling is a normal practice in POSIX. On the other hand, most program use POSIX pthread mutex as lock.
I have to say that it's very bad choice to enable priority inheritance by default when priority inheritance get implemented, because:
1. It's an optional feature, which mean that the code which use sem_t but not call nxsem_set_protocol(NuttX specific), the behavior will change with/without enable this feature.
2. Since priority inheritance is the default setting, which mean the wrong usage will crash the system. It's more serious than the priority inversion.
>
> It seams to me this change can be done with adding an Kconfig option for the default and leave it as it was.
Kconfig is good to disable/enable the optional feature, but it isn't a good solution here. Do you want sem_t change it's behavior by an option? And all place which call sem_init/nxsem_init need check the option and call nxsem_set_protocol with the different value.
> But allow the default to be changed if someone is using Linux code semantics.
First, it isn't Linux semantics. Second, my demo show that isn't a minor issue because the program will crash the system.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000908819
> 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;
}
```
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng removed a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng removed a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005610175
>
I have pushed the modificaiton, let's discuss in the PR.
https://github.com/apache/incubator-nuttx/pull/5171
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1006443287
I started similar `pthread mutex` related activity in https://github.com/apache/incubator-nuttx/pull/5180
--
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
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1005614899
@pkarashchenko I have push the modification, let's discuss in the PR.
https://github.com/apache/incubator-nuttx/pull/5171
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1000869809
> This is a massive breaking change. Please do not merge this until there is time after the holidays to get proper feed back from the reviews listed.
Yes, this is a partial change to show the issue and potential modification. If the community agree the fix after review, all [nx]sem_init need review and modify.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003017113
@anjiahao1 has a simple fix which change tcb_s to pid, so we can detect the abnormal case and remove the orphan node easily. But, we want to fix the root cause first, and then add the safe guard in case of the wrong usage.
--
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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1002780625
Seems that disabling priority inheritance by default is the right way of doing things. My arguments are that by default priority inheritance is used to struggle with resource guarding issue (mutual exclusion use case) and does not have much sense in case of event signaling use case. The POSIX semaphores however have have more wide range then mutual exclusion use case and it is more a coincidence that `phtread_mutex`s are implemented on top of POSIX semaphores in NuttX (I just want to emphasize that `phtread_mutex`s could have their own implementation as an alternative and are not tight together with semaphores in general). `pthread_mutex`s are will still have priority inheritance enabled by default and that is fine because I truly believe that when NuttX user enables priority inheritance that is the exactly use case that needs to be handled by this feature.
Summarizing above: my vote is to change the default protocol for POSIX semaphores and proceed with this change.
--
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
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1003023972
@xiaoxiang781216 changing `tcb_s` to `pid` adds at least some reliability, but still seems to be not bulletproof. In such case I would suggest to store both `tcb_s` and `pid`, so we can use next mechanism to improve validity check: `if (pholder->htcb == nxsched_get_tcb(pholder->pid))`, so the probability that same memory block will be allocated for a task and same `pid` will be assigned is relatively low.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5070: libc/semaphore:sem_init change defult protocol
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5070:
URL: https://github.com/apache/incubator-nuttx/pull/5070#issuecomment-1006118747
> > Another real case: #5170, both case will generate hard fault.
>
> I run ostest with https://github.com/apache/incubator-nuttx/pull/5171, no such assertions were found.
>
> I think priority inheritance should only be valid for mutex, so there is only one holder per semphore at most.
>
> The current implementation still cannot avoid priority flipping completely.
> For example, there are 3 tasks t2 > t1 > t0, t0 held sem1, t1 held sem2 and wait sem1, then t2 wait sem2. In this case, only t1's priority is raised.
> If multiple holders are supported, this problem is very difficult to solve, which is the O(n^2) level.
>
> If only one holder is supported, many things can be simplified.
>
>
The semaphores are the building blocks for the pthread mutexes and here is some description for pthread mutexes:
> When a thread makes a call to pthread_mutex_lock(), the mutex was initialized with the protocol attribute having the value PTHREAD_PRIO_INHERIT, when the calling thread is blocked because the mutex is owned by another thread, that owner thread shall inherit the priority level of the calling thread as long as it continues to own the mutex. The implementation shall update its execution priority to the maximum of its assigned priority and all its inherited priorities. Furthermore, if this owner thread itself becomes blocked on another mutex with the protocol attribute having the value PTHREAD_PRIO_INHERIT, the same priority inheritance effect shall be propagated to this other owner thread, in a recursive manner.
--
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