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 2022/01/06 10:11:03 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
pkarashchenko opened a new pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180
## Summary
According to https://pubs.opengroup.org/onlinepubs/9699919799 the `The protocol attribute defines the protocol to be followed in utilizing mutexes.` and `The default value of the attribute shall be PTHREAD_PRIO_NONE.`
Currently when `CONFIG_PRIORITY_INHERITANCE` is enabled the default priority is set to `PTHREAD_PRIO_INHERIT` that violates the spec.
## Impact
All applications that rely on `pthread_mutex_t`
## Testing
WIP. Probably many apps need to be retested and updated
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006521448
If the usage is fixed as lock(e.g. pthread_mutex_t, mutex_t), it's good to enable the priority inheritance by default. If the usage isn't fixed like sem_t, we have to disable the priority inheritance by default.
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006523562
> > @xiaoxiang781216 do you think the priority inheritance as default protocol should be enabled by default?
>
> @pkarashchenko I think enabling priority inheritance as default could be a good idea, but I don't know what could be the drawbacks of doing so. Do you remember the NASA Pathfinder issue? Actually VxWorks had support to priority inheritance, but NASA engineers forgot to enable it. So, if we enable priority inheritance by default we could avoid that others "NASA engineers" forget to enable it! :-)
@acassis I really do not know what is the best. From one hand I see that "Strict POSIX compliance" is top one of "Inviolable Principles of NuttX" and from other hand we are having "NASA engineers" and I'm a bit confused who has the higher priority :)
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006462476
@xiaoxiang781216 do you think it should be enabled by default?
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780519890
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
Done
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1007835849
> How does the choice with both setting as default work?
>
> Seeing the defconfig change:
>
> CONFIG_PRIORITY_INHERITANCE=y
> CONFIG_PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT=y
>
> Makes me question the default Kconfig logic.
By default the pthread mutex priority is `PTHREAD_PRIO_NONE` (according to POSIX) either in case if `CONFIG_PRIORITY_INHERITANCE` is on or off. In case if default priority is selected to `CONFIG_PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT` the default priority becomes `PTHREAD_PRIO_INHERIT` for all pthread mutexes. In this case `pthread_mutex_init(&mutex, NULL);` is equivalent to `pthread_mutexattr_setprotocol(&attr, PTHREAD_PRIO_INHERIT);` + `pthread_mutex_init(&mutex, &attr);`.
--
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] acassis commented on pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006498805
> @xiaoxiang781216 do you think the priority inheritance as default protocol should be enabled by default?
@pkarashchenko I think enabling priority inheritance as default could be a good idea, but I don't know what could be the drawbacks of doing so. Do you remember the NASA Pathfinder issue? Actually VxWorks had support to priority inheritance, but NASA engineers forgot to enable it. So, if we enable priority inheritance by default we could avoid that others "NASA engineers" forget to enable it! :-)
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780516418
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed 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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006553929
> > > If the usage is fixed as lock(e.g. pthread_mutex_t, mutex_t), it's good to enable the priority inheritance by default. If the usage isn't fixed like sem_t, we have to disable the priority inheritance by default.
> >
> >
> > So we need to violate POSIX compliance by default, right?
>
> We can add an option like pthread_mutex_t: https://github.com/apache/incubator-nuttx/blob/master/sched/Kconfig#L644-L657
Yes. But in this example `default PTHREAD_MUTEX_DEFAULT_ROBUST`, so should we `default PTHREAD_MUTEX_PROTO_DEFAULT_NONE` or `default PTHREAD_MUTEX_PROTO_DEFAULT_INHERIT`?
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1007835849
> How does the choice with both setting as default work?
>
> Seeing the defconfig change:
>
> CONFIG_PRIORITY_INHERITANCE=y
> CONFIG_PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT=y
>
> Makes me question the default Kconfig logic.
By default the pthread mutex priority is `PTHREAD_PRIO_NONE` (according to POSIX) either in case if `CONFIG_PRIORITY_INHERITANCE` is on or off. In case if default priority is selected to `CONFIG_PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT` the default priority becomes `PTHREAD_PRIO_INHERIT` for all pthread mutexes. In this case `pthread_mutex_init(&mutex, NULL);` is equivalent to `pthread_mutexattr_setprotocol(&attr, PTHREAD_PRIO_INHERIT);` + `pthread_mutex_init(&mutex, &attr);`.
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780094017
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
also `attr->proto` is not accessible if `CONFIG_PRIORITY_INHERITANCE=n`
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
I think maybe only next:
```
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
return OK;
#else
return OK;
case PTHREAD_PRIO_INHERIT:
#endif
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
But IMO this is ugly.
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
Yes. This one should work fine.
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed change
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed change.
I would like not to introduce `-Wimplicit-fallthrough` warnings as `[[fallthrough]];` can't be used in common code
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed change.
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
Done
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r779998603
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
·
can simplify to:
```
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
#endif
attr->proto = protocol;
return OK;
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006808808
> > > It's minor issue, I am fine with either:).
> >
> >
> > Done. @xiaoxiang781216 @acassis please take a look.
>
> So, let see if I understood correctly: when CONFIG_PRIORITY_INHERITANCE is enabled only PTHREAD_PRIO_PROTECT _cannot_ be used. When CONFIG_PRIORITY_INHERITANCE is not enabled only PTHREAD_PRIO_NONE _can_ be used, correct?
Yes. The PRIO_PROTECT (aka priority priority ceilings) is not implemented currently. so we have either `CONFIG_PRIORITY_INHERITANCE=n` that leads to `PTHREAD_PRIO_NONE` option only or choice between `PTHREAD_PRIO_NONE` and `PTHREAD_PRIO_INHERIT` when `CONFIG_PRIORITY_INHERITANCE=y`.
--
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 merged pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5180:
URL: 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] pkarashchenko commented on a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780516418
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed 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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780049007
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
This will not return `ENOTSUP` in case if priority inheritance is disabled
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780132591
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
Yes. This one should work fine.
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006459951
> Since phtread_mutex_t is always used as lock, the priority inherit isn't harmful as the sem_t used as signal. So an option to select the default value is reasonable here, I think.
Yes. I support it. We can have an option to override the spec if this is a desired behavior. I will add a config option.
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780108849
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
how about:
```
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
#endif
return OK;
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006454958
Since phtread_mutex_t is always used as lock, the priority inherit isn't harmful as the sem_t used as signal. So an option to select the default value is reasonable here, I think.
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780516418
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
@xiaoxiang781216 anyway I do not see any benefit in such rework I will integrate a proposed change.
I would like not to introduce `-Wimplicit-fallthrough` warnings as `[[fallthrough]];` can't be used in common code
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006462476
@xiaoxiang781216 do you think the priority inheritance should be enabled by default?
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780125534
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
I think maybe only next:
```
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
return OK;
#else
return OK;
case PTHREAD_PRIO_INHERIT:
#endif
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
But IMO this is ugly.
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780129746
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
this one?
```
if (protocol < PTHREAD_PRIO_NONE || protocol > PTHREAD_PRIO_PROTECT)
{
return EINVAL;
}
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
#endif
return OK;
default:
return ENOTSUP;
}
```
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006558468
It's minor issue, I am fine with either:).
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006808808
> > > It's minor issue, I am fine with either:).
> >
> >
> > Done. @xiaoxiang781216 @acassis please take a look.
>
> So, let see if I understood correctly: when CONFIG_PRIORITY_INHERITANCE is enabled only PTHREAD_PRIO_PROTECT _cannot_ be used. When CONFIG_PRIORITY_INHERITANCE is not enabled only PTHREAD_PRIO_NONE _can_ be used, correct?
Yes. The PRIO_PROTECT (aka priority priority ceilings) is not implemented currently. So we have either `CONFIG_PRIORITY_INHERITANCE=n` that leads to `PTHREAD_PRIO_NONE` option only or choice between `PTHREAD_PRIO_NONE` and `PTHREAD_PRIO_INHERIT` when `CONFIG_PRIORITY_INHERITANCE=y`.
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780094017
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
also `attr->proto` is not accessible if `CONFIG_PRIORITY_INHERITANCE=n`
--
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 a change in pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780968955
##########
File path: sched/Kconfig
##########
@@ -658,6 +658,25 @@ config PTHREAD_MUTEX_DEFAULT_UNSAFE
endchoice # Default NORMAL mutex robustness
+choice
+ prompt "Default pthread mutex protocol"
+ default PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+ bool "PTHREAD_PRIO_NONE default"
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_NONE protocol
+ (standard)
+
Review comment:
Done
##########
File path: sched/Kconfig
##########
@@ -658,6 +658,25 @@ config PTHREAD_MUTEX_DEFAULT_UNSAFE
endchoice # Default NORMAL mutex robustness
+choice
+ prompt "Default pthread mutex protocol"
+ default PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+ bool "PTHREAD_PRIO_NONE default"
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_NONE protocol
+ (standard)
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT
+ bool "PTHREAD_PRIO_INHERIT default"
+ depends on PRIORITY_INHERITANCE
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_INHERIT protocol
+ (non-standard, but might be the choice for most of real-time systems)
+
Review comment:
Done
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780791275
##########
File path: sched/Kconfig
##########
@@ -658,6 +658,25 @@ config PTHREAD_MUTEX_DEFAULT_UNSAFE
endchoice # Default NORMAL mutex robustness
+choice
+ prompt "Default pthread mutex protocol"
+ default PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+ bool "PTHREAD_PRIO_NONE default"
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_NONE protocol
+ (standard)
+
Review comment:
How about:
```
---help---
By default, pthread mutexes utilize PTHREAD_PRIO_NONE protocol (standard).
```
##########
File path: sched/Kconfig
##########
@@ -658,6 +658,25 @@ config PTHREAD_MUTEX_DEFAULT_UNSAFE
endchoice # Default NORMAL mutex robustness
+choice
+ prompt "Default pthread mutex protocol"
+ default PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_NONE
+ bool "PTHREAD_PRIO_NONE default"
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_NONE protocol
+ (standard)
+
+config PTHREAD_MUTEX_DEFAULT_PRIO_INHERIT
+ bool "PTHREAD_PRIO_INHERIT default"
+ depends on PRIORITY_INHERITANCE
+ ---help---
+ The default is the mutexes utilizing PTHREAD_PRIO_INHERIT protocol
+ (non-standard, but might be the choice for most of real-time systems)
+
Review comment:
How about:
```
---help---
By default, pthread mutexes utilize PTHREAD_PRIO_INHERIT protocol
(not POSIX-standard but a reasonable choice for most real-time systems).
```
or something along those lines?
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006525216
> If the usage is fixed as lock(e.g. pthread_mutex_t, mutex_t), it's good to enable the priority inheritance by default. If the usage isn't fixed like sem_t, we have to disable the priority inheritance by default.
So we need to violate POSIX compliance by default, right?
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r779998603
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
·
can simplify to:
···
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
#endif
attr->proto = protocol;
return OK;
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
}
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006462476
@xiaoxiang781216 do you think the priority inheritance as default protocol should be enabled by default?
--
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] acassis commented on pull request #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006758270
> > It's minor issue, I am fine with either:).
>
> Done. @xiaoxiang781216 @acassis please take a look.
So, let see if I understood correctly: when CONFIG_PRIORITY_INHERITANCE is enabled only PTHREAD_PRIO_PROTECT *cannot* be used. When CONFIG_PRIORITY_INHERITANCE is not enabled only PTHREAD_PRIO_NONE *can* be used, correct?
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006548893
> > If the usage is fixed as lock(e.g. pthread_mutex_t, mutex_t), it's good to enable the priority inheritance by default. If the usage isn't fixed like sem_t, we have to disable the priority inheritance by default.
>
> So we need to violate POSIX compliance by default, right?
We can add an option like pthread_mutex_t:
https://github.com/apache/incubator-nuttx/blob/master/sched/Kconfig#L644-L657
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1006736204
> It's minor issue, I am fine with either:).
Done.
@xiaoxiang781216 @acassis please take a look.
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#discussion_r780108849
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
how about:
```
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
#endif
return OK;
case PTHREAD_PRIO_PROTECT:
return ENOTSUP;
default:
return EINVAL;
}
```
##########
File path: libs/libc/pthread/pthread_mutexattr_setprotocol.c
##########
@@ -54,21 +54,24 @@ int pthread_mutexattr_setprotocol(FAR pthread_mutexattr_t *attr,
linfo("attr=0x%p protocol=%d\n", attr, protocol);
DEBUGASSERT(attr != NULL);
-#ifdef CONFIG_PRIORITY_INHERITANCE
if (protocol >= PTHREAD_PRIO_NONE && protocol <= PTHREAD_PRIO_PROTECT)
Review comment:
this one?
```
if (protocol < PTHREAD_PRIO_NONE || protocol > PTHREAD_PRIO_PROTECT)
{
return EINVAL;
}
switch (protocol)
{
case PTHREAD_PRIO_NONE:
#ifdef CONFIG_PRIORITY_INHERITANCE
case PTHREAD_PRIO_INHERIT:
attr->proto = protocol;
#endif
return OK;
default:
return ENOTSUP;
}
```
--
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 #5180: pthread: restore pthread mutex default protocol POSIX compatibility
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5180:
URL: https://github.com/apache/incubator-nuttx/pull/5180#issuecomment-1008366792
> Is there a dependency between this and [PR-5070](https://github.com/apache/incubator-nuttx/pull/5070)?
There is indirect dependency. `pthread mutex` is based on semaphore so initially inherited the same behavior in case if `CONFIG_PRIORITY_INHERITANCE` is enabled. The semaphore default behavior is not regulated by the standard while for `pthread mutex` we have clear description from the OpenGroup.
--
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